-
-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Include unfinished spans in transactions #1592
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #1592 +/- ##
=======================================
Coverage 95.94% 95.95%
=======================================
Files 157 157
Lines 7061 7068 +7
=======================================
+ Hits 6775 6782 +7
Misses 286 286
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, two comments to address.
Sources/Sentry/SentryTracer.m
Outdated
if (finishedSpan != self.rootSpan) { | ||
// calls finish on the rootSpan. If the root span is finished, it alread finished every | ||
// unfinished child. | ||
if (finishedSpan != self.rootSpan && !self.rootSpan.isFinished) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: I think it would be better to avoid the endless loop in canBeFinished
not here. So canBeFinished
should check if finishedSpan != self.rootSpan
.
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks 👏
📜 Description
Instead of removing unfinished spans from the transaction in order to avoid the transaction to be discarded, we close all unfinished spans with
deadline_exceeded
status.💡 Motivation and Context
closes #1303
💚 How did you test it?
Unit tests
📝 Checklist
🔮 Next steps