Specify uniqueness of tracestate keys as the sender's responsibility #477

basti1302 · 2021-11-11T16:23:38Z

This ~~partially~~ resolves #446.

This adds a new rule to "Mutating the tracestate Field" that makes it
clear that a sender must not create duplicated tracestate keys. It is a
non-breaking change because the section "Combined Header Value" already
had called that out with "Only one entry per key is allowed". Basically
this adds that rule to the section it actually belongs into.

Having that as the responsibility of the sender seems to be consensus in #446.

There seems to be no consensus yet whether an implementation should
clean up a tracestate value it receives by discarding duplicate keys
that it does not own. Thus, that aspect is not considered in this
change.

Update: Consensus has been reached that an implementation MAY discard duplicate key but is not obliged to. This aspect has been added to this change as well now.

Finally, this change fixes the test suite that was too strict and demanded that the whole tracestate should be dropped when duplicated keys are present. Thus, this PR fixes #369, too.

Preview | Diff

basti1302 · 2021-11-11T16:26:18Z

spec/20-http_request_header_format.md

@@ -297,7 +297,7 @@ The `tracestate` value is the concatenation of trace graph key/value pairs

 Example: `vendorname1=opaqueValue1,vendorname2=opaqueValue2`

-Only one entry per key is allowed because the entry represents that last position in the trace. Hence vendors must overwrite their entry upon reentry to their tracing system.


"because the entry represents that last position in the trace." had been called out here by @yurishkuro as not making a lot of sense, and I agree. Hence, I removed that part.

Also, I think update could be a better phrase instead of overwrite because we also use that in the section "Mutating the traceparent Field".

Finally "upon reentry to their tracing system" is really unwieldy, IMO.

basti1302 · 2021-11-11T16:26:54Z

spec/20-http_request_header_format.md

@@ -353,6 +355,6 @@ Vendors receiving a `tracestate` request header MUST send it to outgoing request

 Following are allowed mutations:

- **Add a new key/value pair**. The new key/value pair SHOULD be added to the beginning of the list.
+- **Add a new key/value pair**. The new key/value pair SHOULD be added to the beginning of the list. Adding a key/value pair MUST NOT result in the same key being present multiple times.


This had been called out in "Combined Header Value" already, but it actually belong here, IMO.

I like how this formulated. It eliminates concern that if platform doesn't deal with tracestate keys, just propagates it as is - it is not responsible for the duplicates. We may want to spell it out explicitly

See this part of my PR comment:

There seems to be no consensus yet whether an implementation should
clean up a tracestate value it receives by discarding duplicate keys
that it does not own. Thus, that aspect is not considered in this
change.

And my message here #446 (comment)

TL;DR: I agree with you that this is how it should work (vendors should not need to care about foreign keys at all) but I am not sure if this is consensus, so for now I was reluctant to add this aspect to this PR.

I agree that a tracing system should be responsible for deduping their own keys and shouldn't need to analyze or dedupe other keys.

I've added that aspect as well now: ccbabd6

yurishkuro · 2021-11-11T19:17:00Z

spec/20-http_request_header_format.md

@@ -297,7 +297,7 @@ The `tracestate` value is the concatenation of trace graph key/value pairs

 Example: `vendorname1=opaqueValue1,vendorname2=opaqueValue2`

-Only one entry per key is allowed because the entry represents that last position in the trace. Hence vendors must overwrite their entry upon reentry to their tracing system.
+Only one entry per key is allowed. Hence vendors MUST update their entry instead of adding it twice when propagating a modfied `tracestate` header downstream.


the word "hence" not needed

instead of speaking of updating/adding, I would rather say vendors MUST ensure that the entry with their vendor key is only present once in the header sent downstream.

+1 to just say that vendor is responsible for not duplicating keys it owns

Well, that is exactly what I have added down in the "Mutating the traceparent Field" section -> https://github.com/w3c/trace-context/pull/477/files#diff-3832602716774745d030f22592f4c5b7d8f316e4d44be0165282244f9d6f14d9R358

Do we just want to duplicate that phrasing here? Seems redundant. How about doing away with the second sentence entirely? So just "Only one entry per key is allowed." and that's it. We also have the cross-reference

See Mutating the tracestate Field for details.

so I think spelling out specifics here too is not required, the actual normative rules belong in the "Mutating" section and are already phrased as you requested.

