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.

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

Make clear the propagated/non-recording span concept needs no type. #1131

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

Oberon00
Copy link
Member

Fixes #1087, closes #1124, closes #1125

Changes

Makes clear that the non-recording span / propagated Span has no semantic meaning and should not be a new public type.

@Oberon00 Oberon00 requested review from a team October 22, 2020 14:06
@Oberon00 Oberon00 added area:api Cross language API specification issue spec:trace Related to the specification/trace directory labels Oct 22, 2020
specification/trace/api.md Show resolved Hide resolved
@Oberon00 Oberon00 requested a review from tsloughter October 22, 2020 15:24
@tsloughter
Copy link
Member

tsloughter commented Oct 22, 2020

I just don't get why any of this is needed. I think I'd be fine with not using a separate context key if none of this was added to the API spec and the spec of the extractor simply said a Span should be created from propagated trace context with is_recording as false.

Edit: From #1124 it appears why I don't get why this is needed is I was missing the need for making this possible without an implementation available

@Oberon00
Copy link
Member Author

Also, from a performance perspective, a non-recording span can be implemented more efficiently and lightweight than a full span.

@Oberon00
Copy link
Member Author

And there is also no other API to create a non-recording span (except if you muck around with samplers maybe).

@carlosalberto carlosalberto self-requested a review October 22, 2020 17:18
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much easier for me to understand, thanks

specification/trace/api.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

@open-telemetry/specs-approvers Please review. The more eyes/approvals, the better ;)

@bogdandrutu
Copy link
Member

@yurishkuro @SergeyKanzhelev ?

@Oberon00 Oberon00 changed the title Make clear the propagated /non-recording span concept needs no type. Make clear the propagated/non-recording span concept needs no type. Oct 24, 2020
@carlosalberto
Copy link
Contributor

Ping @SergeyKanzhelev @yurishkuro

@bogdandrutu bogdandrutu merged commit 06d16d7 into open-telemetry:master Oct 27, 2020
@arminru arminru deleted the nonrecording branch November 2, 2020 16:21
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…pen-telemetry#1131)

* Make clear the propagated /non-recording span concept needs no type.

* Update specification/trace/api.md

Co-authored-by: Armin Ruech <[email protected]>

Co-authored-by: Armin Ruech <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:trace Related to the specification/trace directory
Projects
None yet
9 participants