Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow SpanContext to be exposed directly on Span. #1022

Closed
wants to merge 1 commit into from
Closed

Allow SpanContext to be exposed directly on Span. #1022

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

@Oberon00 Oberon00 Sep 29, 2020 •
edited
Loading

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.

Copy link
Contributor Author

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?

Copy link
Member

@Oberon00 Oberon00 Sep 29, 2020 •
edited
Loading

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.


### Retrieving the TraceId and SpanId

The API MUST allow retrieving the `TraceId` and `SpanId` in the following forms:
Expand Down