Default Sampler update + ParentOrElse fix (#1013)

* Updated default sampler to match the spec. Fixed broken ParentOrElseSampler.

* Fixed http-in instrumentation creating Activity objects with invalid parents.

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

Author: CodeBlanch

src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs

@@ -64,23 +64,25 @@ public override void OnStartActivity(Activity activity, object payload)
                 // This requires to ignore the current activity and create a new one
                 // using the context extracted using the format TextFormat supports.
                 var ctx = this.options.TextFormat.Extract(default, request, HttpRequestHeaderValuesGetter);
-
-                // Create a new activity with its parent set from the extracted context.
-                // This makes the new activity as a "sibling" of the activity created by
-                // Asp.Net.
-                Activity newOne = new Activity(ActivityNameByHttpInListener);
-                newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
-                newOne.TraceStateString = ctx.TraceState;
-
-                // Starting the new activity make it the Activity.Current one.
-                newOne.Start();
-
-                // Both new activity and old one store the other activity
-                // inside them. This is required in the Stop step to
-                // correctly stop and restore Activity.Current.
-                newOne.SetCustomProperty("ActivityByAspNet", activity);
-                activity.SetCustomProperty("ActivityByHttpInListener", newOne);
-                activity = newOne;
+                if (ctx != default)
+                {
+                    // Create a new activity with its parent set from the extracted context.
+                    // This makes the new activity as a "sibling" of the activity created by
+                    // Asp.Net.
+                    Activity newOne = new Activity(ActivityNameByHttpInListener);
+                    newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
+                    newOne.TraceStateString = ctx.TraceState;
+
+                    // Starting the new activity make it the Activity.Current one.
+                    newOne.Start();
+
+                    // Both new activity and old one store the other activity
+                    // inside them. This is required in the Stop step to
+                    // correctly stop and restore Activity.Current.
+                    newOne.SetCustomProperty("ActivityByAspNet", activity);
+                    activity.SetCustomProperty("ActivityByHttpInListener", newOne);
+                    activity = newOne;
+                }
             }
 
             // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -72,17 +72,19 @@ public override void OnStartActivity(Activity activity, object payload)
                 // using the format TextFormat supports.
 
                 var ctx = this.options.TextFormat.Extract(default, request, HttpRequestHeaderValuesGetter);
-
-                // Create a new activity with its parent set from the extracted context.
-                // This makes the new activity as a "sibling" of the activity created by
-                // Asp.Net Core.
-                Activity newOne = new Activity(ActivityNameByHttpInListener);
-                newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
-                newOne.TraceStateString = ctx.TraceState;
-
-                // Starting the new activity make it the Activity.Current one.
-                newOne.Start();
-                activity = newOne;
+                if (ctx != default)
+                {
+                    // Create a new activity with its parent set from the extracted context.
+                    // This makes the new activity as a "sibling" of the activity created by
+                    // Asp.Net Core.
+                    Activity newOne = new Activity(ActivityNameByHttpInListener);
+                    newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
+                    newOne.TraceStateString = ctx.TraceState;
+
+                    // Starting the new activity make it the Activity.Current one.
+                    newOne.Start();
+                    activity = newOne;
+                }
             }
 
             activity.SetKind(ActivityKind.Server);

src/OpenTelemetry/CHANGELOG.md

@@ -12,6 +12,8 @@
   disposes the containing exporter.
 * `BroadcastActivityProcessor`is disposable and it disposes the processors.
 * Samplers now get the actual TraceId of the Activity to be created.
+* Default Sampler changed from AlwaysOn to ParentOrElse(AlwaysOn) to match the
+  spec.
 
 ## 0.3.0-beta
 

src/OpenTelemetry/Sdk.cs

@@ -64,7 +64,7 @@ public static MeterProvider CreateMeterProvider(Action<MeterBuilder> configure)
                 meterRegistry,
                 metricProcessor,
                 metricExporter,
-                meterBuilder.MetricPushInterval == default(TimeSpan) ? DefaultPushInterval : meterBuilder.MetricPushInterval,
+                meterBuilder.MetricPushInterval == default ? DefaultPushInterval : meterBuilder.MetricPushInterval,
                 cancellationTokenSource);
 
             var meterProviderSdk = new MeterProviderSdk(metricProcessor, meterRegistry, controller, cancellationTokenSource);
