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

Add description of TraceState operations #905

Merged
merged 7 commits into from
Sep 3, 2020

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Sep 1, 2020

Fixes #759

Changes

Add description of TraceState operations.

specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
Co-authored-by: Christian Neumüller <[email protected]>
specification/trace/sdk.md Outdated Show resolved Hide resolved
@iNikem iNikem marked this pull request as ready for review September 1, 2020 12:17
@iNikem iNikem requested review from a team September 1, 2020 12:18
Oberon00
Oberon00 previously approved these changes Sep 1, 2020
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
@Oberon00 Oberon00 dismissed their stale review September 1, 2020 16:13

Points of W3C-validation need to be decided, #905 (comment)

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

This PR is LGTM overall. Marking as "Request changes" to finish feedback from @Oberon00 and @reyang:

  • clarify implementeablity (are operations implemented in SDK or API).
  • add a wording on strict operations.

specification/trace/sdk.md Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

Note that as @anuraaga suggested in #875 (comment) an alternative design would be to replace TraceState with a Map<Key<T>, T>. This could have multiple advantages:

@iNikem
Copy link
Contributor Author

iNikem commented Sep 2, 2020

It may be so, indeed.

To avoid confusion: I still think this PR is useful for clarifying current state of the API and should be merged. I doubt we decide on removing TraceState completely very soon :)

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

The alternative approach would not be removing TraceState, it would just move it from a first-class member of SpanContext to being implemented with that more generic Map<Key<T>, T>. But that should also be doable later as a non-breaking change (given a careful implementation of TraceState).

@SergeyKanzhelev
Copy link
Member

I think this PR makes things better. Switching to map may be a separate discussion. The worst things about the map is that it is looses the order which is critical for the specification. This is why I wouldn't suggest to jump into this discussion in this PR

@SergeyKanzhelev
Copy link
Member

@iNikem I will let this PR sit for some time for cool off period for new comments. Otherwise it looks good and will be merged if not objections received by tomorrow PT

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

The worst things about the map is that it is looses the order

There are ordered maps, but my suggestion would anyway be to have the current W3C tracestate as a single unparsed string under a single key in that map.

This is why I wouldn't suggest to jump into this discussion in this PR

Right, this discussion is already going on at #875 (comment)

@Oberon00 Oberon00 added area:api Cross language API specification issue area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory labels Sep 2, 2020
@carlosalberto carlosalberto self-requested a review September 2, 2020 18:45
@iNikem
Copy link
Contributor Author

iNikem commented Sep 3, 2020

@SergeyKanzhelev can we merge? :)

@SergeyKanzhelev SergeyKanzhelev merged commit 293a508 into open-telemetry:master Sep 3, 2020
@iNikem iNikem deleted the tracecontext branch September 3, 2020 19:10
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
* Add description of TraceState operations

* Apply suggestions from code review

Co-authored-by: Christian Neumüller <[email protected]>

* Apply suggestions from code review

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

* Add validation requirement

* Fix error handling

Co-authored-by: Christian Neumüller <[email protected]>
Co-authored-by: Armin Ruech <[email protected]>
Co-authored-by: Sergey Kanzhelev <[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 area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

W3C TraceState underspecified
6 participants