Fix ParentBased sampler for implicit parent spans (open-telemetry#1394)

mariojonke · web-flow · commit 7b33dd69080e · 2020-11-25T09:32:14.000-08:00
consider the sample decision of implicit parent spans (when creating
a span without explicitly providing a context) instead of forwarding
to the delegating sampler.

* fix trace_state erasure for dropped spans

* samplers did not return the trace_state in the sampling result
  when a span was dropped. This caused the trace_state extracted
  from a remote parent span to be erased and further context
  propagation to break.
* fix also TraceIdRatioBased which erased the trace_state independent
  of the sampling outcome.
diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md
@@ -14,8 +14,9 @@
   ([#1373](https://github.com/open-telemetry/opentelemetry-python/pull/1373))
 - Rename Meter class to Accumulator in Metrics SDK
   ([#1372](https://github.com/open-telemetry/opentelemetry-python/pull/1372))
-- Rename Meter class to Accumulator in Metrics SDK
-  ([#1372](https://github.com/open-telemetry/opentelemetry-python/pull/1372))
+- Fix `ParentBased` sampler for implicit parent spans. Fix also `trace_state` 
+  erasure for dropped spans or spans sampled by the `TraceIdRatioBased` sampler. 
+  ([#1394](https://github.com/open-telemetry/opentelemetry-python/pull/1394))
 
 ## Version 0.15b0
 
diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py
@@ -151,7 +151,7 @@ def should_sample(
         trace_state: "TraceState" = None,
     ) -> "SamplingResult":
         if self._decision is Decision.DROP:
-            return SamplingResult(self._decision)
+            attributes = None
         return SamplingResult(self._decision, attributes, trace_state)
 
     def get_description(self) -> str:
@@ -209,8 +209,8 @@ def should_sample(
         if trace_id & self.TRACE_ID_LIMIT < self.bound:
             decision = Decision.RECORD_AND_SAMPLE
         if decision is Decision.DROP:
-            return SamplingResult(decision)
-        return SamplingResult(decision, attributes)
+            attributes = None
+        return SamplingResult(decision, attributes, trace_state)
 
     def get_description(self) -> str:
         return "TraceIdRatioBased{{{}}}".format(self._rate)
@@ -238,18 +238,16 @@ def should_sample(
         links: Sequence["Link"] = None,
         trace_state: "TraceState" = None,
     ) -> "SamplingResult":
-        if parent_context is not None:
-            parent_span_context = get_current_span(
-                parent_context
-            ).get_span_context()
-            # only drop if parent exists and is not a root span
-            if (
-                parent_span_context is not None
-                and parent_span_context.is_valid
-                and not parent_span_context.trace_flags.sampled
-            ):
-                return SamplingResult(Decision.DROP)
-            return SamplingResult(Decision.RECORD_AND_SAMPLE, attributes)
+        parent_span_context = get_current_span(
+            parent_context
+        ).get_span_context()
+        # respect the sampling flag of the parent if present
+        if parent_span_context is not None and parent_span_context.is_valid:
+            decision = Decision.RECORD_AND_SAMPLE
+            if not parent_span_context.trace_flags.sampled:
+                decision = Decision.DROP
+                attributes = None
+            return SamplingResult(decision, attributes, trace_state)
 
         return self._delegate.should_sample(
             parent_context=parent_context,
diff --git a/opentelemetry-sdk/tests/trace/test_sampling.py b/opentelemetry-sdk/tests/trace/test_sampling.py
