-
Notifications
You must be signed in to change notification settings - Fork 48
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
Assumes that INVALID parent span id is actually null in TraceContext #720
Conversation
- without this change we're returning an INVALID parent span even though we define in the javadocs of TraceContext that we would return null - with this change we're verifying whether the INVALID parent is set in which case we're returning null fixes gh-687
class OtelTraceContextTests { | ||
|
||
SdkTracerProvider sdkTracerProvider = SdkTracerProvider.builder() | ||
.setSampler(io.opentelemetry.sdk.trace.samplers.Sampler.alwaysOn()) |
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.
Since the result in the original issue was dependent on the sampling decision, should we double the tests and test sampled and not sampled scenarios as well?
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.
You're right. The problem is that with sampler e.g. always off we can't get the parent id from OTel even though we should.
@Test
void should_return_parentid_when_spans_not_sampled() {
SdkTracerProvider sdkTracerProvider = SdkTracerProvider.builder()
.setSampler(io.opentelemetry.sdk.trace.samplers.Sampler.alwaysOff())
.build();
OpenTelemetrySdkBuilder openTelemetrySdkBuilder = OpenTelemetrySdk.builder().setTracerProvider(sdkTracerProvider);
try (OpenTelemetrySdk openTelemetrySdk = openTelemetrySdkBuilder.build()) {
Tracer otelTracer = tracer(openTelemetrySdk);
Span parentSpan = otelTracer.spanBuilder("parent").startSpan();
Span span = otelTracer.spanBuilder("foo")
.setParent(parentSpan.storeInContext(Context.current()))
.startSpan();
OtelTraceContext otelTraceContext = new OtelTraceContext(span);
then(otelTraceContext.parentId()).isEqualTo(parentSpan.getSpanContext().getSpanId());
}
}
This will not pass. I need to figure out how to solve this
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.
we can't get the parent id from OTel even though we should
According to OTel we should never be able to get the parentId "instrumentation time", only in the processors/exporters by looking at the span tree. I think in that case it might be ok to return and assert null
as we did earlier since that's the best I think we can do.
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 would work differently for Brave and OTel then I guess?
…720) * Assumes that INVALID parent span id is actually null in TraceContext - without this change we're returning an INVALID parent span even though we define in the javadocs of TraceContext that we would return null - with this change we're verifying whether the INVALID parent is set in which case we're returning null note: OTel doesn't store parent id for non sampled spans in which case we will return null fixes gh-687
fixes gh-687