WDYT?

Yes, perhaps just delete the entire phrase including "Only one entry per key is allowed."

I would keep the first sentence.

I kept the first sentence, it leads nicely into the following example, but I removed the duplication about not adding your own header twice.

kalyanaj · 2021-12-07T22:09:00Z

spec/20-http_request_header_format.md

 - **Update an existing value**. The value for any given key can be updated. Modified keys SHOULD be moved to the beginning (left) of the list.
- **Delete a key/value pair**. Any key/value pair MAY be deleted. Vendors SHOULD NOT delete keys that were not generated by them. The deletion of an unknown key/value pair will break correlation in other systems. This mutation enables two scenarios. The first is that proxies can block certain `tracestate` keys for privacy and security concerns. The second scenario is a truncation of long `tracestate`s.
+- **Delete a key/value pair**. Any key/value pair MAY be deleted. Vendors SHOULD NOT delete keys that were not generated by them. The deletion of an unknown key/value pair will break correlation in other systems. This mutation enables three scenarios. The first is that proxies can block certain `tracestate` keys for privacy and security concerns. The second scenario is a truncation of long `tracestate`s. Finally, vendors MAY also discard duplicate keys that were not generated by them, however, they are not obliged to do that.


Suggested change

- **Delete a key/value pair**. Any key/value pair MAY be deleted. Vendors SHOULD NOT delete keys that were not generated by them. The deletion of an unknown key/value pair will break correlation in other systems. This mutation enables three scenarios. The first is that proxies can block certain `tracestate` keys for privacy and security concerns. The second scenario is a truncation of long `tracestate`s. Finally, vendors MAY also discard duplicate keys that were not generated by them, however, they are not obliged to do that.

- **Delete a key/value pair**. Any key/value pair MAY be deleted. Vendors SHOULD NOT delete keys that were not generated by them. The deletion of an unknown key/value pair will break correlation in other systems. This mutation enables three scenarios. The first is that proxies can block certain `tracestate` keys for privacy and security concerns. The second scenario is a truncation of long `tracestate`s. The third scenario is vendors MAY discard duplicate keys that were not generated by them.

"The third scenario is vendors MAY discard" sounds a bit awkward IMO. I would prefer the current phrasing, to be honest.

The current phrasing of "Finally, vendors..." is fine with me. However, the phrase ", however, they are not obliged to do that" feels redundant since it is conveyed by the MAY semantics in the previous sentence, so it feels we can remove that part to make it tighter.

Good call, I have removed that part.

basti1302 · 2021-12-14T10:37:53Z

I have added another commit that fixes the broken test case that originally sparked this discussion in #369.

This partially resolves w3c#446. This adds a new rule to "Mutating the tracestate Field" that makes it clear that a sender must not create duplicated tracestate keys. It is a non-breaking change because the section "Combined Header Value" already had called that out with "Only one entry per key is allowed". Basically this adds that rule to the section it actually belongs into. Having that as the responsibility of the sender seems to be consensus in There seems to be no consensus yet whether an implementation should clean up a tracestate value it receives by discarding duplicate keys that it does not own. Thus, that aspect is not considered in this change.

This is already called out in the section "Mutating the traceparent Field".

fixes w3c#446

The test_tracestate_duplicated_keys was broken, it demanded to drop the whole tracestate when a duplicate key was found. This does not match what the specification mandates in that situation. Instead, implementations MAY remove the duplicates from tracestate or even leave them in as-is. fixes w3c#369

basti1302 · 2021-12-14T10:52:19Z

@dyladan @yurishkuro @SergeyKanzhelev I think all comments have been addressed by now and the PR also includes a fix for the test case related to duplicate keys in the tracestate header. Could you please re-review?

yurishkuro · 2021-12-14T14:16:13Z

spec/20-http_request_header_format.md

-Only one entry per key is allowed because the entry represents that last position in the trace. Hence vendors must overwrite their entry upon reentry to their tracing system.
-
-For example, if a vendor name is Congo and a trace started in their system and then went through a system named Rojo and later returned to Congo, the `tracestate` value would not be:
+Only one entry per key is allowed. For example, if a vendor name is Congo and a trace started in their system and then went through a system named Rojo and later returned to Congo, the `tracestate` value would not be:


