Fix parent based sampling in tracer (#354)

Currently the sdk `Tracer` implementation will filter out invalid /
dropped parent span contexts. This causes the parent based sampler to
not function properly as it will then assume that child spans are roots.

This patch resolves this issue by adding a `has_active_span` method to
the trace context extension, which allows the tracer to distinguish
between invalid and unset parent contexts.

Author: jtescher

opentelemetry/src/api/trace/context.rs

@@ -42,6 +42,11 @@ pub trait TraceContextExt {
     /// ```
     fn span(&self) -> &dyn crate::trace::Span;
 
+    /// Used to see if a span has been marked as active
+    ///
+    /// This is useful for building tracers.
+    fn has_active_span(&self) -> bool;
+
     /// Returns a copy of this context with the span context included.
     ///
     /// This is useful for building propagators.
@@ -71,6 +76,10 @@ impl TraceContextExt for Context {
         }
     }
 
+    fn has_active_span(&self) -> bool {
+        self.get::<Span>().is_some()
+    }
+
     fn with_remote_span_context(&self, span_context: crate::trace::SpanContext) -> Self {
         self.with_value(RemoteSpanContext(span_context))
     }

opentelemetry/src/api/trace/noop.rs

@@ -150,12 +150,13 @@ impl trace::Tracer for NoopTracer {
             .parent_context
             .take()
             .or_else(|| {
-                Some(cx.span().span_context())
-                    .filter(|sc| sc.is_valid())
-                    .cloned()
+                if cx.has_active_span() {
+                    Some(cx.span().span_context().clone())
+                } else {
+                    None
+                }
             })
-            .or_else(|| cx.remote_span_context().filter(|sc| sc.is_valid()).cloned())
-            .filter(|cx| cx.is_valid());
+            .or_else(|| cx.remote_span_context().cloned());
         if let Some(span_context) = parent_span_context {
             trace::NoopSpan { span_context }
         } else {

opentelemetry/src/api/trace/span_context.rs

@@ -164,6 +164,7 @@ impl TraceState {
     /// assert!(trace_state.is_ok());
     /// assert_eq!(trace_state.unwrap().header(), String::from("foo=bar,apple=banana"))
     /// ```
+    #[allow(clippy::all)]
     pub fn from_key_value<T, K, V>(trace_state: T) -> Result<Self, ()>
     where
         T: IntoIterator<Item = (K, V)>,
@@ -209,6 +210,7 @@ impl TraceState {
     /// updated key/value is returned.
     ///
     /// ['spec']: https://www.w3.org/TR/trace-context/#list
+    #[allow(clippy::all)]
     pub fn insert(&self, key: String, value: String) -> Result<TraceState, ()> {
         if !TraceState::valid_key(key.as_str()) || !TraceState::valid_value(value.as_str()) {
             return Err(());
@@ -227,6 +229,7 @@ impl TraceState {
     /// with the removed entry is returned.
     ///
     /// ['spec']: https://www.w3.org/TR/trace-context/#list
+    #[allow(clippy::all)]
     pub fn delete(&self, key: String) -> Result<TraceState, ()> {
         if !TraceState::valid_key(key.as_str()) {
             return Err(());

opentelemetry/src/sdk/metrics/aggregators/array.rs

@@ -205,7 +205,7 @@ impl Quantile for PointsData {
             return Err(MetricsError::NoDataCollected);
         }
 
-        if q < 0.0 || q > 1.0 {
+        if !(0.0..=1.0).contains(&q) {
             return Err(MetricsError::InvalidQuantile);
         }
 

opentelemetry/src/sdk/metrics/aggregators/ddsketch.rs

@@ -116,7 +116,7 @@ impl Distribution for DDSKetchAggregator {}
 
 impl Quantile for DDSKetchAggregator {
     fn quantile(&self, q: f64) -> Result<Number> {
-        if q < 0.0 || q > 1.0 {
+        if !(0.0..=1.0).contains(&q) {
             return Err(MetricsError::InvalidQuantile);
         }
         self.inner.read().map_err(From::from).and_then(|inner| {

opentelemetry/src/sdk/trace/tracer.rs

@@ -172,9 +172,14 @@ impl crate::trace::Tracer for Tracer {
         let parent_span_context = builder
             .parent_context
             .as_ref()
-            .or_else(|| Some(cx.span().span_context()).filter(|cx| cx.is_valid()))
-            .or_else(|| cx.remote_span_context())
-            .filter(|cx| cx.is_valid());
+            .or_else(|| {
+                if cx.has_active_span() {
+                    Some(cx.span().span_context())
+                } else {
+                    None
+                }
+            })
+            .or_else(|| cx.remote_span_context());
         // Build context for sampling decision
         let (no_parent, trace_id, parent_span_id, remote_parent, parent_trace_flags) =
             parent_span_context
@@ -283,11 +288,12 @@ mod tests {
     use crate::{
         sdk::{
             self,
-            trace::{Config, SamplingDecision, SamplingResult, ShouldSample},
+            trace::{Config, Sampler, SamplingDecision, SamplingResult, ShouldSample},
         },
+        testing::trace::TestSpan,
         trace::{
-            Link, Span, SpanBuilder, SpanContext, SpanId, SpanKind, TraceId, TraceState, Tracer,
-            TracerProvider, TRACE_FLAG_SAMPLED,
+            Link, Span, SpanBuilder, SpanContext, SpanId, SpanKind, TraceContextExt, TraceId,
+            TraceState, Tracer, TracerProvider, TRACE_FLAG_SAMPLED,
         },
         Context, KeyValue,
     };
@@ -340,4 +346,19 @@ mod tests {
         let expected = span_context.trace_state();
         assert_eq!(expected.get("foo"), Some("notbar"))
     }
+
+    #[test]
+    fn drop_parent_based_children() {
+        let sampler = Sampler::ParentBased(Box::new(Sampler::AlwaysOn));
+        let config = Config::default().with_default_sampler(sampler);
+        let tracer_provider = sdk::trace::TracerProvider::builder()
+            .with_config(config)
+            .build();
+
+        let context = Context::current_with_span(TestSpan(SpanContext::empty_context()));
+        let tracer = tracer_provider.get_tracer("test", None);
+        let span = tracer.start_from_context("must_not_be_sampled", &context);
+
+        assert!(!span.span_context().is_sampled());
+    }
 }