Issue 6037 - Wrapping "invalid" SpanContexts in Span does not preserve SpanContext #6044
Conversation
| @@ -81,9 +81,6 @@ static Span wrap(SpanContext spanContext) { | |||
| ApiUsageLogger.log("context is null"); | |||
| return getInvalid(); | |||
| } | |||
| if (!spanContext.isValid()) { | |||
There was a problem hiding this comment.
This seems to make sense to me, but the javadoc above somewhat implies that this behavior:
It will not be exported and all tracing operations are no-op, but it can be used to propagate a valid {@link SpanContext} downstream.
Can we remove the word "valid"?
Also, you could argue this is breaking behavior change, but I don't think so. If you provide an invalid span context (i.e. invalid span id / trace id) but useful TraceFlags / TraceState, you'll still get a Span with an invalid span context back. It would be odd for someone to be relying on the behavior of Span.wrap(SpanContext clearing effectively clearing TraceFlags / TraceState when span id and trace id are invalid.
There was a problem hiding this comment.
Yes, I fully agree. Furthermore, I think that using valid or invalid with respect to the SpanContext is a bit misleading. The context represents an empty trace (i.e. with no spans yet), and every trace starts from such a context.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6044 +/- ##
============================================
- Coverage 91.18% 91.17% -0.01%
+ Complexity 5623 5622 -1
============================================
Files 618 618
Lines 16580 16578 -2
Branches 1642 1641 -1
============================================
- Hits 15118 15115 -3
Misses 1013 1013
- Partials 449 450 +1 ☔ View full report in Codecov by Sentry. |
See Issue 6037 for context.