From 9e9c2779a80fdd4e922afa74c903bd4e45af733c Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 5 Aug 2020 11:21:27 -0700 Subject: [PATCH 1/2] Updated default sampler to match the spec. Fixed broken ParentOrElseSampler. --- src/OpenTelemetry/CHANGELOG.md | 2 ++ src/OpenTelemetry/Sdk.cs | 7 ++++--- src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs | 3 ++- .../GrpcTests.client.cs | 3 +++ 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index d780fc4d852..27d929dc5fb 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -11,6 +11,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 diff --git a/src/OpenTelemetry/Sdk.cs b/src/OpenTelemetry/Sdk.cs index 2ac5d4f1414..d44b9459ba6 100644 --- a/src/OpenTelemetry/Sdk.cs +++ b/src/OpenTelemetry/Sdk.cs @@ -64,7 +64,7 @@ public static MeterProvider CreateMeterProvider(Action 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 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 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, diff --git a/src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs b/src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs index c01618be413..54a9892344b 100644 --- a/src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs +++ b/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); diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs index 58eca8970b7..7f72539ad65 100644 --- a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs +++ b/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)))) From 5c46069248f0e290fc90835ac58693abac430918 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 5 Aug 2020 11:43:34 -0700 Subject: [PATCH 2/2] Fixed http-in instrumentation creating Activity objects with invalid parents. --- .../Implementation/HttpInListener.cs | 36 ++++++++++--------- .../Implementation/HttpInListener.cs | 24 +++++++------ .../Trace/ActivitySourceAdapter.cs | 25 ++++++------- 3 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs index ac16a2f20ec..90f6dd0dbb7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs +++ b/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 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 38ed2ff82c4..9b37bbc05be 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/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); diff --git a/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs b/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs index fc3ca46e45a..cc3eb187463 100644 --- a/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs +++ b/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(