-
Notifications
You must be signed in to change notification settings - Fork 599
feat(tracer): add more convenience methods for enterings spans #3251
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
base: main
Are you sure you want to change the base?
feat(tracer): add more convenience methods for enterings spans #3251
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3251 +/- ##
=======================================
+ Coverage 80.8% 80.9% +0.1%
=======================================
Files 129 129
Lines 23203 23372 +169
=======================================
+ Hits 18750 18919 +169
Misses 4453 4453 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cijothomas
left a comment
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 good. lets add a changelog.md entry before merge.
a6e09d5 to
e0260b2
Compare
| /// ) | ||
| /// } | ||
| /// ``` | ||
| fn in_span_with_builder_and_context<T, F>( |
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.
based on our benchmarks (https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/benches/span.rs#L71) the in_span is not the most performant way to span creation/activation.
Not opposed to this change, just noting the perf implications. Maybe we can improve the implementation to make it better, and/or warn users about perf implication in the doc in follow ups.
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'll take a look at the benchmarks and extend them with versions for these variants.
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.
So the benchmarks are really comparing apples and oranges. Do we want comparable numbers or what are we trying to measure in the benchmarks? Yes, if you don't attach a Span to the Context it will be faster than attaching a Span to the Context. If you don't set an Attribute on a Span it will always be faster than setting an Attribute on a Span.
The bigger question is what type of API do we want to have here. Should we have a method that maybe attaches the Span to the Context and gives you a ref to the Span instead of the Context? This would mean that Context::current() would return a Context that doesn't have this Span attached. This feels weird and I can't really see why you would want that.
26a8454 to
f2faaec
Compare
Changes
This adds 3 convenience methods to the
Tracertrait that allows users to create, start, and enter a scope with an activeSpanwith parent contexts and based on aSpanBuilder.Right now, the easiest way to enter a
Spanis to use the convenience method on the tracer that only takes anameand then add tags to the span afterwards. This is problematic in the sense that samplers and processors that take attributes into account will not see these attributes at creation time.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes