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

* Specify uniqueness of tracestate keys as the sender's responsibility

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

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.

* remove redundant phrase about not adding your own key twice

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

* call out that foreign duplicated keys MAY be deleted

fixes #446

* fix test case for duplicated tracestate keys

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 #369

Author: basti1302

spec/20-http_request_header_format.md

@@ -293,19 +293,19 @@ chr      = %x20 / nblk-chr
 
 ### Combined Header Value
 
-The `tracestate` value is the concatenation of trace graph key/value pairs
+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.
-
-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:
 
 `congo=congosFirstPosition,rojo=rojosFirstPosition,congo=congosSecondPosition`
 
 Instead, the entry would be rewritten to only include the most recent position:
 `congo=congosSecondPosition,rojo=rojosFirstPosition`
 
+See [Mutating the tracestate Field](#mutating-the-tracestate-field) for details.
+
 #### tracestate Limits:
 
 Vendors SHOULD propagate at least 512 characters of a combined header. This length includes commas required to separate list items and optional white space (`OWS`) characters.
@@ -355,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.
 - **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.

test/test.py

@@ -549,37 +549,38 @@ def test_tracestate_multiple_headers_different_keys(self):
 	def test_tracestate_duplicated_keys(self):
 		'''
 		harness sends a request with an invalid tracestate header with duplicated keys
-		expects the tracestate to be discarded
+		expects the tracestate to be inherited, and the duplicated keys to be either kept as-is or one of them
+        to be discarded
 		'''
 		traceparent, tracestate = self.make_single_request_and_get_tracecontext([
 			['traceparent', '00-12345678901234567890123456789012-1234567890123456-00'],
 			['tracestate', 'foo=1,foo=1'],
 		])
 		self.assertEqual(traceparent.trace_id.hex(), '12345678901234567890123456789012')
-		self.assertRaises(KeyError, lambda: tracestate['foo'])
+		self.assertTrue('foo=1' in str(tracestate))
 
 		traceparent, tracestate = self.make_single_request_and_get_tracecontext([
 			['traceparent', '00-12345678901234567890123456789012-1234567890123456-00'],
 			['tracestate', 'foo=1,foo=2'],
 		])
 		self.assertEqual(traceparent.trace_id.hex(), '12345678901234567890123456789012')
-		self.assertRaises(KeyError, lambda: tracestate['foo'])
+		self.assertTrue('foo=1' in str(tracestate) or 'foo=2' in str(tracestate))
 
 		traceparent, tracestate = self.make_single_request_and_get_tracecontext([
 			['traceparent', '00-12345678901234567890123456789012-1234567890123456-00'],
 			['tracestate', 'foo=1'],
 			['tracestate', 'foo=1'],
 		])
 		self.assertEqual(traceparent.trace_id.hex(), '12345678901234567890123456789012')
-		self.assertRaises(KeyError, lambda: tracestate['foo'])
+		self.assertTrue('foo=1' in str(tracestate))
 
 		traceparent, tracestate = self.make_single_request_and_get_tracecontext([
 			['traceparent', '00-12345678901234567890123456789012-1234567890123456-00'],
 			['tracestate', 'foo=1'],
 			['tracestate', 'foo=2'],
 		])
 		self.assertEqual(traceparent.trace_id.hex(), '12345678901234567890123456789012')
-		self.assertRaises(KeyError, lambda: tracestate['foo'])
+		self.assertTrue('foo=1' in str(tracestate) or 'foo=2' in str(tracestate))
 
 	def test_tracestate_all_allowed_characters(self):
 		'''

test/tracecontext/tracestate.py

@@ -56,9 +56,11 @@ def from_string(self, string):
 				if not match:
 					raise ValueError('illegal key-value format {!r}'.format(member))
 				key, eq, value = match.groups()
-				if key in self._traits:
-					raise ValueError('conflict key {!r}'.format(key))
-				self._traits[key] = value
+				if key not in self._traits:
+					self._traits[key] = value
+					# 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.
 		return self
 
 	def to_string(self):