sentry-java
sentry-java copied to clipboard
Fix: Do not add breadcrumbs on the root scope.
:scroll: Description
Do not add breadcrumbs on the root scope.
:bulb: Motivation and Context
As discussed with @bruno-garcia, breadcrumbs added to the root scope will be attached to all events captured in child scopes, which can lead to reaching max breadcrumbs, if scope is not pushed. The proposed solution by @bruno-garcia is to not add breadcrumbs in root scope.
:green_heart: How did you test it?
Unit tests.
:pencil: Checklist
- [x] I reviewed the submitted code
- [x] I added tests to verify the changes
- [ ] I updated the docs if needed
- [x] No breaking changes
Codecov Report
Merging #1864 (ae58390) into main (9f87477) will increase coverage by
0.02%. The diff coverage is100.00%.
@@ Coverage Diff @@
## main #1864 +/- ##
============================================
+ Coverage 75.51% 75.53% +0.02%
- Complexity 2239 2242 +3
============================================
Files 225 225
Lines 8001 8008 +7
Branches 846 848 +2
============================================
+ Hits 6042 6049 +7
Misses 1549 1549
Partials 410 410
| Impacted Files | Coverage Δ | |
|---|---|---|
| sentry/src/main/java/io/sentry/Hub.java | 74.93% <100.00%> (+0.20%) |
:arrow_up: |
| sentry/src/main/java/io/sentry/Stack.java | 97.56% <100.00%> (+0.26%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 9f87477...ae58390. Read the comment docs.
The change makes sense and solves a bug on Webflux but I wonder if we should do this under an opt-in option.
The reasoning is that we could be breaking CLI tooks that simply call:
Sentry.initthen add some crumbs. That's totally legal in that environment. We just want to avoid this in server-mode if I got things right?
This, I guess we need to rethink this solution and why this is a problem.
breadcrumbs added to the root scope will be attached to all events captured in child scopes, which can lead to reaching max breadcrumbs
Why is it a bug? it's the intended behavior, I'd like to understand the motivation of the change, I lack context here.
It's not only about Webflux, but in general about any long running thread where scope is not pushed.
For example when sentry-spring-boot-starter and sentry-logback are added as dependencies, and user has a scheduled job:
@Scheduled(fixedRate = 10000)
void handle() {
LOGGER.info("from scheduled job");
LOGGER.error("oopsie");
}
These info & error logs are turned into breadcrumbs. Every invocation of this job happens on the same thread - without explicit push/pop scope, these breadcrumbs accumulate.
The idea of not adding breadcrumbs to root scope comes from @bruno-garcia. I am open for alternatives.
Closing this now due to lack of activity. Feel free to reopen if we still need this.