Fix: Discard unfinished Spans before sending them over to Sentry#1279
Fix: Discard unfinished Spans before sending them over to Sentry#1279maciejwalkowiak merged 6 commits intomainfrom
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
|
A question from #1262 (comment) must be answered before we can merge it |
Codecov Report
@@ Coverage Diff @@
## main #1279 +/- ##
============================================
+ Coverage 75.04% 75.06% +0.02%
- Complexity 1752 1754 +2
============================================
Files 183 183
Lines 6159 6165 +6
Branches 610 612 +2
============================================
+ Hits 4622 4628 +6
Misses 1258 1258
Partials 279 279
Continue to review full report at Codecov.
|
| unfinishedSpans.add(span); | ||
| } | ||
| } | ||
| transaction.getSpans().removeAll(unfinishedSpans); |
There was a problem hiding this comment.
Would be nice to log out. Maybe check if logging is enabled to warn level, if yes we log count of dropped span and the operations?
the answer is here: getsentry/develop#274 (comment) |
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
I'd log unfinished spans and missing changelog, other than that, LGTM
| unfinishedSpans.add(span); | ||
| } | ||
| } | ||
| if (options.getLogger().isEnabled(SentryLevel.WARNING) && !unfinishedSpans.isEmpty()) { |
There was a problem hiding this comment.
| if (options.getLogger().isEnabled(SentryLevel.WARNING) && !unfinishedSpans.isEmpty()) { | |
| if (!unfinishedSpans.isEmpty()) { |
There was a problem hiding this comment.
options.getLogger().log already checks if its enabled internally, !unfinishedSpans.isEmpty() isn't a heavy call I believe, it'd be fine logging directly
4 participants
📜 Description
Discard unfinished Spans before sending them over to Sentry
💡 Motivation and Context
Fixes #1262
💚 How did you test it?
Unit tests.
📝 Checklist