tracecontexthttptestformat now works with integration test

toumorokoshi · toumorokoshi · commit 791baaf0466b · 2019-10-20T20:46:09.000-07:00
The tracecontexthttptextformat now adheres completely to the w3c
tracecontext test suite.

moving the test endpoint to a non-root, to
ensure that the basic example is clear.

Adding unit tests to test_tracecontexhttptextformat
that were helpful.
diff --git a/examples/trace/server.py b/examples/trace/server.py
@@ -45,8 +45,17 @@
 app.wsgi_app = OpenTelemetryMiddleware(app.wsgi_app)
 
 
-@app.route("/", methods=["POST"])
+@app.route("/")
 def index():
+    """An example which starts a span within the span created for
+    the request."""
+    with trace.tracer().start_span("parent"):
+        requests.get("https://www.wikipedia.org/wiki/Rabbit")
+    return "hello"
+
+
+@app.route("/verify-tracecontext", methods=["POST"])
+def verify_tracecontext():
     """Upon reception of some payload, sends a request back to the designated url.
 
     This route is designed to be testable with the w3c tracecontext server / client test.
@@ -64,15 +73,8 @@ def index():
     return "hello"
 
 
-@app.route("/child_span_example")
-def child_span_example():
-    """An example which starts a span within the span created for
-    the request."""
-    with trace.tracer().start_span("parent"):
-        requests.get("https://www.wikipedia.org/wiki/Rabbit")
-    return "hello"
-
-
 if __name__ == "__main__":
-    app.run(debug=True)
-    span_processor.shutdown()
+    try:
+        app.run(debug=True)
+    finally:
+        span_processor.shutdown()
diff --git a/opentelemetry-api/src/opentelemetry/context/propagation/tracecontexthttptextformat.py b/opentelemetry-api/src/opentelemetry/context/propagation/tracecontexthttptextformat.py
@@ -35,7 +35,7 @@
 
 _KEY_FORMAT = _KEY_WITHOUT_VENDOR_FORMAT + "|" + _KEY_WITH_VENDOR_FORMAT
 _VALUE_FORMAT = (
-    r"[\x20-\x2b\x2d-\x3c\x3e-\x7e]{0,255}[\x21-\x2b\x2d-\x3c\x3e-\x7e]?"
+    r"[\x20-\x2b\x2d-\x3c\x3e-\x7e]{1,255}[\x21-\x2b\x2d-\x3c\x3e-\x7e]?"
 )
 
 _DELIMITER_FORMAT = "[ \t]*,[ \t]*"
@@ -88,17 +88,10 @@ def extract(
         if version == "ff":
             return trace.generate_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)
-            )
-        if len(tracestate) > _TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS:
-            tracestate = trace.TraceState()
+        )
+        tracestate = _parse_tracestate(tracestate_headers)
 
         span_context = trace.SpanContext(
             trace_id=int(trace_id, 16),
@@ -131,8 +124,8 @@ 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.
@@ -141,19 +134,32 @@ def _parse_tracestate(string: str) -> trace.TraceState:
         A valid TraceState that contains values extracted from
         the tracestate header.
 
-        If the format of the TraceState is illegal, all values will
+        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.fullmatch(member)
-        if not match:
-            # TODO: log this?
-            return trace.TraceState()
-        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
+    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:
+                # 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
+    if len(tracestate) > _TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS:
+        tracestate = trace.TraceState()
     return tracestate
 
 
diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py
@@ -360,10 +360,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={})".format(
             type(self).__name__,
             format_trace_id(self.trace_id),
             format_span_id(self.span_id),
+            repr(self.trace_state),
         )
 
     def is_valid(self) -> bool:
diff --git a/opentelemetry-api/tests/context/propagation/test_tracecontexthttptextformat.py b/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):
@@ -44,33 +44,6 @@ def test_no_traceparent_header(self):
         span_context = FORMAT.extract(get_as_list, output)
         self.assertTrue(isinstance(span_context, trace.SpanContext))
 
-    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.
@@ -82,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)
@@ -98,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.
 
@@ -115,11 +92,19 @@ 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)
+        self.assertNotEqual(
+            span_context.span_id, trace.INVALID_SPAN_CONTEXT.span_id
+        )
+        self.assertNotEqual(
+            span_context.trace_id, trace.INVALID_SPAN_CONTEXT.trace_id
+        )
+        self.assertNotEqual(span_context.span_id, "1234567890123456")
 
     def test_invalid_parent_id(self):
         """If the parent id is invalid, we must ignore the full traceparent header.
@@ -139,11 +124,14 @@ 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)
+        self.assertNotEqual(span_context, trace.INVALID_SPAN_CONTEXT)
+        self.assertEqual(span_context.trace_state, trace.TraceState())
 
     def test_no_send_empty_tracestate(self):
         """If the tracestate is empty, do not set the header.
@@ -174,15 +162,46 @@ def test_format_not_supported(self):
         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)
+        self.assertNotEqual(span_context, trace.INVALID_SPAN_CONTEXT)
+        self.assertNotEqual(span_context.trace_id, self.TRACE_ID)
 
     def test_propagate_invalid_context(self):
         """Do not propagate invalid trace context.
         """
         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):
+        """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")
+
+    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")
diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
@@ -1,5 +1,4 @@
 # Copyright 2019, OpenTelemetry Authors
-#
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
diff --git a/scripts/tracecontext-integration-test.sh b/scripts/tracecontext-integration-test.sh
@@ -1,5 +1,5 @@
 #!/bin/bash
-set -e 
+# set -e 
 # hard-coding the git tag to ensure stable builds.
 TRACECONTEXT_GIT_TAG="98f210efd89c63593dce90e2bae0a1bdcb986f51"
 # clone w3c tracecontext tests
@@ -9,15 +9,18 @@ git clone https://github.com/w3c/trace-context ./target/trace-context
 cd ./target/trace-context && git checkout $TRACECONTEXT_GIT_TAG && cd -
 # start example opentelemetry service, which propagates trace-context by 
 # default.
-python ./examples/trace/server.py &
+python ./examples/trace/server.py 1>&2 &
 EXAMPLE_SERVER_PID=$!
 # give the app server a little time to start up. Not adding some sort
 # of delay would cause many of the tracecontext tests to fail being
 # unable to connect.
 sleep 1
 function on-shutdown {
-    kill $EXAMPLE_SERVER_PID
+    # send a sigint, to ensure
+    # it is caught as a KeyboardInterrupt in the
+    # example service.
+    kill -2 $EXAMPLE_SERVER_PID
 }
 trap on-shutdown EXIT
 cd ./target/trace-context/test
-python test.py http://127.0.0.1:5000/
+python test.py http://127.0.0.1:5000/verify-tracecontext

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`# Copyright 2019, OpenTelemetry Authors`
`2`		`-#`
`3`	`2`	`# Licensed under the Apache License, Version 2.0 (the "License");`
`4`	`3`	`# you may not use this file except in compliance with the License.`
`5`	`4`	`# You may obtain a copy of the License at`