-
Notifications
You must be signed in to change notification settings - Fork 219
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(tracing): Add a method to get transaction for given span: GetTransaction() #558
Conversation
Codecov ReportBase: 75.47% // Head: 75.48% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #558 +/- ##
==========================================
+ Coverage 75.47% 75.48% +0.01%
==========================================
Files 37 37
Lines 3751 3765 +14
==========================================
+ Hits 2831 2842 +11
Misses 808 808
- Partials 112 115 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View 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.
I think this is mostly OK for the time being.
No hard feelings about the name, it's Span::getTransaction
in PHP as well.
Slightly OT, but I think this is a good example of why we should add proper abstractions for Transactions/Spans.
https://github.com/getsentry/sentry-php/blob/bfa3193944fe385af7d6b6b063962fc45956a817/src/Tracing/Span.php#L390-L406
I kinda like the way all parent properties are explicitly set on a child span.
Co-authored-by: Michi Hoffmann <[email protected]>
Add a function to get the transaction span for the given span:
Span.GetTransaction()
This is needed for a proper fix for #553.
Things left to agree on: Function nameIn Python, for example, there's acontaining_transaction
method on Spans and Transactions.