"is allowed" is not a normative language. I would change it to "There SHOULD be only one entry per key".

The normative language around this is in the section "Mutating the tracestate Field":

Adding a key/value pair MUST NOT result in the same key being present multiple times.

I would restrict the normative description of that aspect to the "Mutating" section because it describes what an implementation is allowed to do with the header and what not. I do not see any added benefit by duplicating this aspect in normative language here. "Only one entry per key is allowed." is given in the context of the example (see the line above) and in my understanding, examples are never normative.

basti1302 commented Nov 11, 2021

View reviewed changes

basti1302 mentioned this pull request Nov 11, 2021

Clarify tracestate keys MUST be unique #446

Closed

yurishkuro reviewed Nov 11, 2021

View reviewed changes

kalyanaj linked an issue Dec 7, 2021 that may be closed by this pull request

Clarify handling of duplicate keys in tracestate #369

Closed

kalyanaj reviewed Dec 7, 2021

View reviewed changes

basti1302 force-pushed the uniqueness-of-tracestate-keys branch from ccbabd6 to 02efeb6 Compare December 10, 2021 16:30

kalyanaj approved these changes Dec 10, 2021

View reviewed changes

basti1302 force-pushed the uniqueness-of-tracestate-keys branch from f1234ad to e3f16d7 Compare December 14, 2021 10:46

basti1302 added 4 commits December 14, 2021 11:47

remove redundant phrase about not adding your own key twice

9329e9a

This is already called out in the section "Mutating the traceparent Field".

call out that foreign duplicated keys MAY be deleted

b798e72

fixes w3c#446

basti1302 force-pushed the uniqueness-of-tracestate-keys branch from e3f16d7 to 3590d0a Compare December 14, 2021 10:47

yurishkuro approved these changes Dec 14, 2021

View reviewed changes

bogdandrutu approved these changes Jan 4, 2022

View reviewed changes

dyladan approved these changes Jan 4, 2022

View reviewed changes

kalyanaj merged commit 0c3135d into w3c:main Jan 4, 2022

basti1302 deleted the uniqueness-of-tracestate-keys branch January 4, 2022 19:48

This was referenced Oct 16, 2023

Update tracestate duplicate keys in test_data.json #547

Closed

trace: ParseTraceState ignores duplicated tracestate keys open-telemetry/opentelemetry-go#4638

Closed

W3C Trace propagator tests - switch to level 1 open-telemetry/opentelemetry-dotnet#4937

Merged

emdneto mentioned this pull request Aug 7, 2024

build(scripts-tracecontext-integration-test.sh): fixed issue #4105 open-telemetry/opentelemetry-python#4109

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify uniqueness of tracestate keys as the sender's responsibility #477

Specify uniqueness of tracestate keys as the sender's responsibility #477

basti1302 commented Nov 11, 2021 •

edited by pr-preview bot

Loading

basti1302 Nov 11, 2021

basti1302 Nov 11, 2021

SergeyKanzhelev Nov 12, 2021

basti1302 Nov 12, 2021

dyladan Nov 16, 2021

basti1302 Nov 16, 2021

yurishkuro Nov 11, 2021

SergeyKanzhelev Nov 12, 2021

basti1302 Nov 12, 2021

SergeyKanzhelev Nov 12, 2021

dyladan Nov 16, 2021

basti1302 Nov 16, 2021

kalyanaj Dec 7, 2021

basti1302 Dec 8, 2021

kalyanaj Dec 8, 2021

basti1302 Dec 10, 2021

basti1302 commented Dec 14, 2021

basti1302 commented Dec 14, 2021

yurishkuro Dec 14, 2021

basti1302 Dec 14, 2021

		@@ -297,7 +297,7 @@ The `tracestate` value is the concatenation of trace graph key/value pairs

		Example: `vendorname1=opaqueValue1,vendorname2=opaqueValue2`

		Only one entry per key is allowed because the entry represents that last position in the trace. Hence vendors must overwrite their entry upon reentry to their tracing system.

Specify uniqueness of tracestate keys as the sender's responsibility #477

Specify uniqueness of tracestate keys as the sender's responsibility #477

Conversation

basti1302 commented Nov 11, 2021 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basti1302 commented Dec 14, 2021

basti1302 commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basti1302 commented Nov 11, 2021 •

edited by pr-preview bot

Loading