sentry-java icon indicating copy to clipboard operation
sentry-java copied to clipboard

Fix: Do not add breadcrumbs on the root scope.

Open maciejwalkowiak opened this issue 3 years ago • 3 comments

: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

maciejwalkowiak avatar Jan 06 '22 11:01 maciejwalkowiak

Codecov Report

Merging #1864 (ae58390) into main (9f87477) will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@             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 data Powered by Codecov. Last update 9f87477...ae58390. Read the comment docs.

codecov-commenter avatar Jan 06 '22 11:01 codecov-commenter

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.init then 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.

marandaneto avatar Jan 12 '22 10:01 marandaneto

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.

maciejwalkowiak avatar Jan 13 '22 07:01 maciejwalkowiak

Closing this now due to lack of activity. Feel free to reopen if we still need this.

adinauer avatar Jul 17 '23 07:07 adinauer