Document handling of tracestate duplicate keys

[pellared]

I think that the following is not clearly defined in https://github.com/w3c/trace-context/blob/master/spec/20-http_request_header_format.md:

• trace-context/test/test.py

  Lines 563 to 568 in 551a5b0

  	def test_tracestate_duplicated_keys(self):
  	'''
  	harness sends a request with an invalid tracestate header with duplicated keys
  	expects the tracestate to be inherited, and the duplicated keys to be either kept as-is or one of them
  	to be discarded
  	'''

• trace-context/test/tracecontext/tracestate.py

  Lines 61 to 63 in 551a5b0

  	# If key is already in self._traits, the incoming tracestate header contained a duplicated key.
  	# According to the spec, two behaviors are valid: Either pass on the duplicated key as-is or drop
  	# it. We opt for dropping it.

See also #547

[kalyanaj]

Thanks Bastian - assigning to you per our discussion in the Working Group meeting.

[basti1302]

Hey Robert,

the intention of the wording in the spec is that there is a difference between receiving a tracestate header and modifying it for sending it downstream.

That is, if you add a key to tracestate, you MUST make sure that this key only exists once in the list, no matter if it occurred 0 time, once or multiple times before you modified tracestate. This is what https://w3c.github.io/trace-context/#mutating-the-tracestate-field is about, in particular the sentence "Adding a key/value pair MUST NOT result in the same key being present multiple times."

However, if you receive the tracestate header, you are not responsible for validating the uniqueness of tracestate list members, nor are you responsible for removing duplicated headers. But: If you want to, you can. (exact wording: "Finally, vendors MAY also discard duplicate keys that were not generated by them.") Reasoning: Some implementations might not want to spend the computational overhead of even reading/parsing the full tracestate header, and the spec should not force them to do that. OTOH, there are limits for the length of the tracestate keys, and sometimes, implementations may need to drop key value pairs. Allowing to drop key value pairs that are obviously invalid anyway (because of duplicated keys) seems reasonable.

I am not sure if this background is helpful? In my opinion, the two pieces of test code you linked to agree with that interpretation. Do you see an issue with these tests? Or was your comment around the aspect that the spec should mandate one specific behavior instead of allowing two behaviors?

By the way, you referenced a fork of the spec (coincidentlly, my fork :-) ) at https://github.com/instana/trace-context/blob/master/spec/20-http_request_header_format.md. Forks might be outdated/unmaintained. We recommend to always reference this repository. Use the main branch for the latest version (which might contain unpublished edits that will never make it into a published spec) or the level-2 etc. branches for the respective level of the spec.

[pellared]

First of all, thanks a lot for your response. I really apricate the amount of effort you put into explaining.

The reason I am asking is that I try to double-check if the current behavior (returns an error when there is a duplicate key) of https://pkg.go.dev/go.opentelemetry.io/otel/trace#ParseTraceState is correct. Reference: open-telemetry/opentelemetry-go#4638

However, if you receive the tracestate header, you are not responsible for validating the uniqueness of tracestate list members, nor are you responsible for removing duplicated headers.

How does it play with the following from https://w3c.github.io/trace-context/#combined-header-value:

Only one entry per key is allowed.

This fragment feels like duplicates are not really allowed.

[basti1302]

I think this is still in line with what I said earlier about the different responsibilities and perspectives when receiving the header and modifying it yourself. The intent of that paragraph is to explain that participants modifying tracestate are not supposed to add "their own" key multiple times. But I agree that the phrasing is a bit misleading.

How about we change

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:

to

Tracing tools are not supposed to add the same header multiple times. 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:

Do you think that would clarify this point?

[pellared]

Do you think that would clarify this point?

Yes 👍

[basti1302]

How about we change

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:

to

Tracing tools are not supposed to add the same header multiple times. 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:

See #552.