Adding w3c tracecontext integration test (#228)

Verifying that our tracecontext is compliant with the w3c
tracecontext reference is valuable. Adding a tox command
to verify that the TraceContext propagator adheres to the
w3c spec.

The tracecontexthttptextformat is now completely compliant with the w3c
tracecontext test suite.

Author: toumorokoshi

.flake8

@@ -10,6 +10,7 @@ exclude =
   .svn
   .tox
   CVS
+  target
   __pycache__
   ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/
   ext/opentelemetry-ext-jaeger/build/*

.gitignore

@@ -53,3 +53,4 @@ _build/
 
 # mypy
 .mypy_cache/
+target

.isort.cfg

@@ -12,4 +12,5 @@ line_length=79
 ; )
 ; docs: https://github.com/timothycrosley/isort#multi-line-output-modes
 multi_line_output=3
+skip=target
 skip_glob=ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/*

ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py

@@ -210,7 +210,7 @@ def test_request_attributes_with_partial_raw_uri(self):
         self.validate_url("http://127.0.0.1/#top")
 
     def test_request_attributes_with_partial_raw_uri_and_nonstandard_port(
-        self
+        self,
     ):
         self.environ["RAW_URI"] = "/?"
         del self.environ["HTTP_HOST"]

opentelemetry-api/src/opentelemetry/context/propagation/tracecontexthttptextformat.py

@@ -39,11 +39,13 @@
 )
 
 _DELIMITER_FORMAT = "[ \t]*,[ \t]*"
-_MEMBER_FORMAT = "({})(=)({})".format(_KEY_FORMAT, _VALUE_FORMAT)
+_MEMBER_FORMAT = "({})(=)({})[ \t]*".format(_KEY_FORMAT, _VALUE_FORMAT)
 
 _DELIMITER_FORMAT_RE = re.compile(_DELIMITER_FORMAT)
 _MEMBER_FORMAT_RE = re.compile(_MEMBER_FORMAT)
 
+_TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS = 32
+
 
 class TraceContextHTTPTextFormat(httptextformat.HTTPTextFormat):
     """Extracts and injects using w3c TraceContext's headers.
@@ -86,15 +88,10 @@ def extract(
         if version == "ff":
             return trace.INVALID_SPAN_CONTEXT
 
-        tracestate = trace.TraceState()
-        for tracestate_header in get_from_carrier(
+        tracestate_headers = get_from_carrier(
             carrier, cls._TRACESTATE_HEADER_NAME
-        ):
-            # typing.Dict's update is not recognized by pylint:
-            # https://github.com/PyCQA/pylint/issues/2420
-            tracestate.update(  # pylint:disable=E1101
-                _parse_tracestate(tracestate_header)
-            )
+        )
+        tracestate = _parse_tracestate(tracestate_headers)
 
         span_context = trace.SpanContext(
             trace_id=int(trace_id, 16),
@@ -127,25 +124,44 @@ def inject(
             )
 
 
-def _parse_tracestate(string: str) -> trace.TraceState:
-    """Parse a w3c tracestate header into a TraceState.
+def _parse_tracestate(header_list: typing.List[str]) -> trace.TraceState:
+    """Parse one or more w3c tracestate header into a TraceState.
 
     Args:
         string: the value of the tracestate header.
 
     Returns:
         A valid TraceState that contains values extracted from
         the tracestate header.
+
+        If the format of one headers is illegal, all values will
+        be discarded and an empty tracestate will be returned.
+
+        If the number of keys is beyond the maximum, all values
+        will be discarded and an empty tracestate will be returned.
     """
     tracestate = trace.TraceState()
-    for member in re.split(_DELIMITER_FORMAT_RE, string):
-        match = _MEMBER_FORMAT_RE.match(member)
-        if not match:
-            raise ValueError("illegal key-value format %r" % (member))
-        key, _eq, value = match.groups()
-        # typing.Dict's update is not recognized by pylint:
-        # https://github.com/PyCQA/pylint/issues/2420
-        tracestate[key] = value  # pylint:disable=E1137
+    value_count = 0
+    for header in header_list:
+        for member in re.split(_DELIMITER_FORMAT_RE, header):
+            # empty members are valid, but no need to process further.
+            if not member:
+                continue
+            match = _MEMBER_FORMAT_RE.fullmatch(member)
+            if not match:
+                # TODO: log this?
+                return trace.TraceState()
+            key, _eq, value = match.groups()
+            if key in tracestate:  # pylint:disable=E1135
+                # duplicate keys are not legal in
+                # the header, so we will remove
+                return trace.TraceState()
+            # typing.Dict's update is not recognized by pylint:
+            # https://github.com/PyCQA/pylint/issues/2420
+            tracestate[key] = value  # pylint:disable=E1137
+            value_count += 1
+            if value_count > _TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS:
+                return trace.TraceState()
     return tracestate
 
 

opentelemetry-api/src/opentelemetry/distributedcontext/propagation/binaryformat.py

@@ -44,7 +44,7 @@ def to_bytes(context: DistributedContext) -> bytes:
     @staticmethod
     @abc.abstractmethod
     def from_bytes(
-        byte_representation: bytes
+        byte_representation: bytes,
     ) -> typing.Optional[DistributedContext]:
         """Return a DistributedContext that was represented by bytes.
 

opentelemetry-api/src/opentelemetry/propagators/__init__.py

@@ -78,7 +78,7 @@ def get_global_httptextformat() -> httptextformat.HTTPTextFormat:
 
 
 def set_global_httptextformat(
-    http_text_format: httptextformat.HTTPTextFormat
+    http_text_format: httptextformat.HTTPTextFormat,
 ) -> None:
     global _HTTP_TEXT_FORMAT  # pylint:disable=global-statement
     _HTTP_TEXT_FORMAT = http_text_format

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -338,10 +338,11 @@ def __init__(
         self.trace_state = trace_state
 
     def __repr__(self) -> str:
-        return "{}(trace_id={}, span_id={})".format(
+        return "{}(trace_id={}, span_id={}, trace_state={!r})".format(
             type(self).__name__,
             format_trace_id(self.trace_id),
             format_span_id(self.span_id),
+            self.trace_state,
         )
 
     def is_valid(self) -> bool:
@@ -589,7 +590,7 @@ def tracer() -> Tracer:
 
 
 def set_preferred_tracer_implementation(
-    factory: ImplementationFactory
+    factory: ImplementationFactory,
 ) -> None:
     """Set the factory to be used to create the tracer.
 

opentelemetry-api/src/opentelemetry/util/loader.py

@@ -173,7 +173,7 @@ def _load_impl(
 
 
 def set_preferred_default_implementation(
-    implementation_factory: _UntrustedImplFactory[_T]
+    implementation_factory: _UntrustedImplFactory[_T],
 ) -> None:
     """Sets a factory function that may be called for any implementation
     object. See the :ref:`module docs <loader-factory>` for more details."""

opentelemetry-api/tests/context/propagation/test_tracecontexthttptextformat.py

@@ -22,10 +22,10 @@
 
 
 def get_as_list(
-    dict_object: typing.Dict[str, str], key: str
+    dict_object: typing.Dict[str, typing.List[str]], key: str
 ) -> typing.List[str]:
     value = dict_object.get(key)
-    return [value] if value is not None else []
+    return value if value is not None else []
 
 
 class TestTraceContextFormat(unittest.TestCase):
@@ -40,64 +40,10 @@ def test_no_traceparent_header(self):
 
         If no traceparent header is received, the vendor creates a new trace-id and parent-id that represents the current request.
         """
-        output = {}  # type:typing.Dict[str, str]
+        output = {}  # type:typing.Dict[str, typing.List[str]]
         span_context = FORMAT.extract(get_as_list, output)
         self.assertTrue(isinstance(span_context, trace.SpanContext))
 
-    def test_from_headers_tracestate_entry_limit(self):
-        """If more than 33 entries are passed, allow them.
-
-        We are explicitly choosing not to limit the list members
-        as outlined in RFC 3.3.1.1
-
-        RFC 3.3.1.1
-
-        There can be a maximum of 32 list-members in a list.
-        """
-
-        span_context = FORMAT.extract(
-            get_as_list,
-            {
-                "traceparent": "00-12345678901234567890123456789012-1234567890123456-00",
-                "tracestate": ",".join(
-                    [
-                        "a00=0,a01=1,a02=2,a03=3,a04=4,a05=5,a06=6,a07=7,a08=8,a09=9",
-                        "b00=0,b01=1,b02=2,b03=3,b04=4,b05=5,b06=6,b07=7,b08=8,b09=9",
-                        "c00=0,c01=1,c02=2,c03=3,c04=4,c05=5,c06=6,c07=7,c08=8,c09=9",
-                        "d00=0,d01=1,d02=2",
-                    ]
-                ),
-            },
-        )
-        self.assertEqual(len(span_context.trace_state), 33)
-
-    def test_from_headers_tracestate_duplicated_keys(self):
-        """If a duplicate tracestate header is present, the most recent entry
-        is used.
-
-        RFC 3.3.1.4
-
-        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:
-
-        congo=congosFirstPosition,rojo=rojosFirstPosition,congo=congosSecondPosition
-
-        Instead, the entry would be rewritten to only include the most recent position:
-
-        congo=congosSecondPosition,rojo=rojosFirstPosition
-        """
-        span_context = FORMAT.extract(
-            get_as_list,
-            {
-                "traceparent": "00-12345678901234567890123456789012-1234567890123456-00",
-                "tracestate": "foo=1,bar=2,foo=3",
-            },
-        )
-        self.assertEqual(span_context.trace_state, {"foo": "3", "bar": "2"})
-
     def test_headers_with_tracestate(self):
         """When there is a traceparent and tracestate header, data from
         both should be addded to the SpanContext.
@@ -109,7 +55,10 @@ def test_headers_with_tracestate(self):
         tracestate_value = "foo=1,bar=2,baz=3"
         span_context = FORMAT.extract(
             get_as_list,
-            {"traceparent": traceparent_value, "tracestate": tracestate_value},
+            {
+                "traceparent": [traceparent_value],
+                "tracestate": [tracestate_value],
+            },
         )
         self.assertEqual(span_context.trace_id, self.TRACE_ID)
         self.assertEqual(span_context.span_id, self.SPAN_ID)
@@ -125,7 +74,8 @@ def test_headers_with_tracestate(self):
         self.assertEqual(output["tracestate"].count(","), 2)
 
     def test_invalid_trace_id(self):
-        """If the trace id is invalid, we must ignore the full traceparent header.
+        """If the trace id is invalid, we must ignore the full traceparent header,
+        and return a random, valid trace.
 
         Also ignore any tracestate.
 
@@ -142,8 +92,10 @@ def test_invalid_trace_id(self):
         span_context = FORMAT.extract(
             get_as_list,
             {
-                "traceparent": "00-00000000000000000000000000000000-1234567890123456-00",
-                "tracestate": "foo=1,bar=2,foo=3",
+                "traceparent": [
+                    "00-00000000000000000000000000000000-1234567890123456-00"
+                ],
+                "tracestate": ["foo=1,bar=2,foo=3"],
             },
         )
         self.assertEqual(span_context, trace.INVALID_SPAN_CONTEXT)
@@ -166,8 +118,10 @@ def test_invalid_parent_id(self):
         span_context = FORMAT.extract(
             get_as_list,
             {
-                "traceparent": "00-00000000000000000000000000000000-0000000000000000-00",
-                "tracestate": "foo=1,bar=2,foo=3",
+                "traceparent": [
+                    "00-00000000000000000000000000000000-0000000000000000-00"
+                ],
+                "tracestate": ["foo=1,bar=2,foo=3"],
             },
         )
         self.assertEqual(span_context, trace.INVALID_SPAN_CONTEXT)
@@ -195,14 +149,15 @@ def test_format_not_supported(self):
 
         RFC 4.3
 
-        If the version cannot be parsed, the vendor creates a new traceparent header and
-        deletes tracestate.
+        If the version cannot be parsed, return an invalid trace header.
         """
         span_context = FORMAT.extract(
             get_as_list,
             {
-                "traceparent": "00-12345678901234567890123456789012-1234567890123456-00-residue",
-                "tracestate": "foo=1,bar=2,foo=3",
+                "traceparent": [
+                    "00-12345678901234567890123456789012-1234567890123456-00-residue"
+                ],
+                "tracestate": ["foo=1,bar=2,foo=3"],
             },
         )
         self.assertEqual(span_context, trace.INVALID_SPAN_CONTEXT)
@@ -213,3 +168,30 @@ def test_propagate_invalid_context(self):
         output = {}  # type:typing.Dict[str, str]
         FORMAT.inject(trace.INVALID_SPAN_CONTEXT, dict.__setitem__, output)
         self.assertFalse("traceparent" in output)
+
+    def test_tracestate_empty_header(self):
+        """Test tracestate with an additional empty header (should be ignored)"""
+        span_context = FORMAT.extract(
+            get_as_list,
+            {
+                "traceparent": [
+                    "00-12345678901234567890123456789012-1234567890123456-00"
+                ],
+                "tracestate": ["foo=1", ""],
+            },
+        )
+        self.assertEqual(span_context.trace_state["foo"], "1")
+
+    def test_tracestate_header_with_trailing_comma(self):
+        """Do not propagate invalid trace context.
+        """
+        span_context = FORMAT.extract(
+            get_as_list,
+            {
+                "traceparent": [
+                    "00-12345678901234567890123456789012-1234567890123456-00"
+                ],
+                "tracestate": ["foo=1,"],
+            },
+        )
+        self.assertEqual(span_context.trace_state["foo"], "1")

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -301,7 +301,6 @@ def set_status(self, status: trace_api.Status) -> None:
 
 def generate_span_id() -> int:
     """Get a new random span ID.
-
     Returns:
         A random 64-bit int for use as a span ID
     """
@@ -310,7 +309,6 @@ def generate_span_id() -> int:
 
 def generate_trace_id() -> int:
     """Get a new random trace ID.
-
     Returns:
         A random 128-bit int for use as a trace ID
     """

opentelemetry-sdk/tests/context/propagation/test_b3_format.py

@@ -16,7 +16,7 @@
 
 import opentelemetry.sdk.context.propagation.b3_format as b3_format
 import opentelemetry.sdk.trace as trace
-import opentelemetry.trace as api_trace
+import opentelemetry.trace as trace_api
 
 FORMAT = b3_format.B3Format()
 
@@ -163,8 +163,8 @@ def test_invalid_single_header(self):
         """
         carrier = {FORMAT.SINGLE_HEADER_KEY: "0-1-2-3-4-5-6-7"}
         span_context = FORMAT.extract(get_as_list, carrier)
-        self.assertEqual(span_context.trace_id, api_trace.INVALID_TRACE_ID)
-        self.assertEqual(span_context.span_id, api_trace.INVALID_SPAN_ID)
+        self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID)
+        self.assertEqual(span_context.span_id, trace_api.INVALID_SPAN_ID)
 
     def test_missing_trace_id(self):
         """If a trace id is missing, populate an invalid trace id."""
@@ -173,7 +173,7 @@ def test_missing_trace_id(self):
             FORMAT.FLAGS_KEY: "1",
         }
         span_context = FORMAT.extract(get_as_list, carrier)
-        self.assertEqual(span_context.trace_id, api_trace.INVALID_TRACE_ID)
+        self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID)
 
     def test_missing_span_id(self):
         """If a trace id is missing, populate an invalid trace id."""
@@ -182,4 +182,4 @@ def test_missing_span_id(self):
             FORMAT.FLAGS_KEY: "1",
         }
         span_context = FORMAT.extract(get_as_list, carrier)
-        self.assertEqual(span_context.span_id, api_trace.INVALID_SPAN_ID)
+        self.assertEqual(span_context.span_id, trace_api.INVALID_SPAN_ID)