Propagator return original context if failed to extract (#139)

Author: aabmass

opentelemetry-propagator-gcp/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+- Fix propagator modifying context if failed to extract
+  ([#139](https://github.com/GoogleCloudPlatform/opentelemetry-operations-python/pull/139))
+
 ## Version 1.0.0rc0
 
 Released 2021-04-22

opentelemetry-propagator-gcp/src/opentelemetry/propagators/cloud_trace_propagator/__init__.py

@@ -56,21 +56,24 @@ def extract(
         context: typing.Optional[Context] = None,
         getter: textmap.Getter = textmap.default_getter,
     ) -> Context:
+        if context is None:
+            context = Context()
+
         header = self._get_header_value(getter, carrier)
 
         if not header:
-            return trace.set_span_in_context(trace.INVALID_SPAN, context)
+            return context
 
         match = re.fullmatch(_TRACE_CONTEXT_HEADER_RE, header)
         if match is None:
-            return trace.set_span_in_context(trace.INVALID_SPAN, context)
+            return context
 
         trace_id = match.group("trace_id")
         span_id = match.group("span_id")
         trace_options = match.group("trace_flags")
 
         if trace_id == "0" * 32 or int(span_id) == 0:
-            return trace.set_span_in_context(trace.INVALID_SPAN, context)
+            return context
 
         span_context = SpanContext(
             trace_id=int(trace_id, 16),

opentelemetry-propagator-gcp/tests/test_cloud_trace_propagator.py

@@ -16,6 +16,7 @@
 
 import opentelemetry.trace as trace
 from opentelemetry.context import get_current
+from opentelemetry.context.context import Context
 from opentelemetry.propagators.cloud_trace_propagator import (
     _TRACE_CONTEXT_HEADER_NAME,
     CloudTraceFormatPropagator,
@@ -43,7 +44,13 @@ def _extract(self, header_value):
         new_context = self.propagator.extract(
             carrier=header, getter=default_getter
         )
-        return trace.get_current_span(new_context).get_span_context()
+        return new_context
+
+    def _extract_span_context(self, header_value):
+        """Test helper"""
+        return trace.get_current_span(
+            self._extract(header_value)
+        ).get_span_context()
 
     def _inject(self, span=None):
         """Test helper"""
@@ -54,27 +61,30 @@ def _inject(self, span=None):
         self.propagator.inject(output, context=ctx)
         return output.get(_TRACE_CONTEXT_HEADER_NAME)
 
-    def test_no_context_header(self):
-        header = {}
-        new_context = self.propagator.extract(
-            carrier=header, getter=default_getter
-        )
+    def _assert_failed_to_extract(self, new_context: Context):
+        self.assertEqual(new_context, Context())
         self.assertEqual(
             trace.get_current_span(new_context).get_span_context(),
             trace.INVALID_SPAN.get_span_context(),
         )
 
+    def test_no_context_header(self):
+        headers = {}
+        new_context = self.propagator.extract(
+            carrier=headers, getter=default_getter
+        )
+        self._assert_failed_to_extract(new_context)
+
     def test_empty_context_header(self):
         header = ""
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        new_context = self._extract(header)
+        self._assert_failed_to_extract(new_context)
 
     def test_valid_header(self):
         header = "{}/{};o=1".format(
             format_trace_id(self.valid_trace_id), self.valid_span_id
         )
-        new_span_context = self._extract(header)
+        new_span_context = self._extract_span_context(header)
         self.assertEqual(new_span_context.trace_id, self.valid_trace_id)
         self.assertEqual(new_span_context.span_id, self.valid_span_id)
         self.assertEqual(new_span_context.trace_flags, TraceFlags(1))
@@ -83,7 +93,7 @@ def test_valid_header(self):
         header = "{}/{};o=10".format(
             format_trace_id(self.valid_trace_id), self.valid_span_id
         )
-        new_span_context = self._extract(header)
+        new_span_context = self._extract_span_context(header)
         self.assertEqual(new_span_context.trace_id, self.valid_trace_id)
         self.assertEqual(new_span_context.span_id, self.valid_span_id)
         self.assertEqual(new_span_context.trace_flags, TraceFlags(10))
@@ -92,14 +102,14 @@ def test_valid_header(self):
         header = "{}/{};o=0".format(
             format_trace_id(self.valid_trace_id), self.valid_span_id
         )
-        new_span_context = self._extract(header)
+        new_span_context = self._extract_span_context(header)
         self.assertEqual(new_span_context.trace_id, self.valid_trace_id)
         self.assertEqual(new_span_context.span_id, self.valid_span_id)
         self.assertEqual(new_span_context.trace_flags, TraceFlags(0))
         self.assertTrue(new_span_context.is_remote)
 
         header = "{}/{};o=0".format(format_trace_id(self.valid_trace_id), 345)
-        new_span_context = self._extract(header)
+        new_span_context = self._extract_span_context(header)
         self.assertEqual(new_span_context.trace_id, self.valid_trace_id)
         self.assertEqual(new_span_context.span_id, 345)
         self.assertEqual(new_span_context.trace_flags, TraceFlags(0))
@@ -129,106 +139,73 @@ def test_mixed_case_header_key(self):
 
     def test_invalid_header_format(self):
         header = "invalid_header"
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{};o=".format(
             format_trace_id(self.valid_trace_id), self.valid_span_id
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "extra_chars/{}/{};o=1".format(
             format_trace_id(self.valid_trace_id), self.valid_span_id
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{}extra_chars;o=1".format(
             format_trace_id(self.valid_trace_id), self.valid_span_id
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{};o=1extra_chars".format(
             format_trace_id(self.valid_trace_id), self.valid_span_id
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/;o=1".format(format_trace_id(self.valid_trace_id))
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "/{};o=1".format(self.valid_span_id)
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{};o={}".format("123", "34", "4")
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
     def test_invalid_trace_id(self):
         header = "{}/{};o={}".format(INVALID_TRACE_ID, self.valid_span_id, 1)
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
+
         header = "{}/{};o={}".format("0" * 32, self.valid_span_id, 1)
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "0/{};o={}".format(self.valid_span_id, 1)
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "234/{};o={}".format(self.valid_span_id, 1)
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{};o={}".format(self.too_long_id, self.valid_span_id, 1)
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
     def test_invalid_span_id(self):
         header = "{}/{};o={}".format(
             format_trace_id(self.valid_trace_id), INVALID_SPAN_ID, 1
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{};o={}".format(
             format_trace_id(self.valid_trace_id), "0" * 16, 1
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{};o={}".format(
             format_trace_id(self.valid_trace_id), "0", 1
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
         header = "{}/{};o={}".format(
             format_trace_id(self.valid_trace_id), self.too_long_id, 1
         )
-        self.assertEqual(
-            self._extract(header), trace.INVALID_SPAN.get_span_context()
-        )
+        self._assert_failed_to_extract(self._extract(header))
 
     def test_inject_with_no_context(self):
         output = self._inject()