-
Notifications
You must be signed in to change notification settings - Fork 848
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
Remove DefaultSpan from public API. #1791
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 84.60% 84.67% +0.07%
- Complexity 1429 1431 +2
============================================
Files 177 177
Lines 5554 5554
Branches 579 578 -1
============================================
+ Hits 4699 4703 +4
+ Misses 648 645 -3
+ Partials 207 206 -1
Continue to review full report at Codecov.
|
* functionality. It will not be exported and all operations are no-op, but it can be used to | ||
* propagate the {@link SpanContext} downstream. | ||
*/ | ||
static Span wrapWithNoop(SpanContext spanContext) { |
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.
@Oberon00 Based on the conversation in open-telemetry/opentelemetry-specification#1086 this should be something like wrapForPropagation
, we're wrapping this SpanContext
into a PropagatedSpan
. This seems like odd naming to me though, as there is nothing about propagation happening here really, doubly-so because propagation is still more commonly used for remote propagation than in-process propagation I think. That's not great here since we don't do remote propagation of the invalid span.
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.
as there is nothing about propagation happening here really
If one considers making a span a local parent "propagation" (which I think one should), then it makes sense. Because what other uses would this returned object have?
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.
Because what other uses would this returned object have?
I dunno maybe someone will have something creative :) While it's indirect usage of this API, one case where it will be used is when extracting the SpanContext
from a link - the result of this method will be taken out of the Context
, the link will be made, and the result will be dropped without ever being propagated. So this is where the disconnect with the naming comes from - since it's completely up to the caller on whether it's propagated or not, so I find it pretty confusing. There's the other point where there's no such thing as a non-propagated span - all spans can be propagated, so why do we have the propagated span called out in particular? I'm sure someone is going to call Span.getPropagated(tracer.startSpan().getContext())
assuming that's how you're supposed to propagate your span ;) Span.getNoop(tracer.startSpan().getContext())
, Span.getNonRecording(...)
seems more obviously wrong because it better reflects what's actually happening.
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 don't love this name, but I 100% agree with the sentiment. Making it super clear what the capabilities (and lack thereof) will be of the returned Span seems like a very important signal to the user.
} | ||
|
||
private static <C> SpanContext extractImpl(C carrier, Getter<C> getter) { | ||
private static <C> Span extractImpl(C carrier, Getter<C> getter) { |
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.
This change seems a bit orthogonal to the PR. Does it need to be included?
@@ -123,18 +122,18 @@ public static AwsXRayPropagator getInstance() { | |||
Objects.requireNonNull(carrier, "carrier"); | |||
Objects.requireNonNull(getter, "getter"); | |||
|
|||
SpanContext spanContext = getSpanContextFromHeader(carrier, getter); | |||
if (!spanContext.isValid()) { | |||
Span span = extractSpan(carrier, getter); |
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'd like to separate out this changes into a follow-on PR, just to keep this one a little more tight in its scope.
} | ||
|
||
private static <C> SpanContext getSpanContextFromMultipleHeaders( | ||
private static <C> Span extractFromMultipleHeaders( |
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.
ditto on reducing scope for this PR
@jkwatson Good call - I also went ahead and named the method |
… into remove-default-span
9520587
to
59e676f
Compare
Had a scary unexpected force push but branch seems to be ok, phew |
return spanContext != null && !SpanContext.getInvalid().equals(spanContext) | ||
? new DefaultSpan(spanContext) | ||
: DefaultSpan.getInvalid(); | ||
return Span.getPropagated(spanContext); |
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.
this is a nice cleanup. much tighter and clearer.
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.
Thanks! This is a good improvement.
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 like the overall change of "PropagationSpan" but I don't like that we add other changes to the API like the "isValid" in the same PR. That change is independent and can be added in a separate PR.
* functionality. It will not be exported and all tracing operations are no-op, but it can be used | ||
* to propagate a valid {@link SpanContext} downstream. | ||
*/ | ||
static Span getPropagated(SpanContext spanContext) { |
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.
s/getPropagated/toPropagated?
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 should be called Span.wrap(SpanContext)
. Then we are also safe against renamings of propagated/noop/etc
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 don't think we are safe against behavior change, this needs to always propagate the SC. But +1 for the name
default boolean isValid() { | ||
return getContext().isValid(); | ||
} |
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 change is unrelated with the PR description and purpose, consider to not add "secondary" changes to a PR.
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.
Sure makes sense
… into remove-default-span
We use
DefaultSpan
, a no-op wrapper of a spancontext, for three use casesSo it actually didn't make sense to use the propagated span terminology to replace it. I used a
wrapAsNoop
factory instead since that's literally what we're doing.To give the propagated term some love, I renamed the relevantRealized I was confusing propagated span with remote span but they are different.SpanContext
factory to use the word.Would be much easier to reason about without the
SpanContext
I guess 😋Fixes #1704