@@ -85,7 +85,7 @@ public static TracerProvider CreateTracerProvider(Action<TracerProviderBuilder>
             configureTracerProviderBuilder?.Invoke(tracerProviderBuilder);
 
             var tracerProviderSdk = new TracerProviderSdk();
-            Sampler sampler = tracerProviderBuilder.Sampler ?? new AlwaysOnSampler();
+            Sampler sampler = tracerProviderBuilder.Sampler ?? new ParentOrElseSampler(new AlwaysOnSampler());
 
             ActivityProcessor activityProcessor;
             if (tracerProviderBuilder.ProcessingPipelines == null || !tracerProviderBuilder.ProcessingPipelines.Any())
@@ -165,7 +165,8 @@ internal static ActivityDataRequest ComputeActivityDataRequest(
             in ActivityCreationOptions<ActivityContext> options,
             Sampler sampler)
         {
-            var isRootSpan = options.Parent.SpanId == default;
+            var isRootSpan = /*TODO: Put back once AutoGenerateRootContextTraceId is removed.
+                              options.Parent.TraceId == default ||*/ options.Parent.SpanId == default;
 
             // As we set ActivityListener.AutoGenerateRootContextTraceId = true,
             // Parent.TraceId will always be the TraceId of the to-be-created Activity,

src/OpenTelemetry/Trace/ActivitySourceAdapter.cs

@@ -76,23 +76,20 @@ private void RunGetRequestedData(Activity activity)
             ActivityContext parentContext;
             if (string.IsNullOrEmpty(activity.ParentId))
             {
-                parentContext = default(ActivityContext);
+                parentContext = default;
+            }
+            else if (activity.Parent != null)
+            {
+                parentContext = activity.Parent.Context;
             }
             else
             {
-                if (activity.Parent != null)
-                {
-                    parentContext = activity.Parent.Context;
-                }
-                else
-                {
-                    parentContext = new ActivityContext(
-                        activity.TraceId,
-                        activity.ParentSpanId,
-                        activity.ActivityTraceFlags,
-                        activity.TraceStateString,
-                        isRemote: true);
-                }
+                parentContext = new ActivityContext(
+                    activity.TraceId,
+                    activity.ParentSpanId,
+                    activity.ActivityTraceFlags,
+                    activity.TraceStateString,
+                    isRemote: true);
             }
 
             var samplingParameters = new SamplingParameters(

src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs

@@ -41,7 +41,8 @@ public ParentOrElseSampler(Sampler delegateSampler)
         public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
         {
             var parentContext = samplingParameters.ParentContext;
-            if (parentContext == default)
+            if (/* TODO: TraceId is always provided due to AutoGenerateRootContextTraceId. That is being removed in RC1 and this can be put back.
+                 parentContext.TraceId == default ||*/ parentContext.SpanId == default)
             {
                 // If no parent, use the delegate to determine sampling.
                 return this.delegateSampler.ShouldSample(samplingParameters);

test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs

@@ -21,6 +21,7 @@
 using Moq;
 using OpenTelemetry.Instrumentation.Grpc.Tests.Services;
 using OpenTelemetry.Trace;
+using OpenTelemetry.Trace.Samplers;
 using Xunit;
 
 namespace OpenTelemetry.Instrumentation.Grpc.Tests
@@ -44,6 +45,7 @@ public void GrpcClientCallsAreCollectedSuccessfully(string baseAddress)
 
             using (Sdk.CreateTracerProvider(
                 (builder) => builder
+                    .SetSampler(new AlwaysOnSampler())
                     .AddGrpcClientInstrumentation()
                     .SetResource(expectedResource)
                     .AddProcessorPipeline(p => p.AddProcessor(n => spanProcessor.Object))))
@@ -95,6 +97,7 @@ public void GrpcAndHttpClientInstrumentationIsInvoked()
 
             using (Sdk.CreateTracerProvider(
             (builder) => builder
+                .SetSampler(new AlwaysOnSampler())
                 .AddHttpClientInstrumentation()
                 .AddGrpcClientInstrumentation()
                 .AddProcessorPipeline(p => p.AddProcessor(n => spanProcessor.Object))))