-
Notifications
You must be signed in to change notification settings - Fork 840
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
Fix opencensus shim spanBuilderWithRemoteParent behavior #6415
Conversation
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.
The change LGTM.
I tested this with a grpc client & server and verified that the final client span and first server span are correctly linked.
SpanConverter.mapSpanContext(ocRemoteParentSpanContext, /* isRemoteParent= */ true); | ||
otelSpanBuilder.setParent( | ||
Context.current().with(io.opentelemetry.api.trace.Span.wrap(spanContext))); | ||
otelSpanBuilder.addLink(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.
Why adding link also?
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.
Wasn't sure if anyone is dependent on the existing behavior.
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.
Do you think we'd remove the addLink
before marking the opencensus shim stable?
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'm not sure. I'd want the spec opencensus compatibility doc to say what to do here before marking stable. That doc is currently lax on details which we see in the opentracing compatibility doc, so I don't see this happening any time soon.
Thanks @gavin-nia, so we can tag this PR as Fixes #6416? |
Yes, I believe so. |
Fixes #5475.
The behavior of
spanBuilderWithRemoteParent
seems wrong right now. It adds a span link instead but does not set the span represented by the context argument to be the parent. Not sure why this is, but I can't find anything in the opencensus compatibility spec which states that this is intended, so I'm going to assume its just a bug in the implementation.