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

Remove Included Propagators section from trace API spec #1556

Merged
merged 6 commits into from
Mar 22, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 17, 2021

This is a duplicate of the Propagators Distribution of the context specification.

Fixes #1544

This is a duplicate of the Propagators Distribution of the context
specification.
@MrAlias MrAlias requested a review from a team as a code owner March 17, 2021 17:16
@MrAlias MrAlias requested a review from a team March 17, 2021 17:16
@MrAlias MrAlias requested a review from a team as a code owner March 17, 2021 17:16
Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

LGTM.

@carlosalberto
Copy link
Contributor

Not quite a duplicate, as the Propagators Distribution is a general note, while this one is about the API being the place where TraceContext lives. That being said, we may need to have better wording for it, in order to avoid confusion.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 18, 2021

Not quite a duplicate, as the Propagators Distribution is a general note, while this one is about the API being the place where TraceContext lives. That being said, we may need to have better wording for it, in order to avoid confusion.

@carlosalberto I'm not sure I follow. From this section of the specification:

The API layer or an extension package MUST include the following Propagators

My understanding reading that is that the API or an extension package is the place TraceContext lives. Additionally this is restated in the Propagators Distribution section:

The official list of propagators that MUST be maintained by the OpenTelemetry organization and MUST be distributed as OpenTelemetry extension packages [include] W3C TraceContext [which] MAY alternatively be distributed as part of the OpenTelemetry API.

This excerpt from the Propagators Distribution section contains normative requirements and options and doesn't seem to be a note.

@carlosalberto
Copy link
Contributor

@MrAlias You are definitely correct - the original intention of that paragraph in api.md (before the amendment in the Propagators Distribution doc), is that we really suggest SIGs to have the TraceContext (and Baggage) propagators right in the base API packages, even if distributing them as extension packages is allowed.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 19, 2021

@carlosalberto I'm still not 100% sure what your suggestion for a change here is?

You would definitely be the one to know what the original intention was there and I don't mean to step on toes. I'm just going off what the final result ended up being and what was released. Based on how both section of the specification are written today they appear to me to be duplicates and removing the less comprehensive of the two seems like a good way to remove redundancy and avoid future confusion and internal divergences.

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

@MrAlias We just need a small note here regarding that we suggest (not even a SHOULD) that SIGs include this TracePropagator in the API package/artifact itself. Will do that myself to not stale this PR any longer. Thanks!

@carlosalberto carlosalberto merged commit 0cc8d4c into open-telemetry:main Mar 22, 2021
@MrAlias MrAlias deleted the fix-1544 branch March 23, 2021 15:07
ThomsonTan pushed a commit to ThomsonTan/opentelemetry-specification that referenced this pull request Mar 30, 2021
…ry#1556)

* Remove Included Propagators section from trace API spec

This is a duplicate of the Propagators Distribution of the context
specification.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is the Included Propagators section a duplicate of Propagators Distribution?
9 participants