-
Notifications
You must be signed in to change notification settings - Fork 424
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
Add a method (and benchmark) to Context to speed up active span determination by 7x #1140
Conversation
- no need to clone current context for each processor - use Context::map_current instead, outside loop, to compute optional TraceContext once
A 7x win sounds impressive!
I don't have enough context on the obviousness of the lifetime issue, can you elaborate a bit?
Presumably everyone following along would not be surprised to hear that I wholeheartedly agree with this. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1140 +/- ##
=======================================
- Coverage 49.8% 49.7% -0.1%
=======================================
Files 171 171
Lines 20254 20284 +30
=======================================
+ Hits 10092 10100 +8
- Misses 10162 10184 +22
☔ View full report in Codecov by Sentry. |
Sure! The current context is owned by a |
+1 from |
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.
Looks reasonable to me, the risk of others misusing this api by calling map_current in a way that causes your app to panic through the double borrow at runtime is probably worth it to avoid the clones in performance sensitive cases.
In my use case for tracing instrumentation, I only want to pay the costs of creating a new context with a new span if the current context contains an active span, but I find the current cost associated with simply checking for this condition to be unacceptably high.
Simply calling
Context::current().has_active_span()
does the following:Changes
I added a generic method to
Context
calledmap_current
which, instead of cloning, allows the caller to pass a lambda to be invoked with a reference to the current context (if any) and returns a value of some type of their choosing.With this facility, checking for an active span can look like:
I added a benchmark to demonstrate the cost difference, and it is substantial.
I think this new API feels like a bit of a hack compared with the completely idiomatic way of getting the current context in the first place, which would more resemble:
were it not for the obvious lifetime issue of the returned reference. So I took the standard
map
approach of using the lambda's scope to work with the valid reference to the current context and let the caller algebraically build up whatever construct they'd like to return.I also think there is this tension between the OpenTelemetry spec, which seems to bias/favor towards full-blown "empty" values (e.g.
Context::default()
orNoopSpan
) to represent something that is "absent", rather than promoting the powerful facilities of the particular language binding (e.g.Option<Context>
orOption<Span>
). This is related to #1089, for example.I would like to see us continue to improve the idiomatic aspects of this Rust binding of the OpenTelemetry API with the goal of having this binding be the absolute highest performing one out there among them all. I think that embracing algebraic types more is key to achieving this.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes