getsentry / getsentry/sentry-java

Replace misleading IScopeObserver.setBreadcrumbs with clearBreadcrumbs

Aberta
#5,844 1 comentário 0 reações 0 responsáveis Ver no GitHub
good first issue Platform: Java
Linguagem predominante
Kotlin
Estrelas
1.4k
Forks
478
Merge médio
3d 4h
PRs com merge (30d)
72

Descrição

`IScopeObserver.setBreadcrumbs(Collection)` reads like a bulk setter — "replace the observed breadcrumbs with this collection" — but that isn't its contract. Its real meaning is *"the breadcrumbs were cleared"*, encoded as *"the collection I passed you happens to be empty."*

`PersistingScopeObserver` is the only implementation that does anything, and it never looks at the elements:

```java
public void setBreadcrumbs(@NotNull Collection breadcrumbs) {
if (breadcrumbs.isEmpty()) {
// ...enqueue a clear...
}
// non-empty: silently do nothing
}
```

## Why this is a problem

1. **The parameter is a boolean in disguise.** The only thing read off the collection is `isEmpty()`. A non-empty argument is silently ignored, so anyone who takes the name at face value and writes a bulk-replace implements something the SDK never drives that way.
2. **It hides a real race.** `Scope.clearBreadcrumbs()` clears the live queue and then hands *that same live queue* to the observers. If another thread adds a breadcrumb between the `clear()` and the observer loop, the collection is no longer empty, `PersistingScopeObserver` does nothing, and the pre-clear breadcrumbs survive on disk — cleared in memory, not cleared in the scope cache. The signal is carried by mutable state observed after the fact instead of by the call itself. Narrow in practice (`clearBreadcrumbs()` isn't on a hot path), and the symptom is stale breadcrumbs on the next ANR/crash report rather than anything immediately user-visible.
3. **It's called constantly for nothing.** `Scope.addBreadcrumb` invokes both `addBreadcrumb` and `setBreadcrumbs` on every observer for every breadcrumb. The second call can only ever be a no-op (the collection just had an element added), so we pay an extra virtual call per observer per breadcrumb on a hot path — and it's why the batching work in getsentry/sentry-java#5714 needed extra care around clear ordering.
4. **It's inconsistent with the rest of the interface.** Attachments already do this correctly: `addAttachment` / `clearAttachments`. Breadcrumbs are the odd one out.
5. **The name collides with a genuine setter.** `SentryBaseEvent.setBreadcrumbs(List)` really does replace the list, so the same name means two different things depending on the receiver.

## Proposed change

* Add `clearBreadcrumbs()` to `IScopeObserver` and `ScopeObserverAdapter`.
* `Scope.clearBreadcrumbs()` calls `observer.clearBreadcrumbs()`; `Scope.addBreadcrumb` drops its `observer.setBreadcrumbs(...)` call entirely.
* `PersistingScopeObserver.setBreadcrumbs` becomes `clearBreadcrumbs()`, unconditionally enqueueing the clear marker — which also removes the race in (2).
* Remove `setBreadcrumbs` from `IScopeObserver`. It's public API and not `@ApiStatus.Internal`, hence filing this against the 9.0 major.
* Fix `IScopeObserver`'s javadoc, which claims all methods are `default` — none of them are.
* Update `ScopeTest`, `PersistingScopeObserverTest`, and `PersistingScopeObserverBatchingTest`, which currently express "clear" as `setBreadcrumbs(emptyList())`.

## Notes

`NdkScopeObserver` implements `addBreadcrumb` but not `setBreadcrumbs`, so native breadcrumbs are never cleared today. A `clearBreadcrumbs()` on the interface makes that gap visible; there's no corresponding entry point on `INativeScope`, so closing it would be follow-up work.

**Affected files:** `sentry/src/main/java/io/sentry/IScopeObserver.java`, `ScopeObserverAdapter.java`, `Scope.java`, `cache/PersistingScopeObserver.java`, `sentry/api/sentry.api`

Guia de contribuição

Abrir o guia de contribuição

Direção de pesquisa

Comece por IScopeObserver.java, ScopeObserverAdapter.java e Scope.java para rastrear as chamadas do observer e, em seguida, inspecione PersistingScopeObserver.java e os testes afetados. Atualize a superfície da API e os testes mencionados na issue para que a limpeza use clearBreadcrumbs(), e verifique se os testes de batching e de scope cobrem o novo comportamento e não expressam mais a limpeza por meio de setBreadcrumbs(emptyList()).

Escrita pelo modelo de indexação a partir do texto da issue.

Avaliação

Stack de tecnologia
java
Domínio
backend-api-design
Tipo de issue
Refatoração
Dificuldade
4/5
Tempo estimado
3-5 dias
Status de atividade
Pouca atividade
Clareza
Claramente especificada
Facilidade para iniciantes
56/100

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.