-
Notifications
You must be signed in to change notification settings - Fork 896
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
Allow SpanContext to be exposed directly on Span. #1022
Conversation
Prototype in Java open-telemetry/opentelemetry-java#1712 - we find the API to be far simpler |
Have you checked the occurrences of SpanContext in the spec for what this means? E.g. what do we pass to the Sampler then? A full (Propated)Span? (Would be easier after #881). |
Yup |
Right now, a span is meant to be write only at the API level. The only readable part is the span context. One drawback to inlining the span context into the span is that we'll lose that separation. I'm not saying we shouldn't do it, but this could introduce some confusion about which, and why some span properties are readable, and others are not. |
I endorse this proposal, having made similar statements in the past: open-telemetry/oteps#73. As for @mwear's statements about read vs. write conceptual APIs, this has also been discussed in open-telemetry/oteps#68. If it were not for time and energy constraints, I'd suggest going further than this PR. The approach would be:
It's too complex--this "going further" proposal can be one for the future. |
Thanks @jmacd, I agree with that approach, and that it's probably going too far for now. At recommendation from the maintainers meeting, I have also filed #1033 which is an alternative to this PR that removes |
@@ -195,6 +195,8 @@ The API MUST implement methods to create a `SpanContext`. These methods SHOULD b | |||
create a `SpanContext`. This functionality MUST be fully implemented in the API, and SHOULD NOT be | |||
overridable. | |||
|
|||
The API MAY expose the functionality of `SpanContext` directly on the `Span` without a separate type. |
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 commented on the issue, the case of the SpanContext as input for SDK sampler needs special care. Using the full span as input changes the meaning, making the two options not equivalent functionality-wise. There may be other functionality changes.
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.
Does https://github.com/open-telemetry/opentelemetry-specification/pull/881/files alleviate the issue since it won't be 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.
I think it does (#1022 (comment)) EDIT: Though it is not required, you can also just specify this more clearly here in this PR.
Although I requested changes, I'm in favor of this PR generally. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Going to go ahead and close this since don't think it will be merged. |
Fixes #999
This is a relaxing of the spec to give SDKs more options in presenting the API for
SpanContext
. See attached issue for why they may decide to do this. It's post-freeze, but the hope is that since this doesn't provide any additional requirement it could still be considered.