Remove SpanID from sampling parameters #1286
Conversation
pjanotti
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
pavolloffay,
thisthat and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1286 +/- ##
============================================
+ Coverage 86.21% 86.31% +0.10%
- Complexity 1212 1214 +2
============================================
Files 154 153 -1
Lines 4518 4546 +28
Branches 405 411 +6
============================================
+ Hits 3895 3924 +29
+ Misses 478 471 -7
- Partials 145 151 +6
Continue to review full report at Codecov.
|
| spanKind, | ||
| immutableAttributes, | ||
| immutableLinks); | ||
| parentContext, traceId, spanName, spanKind, immutableAttributes, immutableLinks); |
There was a problem hiding this comment.
One of the goals of this change in the spec was to enable deferring the generation of a span id when one wasn't needed. Is it possible to defer the spanId creation, and if not, why not?
There was a problem hiding this comment.
@jkwatson To be clear, you are talking about SpanId being crated unconditionally before this line? e.g.
SpanId spanId = idsGenerator.generateSpanId();
?
There was a problem hiding this comment.
@carlosalberto Exactly, this was mentioned on the original spec PR (open-telemetry/opentelemetry-specification#621) to improve performance.
There was a problem hiding this comment.
Follow-up idea (for spec): why not also generate trace ID later? And provide an option for samplers that need it (only one single one: ProbabilitySampler) to optionally generate a trace ID themselves that the SDK will use.
There was a problem hiding this comment.
Please move this conversation to an issue. I think it is important to fix API first then we can address the optimizations.
There was a problem hiding this comment.
Yeah, I was focusing on avoiding samplers starting to use the SpanId and missed the optimization. Let me see what can be done.
There was a problem hiding this comment.
Changed the code to use SpanId.getInvalid() if the span is not sampled and only generate a new SpanId if it is sampled.
There was a problem hiding this comment.
Please revert the last commit, and don't do the optimization now.
There was a problem hiding this comment.
Done, reverted the optimization - didn't see your comment on GH prior to the change...
|
Thanks for the PR! My only concern is that @jkwatson mention of us still needing to create a |
| spanKind, | ||
| immutableAttributes, | ||
| immutableLinks); | ||
| parentContext, traceId, spanName, spanKind, immutableAttributes, immutableLinks); |
There was a problem hiding this comment.
Please revert the last commit, and don't do the optimization now.
This reverts commit 5b51eae.
Update code per latest spec, see:
open-telemetry/opentelemetry-specification#621
The goal is to avoid any SDK implementation from taking a dependency on it.