-
Notifications
You must be signed in to change notification settings - Fork 423
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
Simplify expression and improve perf to get parent span id #1088
Conversation
- code is a little easier to understand and shows about 4-6% perf improvement in start-end-span benchmarks
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1088 +/- ##
=======================================
- Coverage 50.9% 50.7% -0.2%
=======================================
Files 165 165
Lines 19689 19789 +100
=======================================
+ Hits 10025 10040 +15
- Misses 9664 9749 +85
☔ View full report in Codecov by Sentry. |
I think this is a good direction, but IMO it doesn't quite go far enough, so submitted #1089. Please have a look -- would be great to hear how that change affects your benchmarks. |
SpanId::INVALID, | ||
builder | ||
.trace_id | ||
.unwrap_or_else(|| config.id_generator.new_trace_id()), |
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.
Hm I don't think this will have identical behavior to the previous implementation as children of invalid spans may be valid (have valid trace ids if not explicitly set to invalid in builder).
Is the perf similar if you get the parent span context once and then check if it's valid subsequently, rather than checking if there is an active span before extracting the span context? |
Superceded by #1089 |
Changes
Avoids calling
Context.has_active_span()
followed byContext.span()
, which is two lookups,and instead relies on checking for an invalid span context on the return value of the latter.
Code is a little easier to understand and shows about 4-6% perf improvement in start-end-span benchmarks.
My assumption here is that a span with an invalid span context could never be made active in the first place.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes