From bf0184e8790aa35df5d592ccffe5a8fd30deecf6 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 10 Mar 2021 16:00:18 -0800 Subject: [PATCH 1/8] Optimize ASP.NET Core instrumentation for SuppressInstrumentation --- .../Implementation/HttpInListener.cs | 54 ++++++++++++------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 911db12ce20..6860a1bcceb 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -57,29 +57,29 @@ public HttpInListener(string name, AspNetCoreInstrumentationOptions options) [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")] public override void OnStartActivity(Activity activity, object payload) { - _ = this.startContextFetcher.TryFetch(payload, out HttpContext context); - if (context == null) + // The overall flow of what HttpClient library does is as below: + // Activity.Start() + // DiagnosticSource.WriteEvent("Start", payload) + // DiagnosticSource.WriteEvent("Stop", payload) + // Activity.Stop() + + // This method is in the WriteEvent("Start", payload) path. + // By this time, samplers have already run and + // activity.IsAllDataRequested populated accordingly. + + if (Sdk.SuppressInstrumentation) { - AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity)); return; } - try - { - if (this.options.Filter?.Invoke(context) == false) - { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName); - activity.IsAllDataRequested = false; - return; - } - } - catch (Exception ex) + _ = this.startContextFetcher.TryFetch(payload, out HttpContext context); + if (context == null) { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(ex); - activity.IsAllDataRequested = false; + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity)); return; } + // Enusre context propagation irrespective of sampling decision var request = context.Request; var textMapPropagator = Propagators.DefaultTextMapPropagator; if (!this.hostingSupportsW3C || !(textMapPropagator is TraceContextPropagator)) @@ -110,11 +110,29 @@ public override void OnStartActivity(Activity activity, object payload) } } - ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource); - ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server); - + // enrich Activity from payload only if sampling decision + // is favorable. if (activity.IsAllDataRequested) { + try + { + if (this.options.Filter?.Invoke(context) == false) + { + AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName); + activity.IsAllDataRequested = false; + return; + } + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.RequestFilterException(ex); + activity.IsAllDataRequested = false; + return; + } + + ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource); + ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server); + var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/"; activity.DisplayName = path; From bb8b09451dff744f2c1a5dbd3942518bd5f5a30d Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 10 Mar 2021 23:44:15 -0800 Subject: [PATCH 2/8] Updated unit tests --- .../BasicTests.cs | 212 ++++++++++++------ ...ry.Instrumentation.AspNetCore.Tests.csproj | 1 + .../Controllers/ChildActivityController.cs | 49 ++++ .../Controllers/ChildActivityController.cs | 49 ++++ .../Controllers/ChildActivityController.cs | 49 ++++ 5 files changed, 286 insertions(+), 74 deletions(-) create mode 100644 test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs create mode 100644 test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs create mode 100644 test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 0dfe11d32ce..cbdf6e03723 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -26,6 +26,7 @@ using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; using Moq; +using Newtonsoft.Json; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Instrumentation.AspNetCore.Implementation; using OpenTelemetry.Tests; @@ -97,6 +98,8 @@ void ConfigureTestServices(IServiceCollection services) var status = activity.GetStatus(); Assert.Equal(status, Status.Unset); + + ValidateAspNetCoreActivity(activity, "/api/values"); } [Theory] @@ -190,95 +193,101 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext() // is always ignored and new one is created by the Instrumentation Assert.Equal("ActivityCreatedByHttpInListener", activity.OperationName); #endif - Assert.Equal(ActivityKind.Server, activity.Kind); Assert.Equal("api/Values/{id}", activity.DisplayName); - Assert.Equal("/api/values/2", activity.GetTagValue(SpanAttributeConstants.HttpPathKey) as string); Assert.Equal(expectedTraceId, activity.Context.TraceId); Assert.Equal(expectedSpanId, activity.ParentSpanId); + + ValidateAspNetCoreActivity(activity, "/api/values/2"); } [Fact] public async Task CustomPropagator() { - var activityProcessor = new Mock>(); - - var expectedTraceId = ActivityTraceId.CreateRandom(); - var expectedSpanId = ActivitySpanId.CreateRandom(); - - var propagator = new Mock(); - propagator.Setup(m => m.Extract(It.IsAny(), It.IsAny(), It.IsAny>>())).Returns( - new PropagationContext( - new ActivityContext( - expectedTraceId, - expectedSpanId, - ActivityTraceFlags.Recorded), - default)); - - // Arrange - using (var testFactory = this.factory - .WithWebHostBuilder(builder => - builder.ConfigureTestServices(services => - { - Sdk.SetDefaultTextMapPropagator(propagator.Object); - this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder() - .AddAspNetCoreInstrumentation() - .AddProcessor(activityProcessor.Object) - .Build(); - }))) + try { - using var client = testFactory.CreateClient(); - var response = await client.GetAsync("/api/values/2"); - response.EnsureSuccessStatusCode(); // Status Code 200-299 + var activityProcessor = new Mock>(); + + var expectedTraceId = ActivityTraceId.CreateRandom(); + var expectedSpanId = ActivitySpanId.CreateRandom(); + + var propagator = new Mock(); + propagator.Setup(m => m.Extract(It.IsAny(), It.IsAny(), It.IsAny>>())).Returns( + new PropagationContext( + new ActivityContext( + expectedTraceId, + expectedSpanId, + ActivityTraceFlags.Recorded), + default)); + + // Arrange + using (var testFactory = this.factory + .WithWebHostBuilder(builder => + builder.ConfigureTestServices(services => + { + Sdk.SetDefaultTextMapPropagator(propagator.Object); + this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder() + .AddAspNetCoreInstrumentation() + .AddProcessor(activityProcessor.Object) + .Build(); + }))) + { + using var client = testFactory.CreateClient(); + var response = await client.GetAsync("/api/values/2"); + response.EnsureSuccessStatusCode(); // Status Code 200-299 - WaitForProcessorInvocations(activityProcessor, 4); - } + WaitForProcessorInvocations(activityProcessor, 4); + } - // List of invocations on the processor - // 1. SetParentProvider for TracerProviderSdk - // 2. OnStart for the activity created by AspNetCore with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn - // 3. OnStart for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener - // 4. OnEnd for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener - Assert.Equal(4, activityProcessor.Invocations.Count); + // List of invocations on the processor + // 1. SetParentProvider for TracerProviderSdk + // 2. OnStart for the activity created by AspNetCore with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + // 3. OnStart for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener + // 4. OnEnd for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener + Assert.Equal(4, activityProcessor.Invocations.Count); - var startedActivities = activityProcessor.Invocations.Where(invo => invo.Method.Name == "OnStart"); - var stoppedActivities = activityProcessor.Invocations.Where(invo => invo.Method.Name == "OnEnd"); - Assert.Equal(2, startedActivities.Count()); - Assert.Single(stoppedActivities); + var startedActivities = activityProcessor.Invocations.Where(invo => invo.Method.Name == "OnStart"); + var stoppedActivities = activityProcessor.Invocations.Where(invo => invo.Method.Name == "OnEnd"); + Assert.Equal(2, startedActivities.Count()); + Assert.Single(stoppedActivities); - // The activity created by the framework and the sibling activity are both sent to Processor.OnStart - Assert.Contains(startedActivities, item => - { - var startedActivity = item.Arguments[0] as Activity; - return startedActivity.OperationName == HttpInListener.ActivityOperationName; - }); + // The activity created by the framework and the sibling activity are both sent to Processor.OnStart + Assert.Contains(startedActivities, item => + { + var startedActivity = item.Arguments[0] as Activity; + return startedActivity.OperationName == HttpInListener.ActivityOperationName; + }); - Assert.Contains(startedActivities, item => - { - var startedActivity = item.Arguments[0] as Activity; - return startedActivity.OperationName == HttpInListener.ActivityNameByHttpInListener; - }); + Assert.Contains(startedActivities, item => + { + var startedActivity = item.Arguments[0] as Activity; + return startedActivity.OperationName == HttpInListener.ActivityNameByHttpInListener; + }); - // Only the sibling activity is sent to Processor.OnEnd - Assert.Contains(stoppedActivities, item => - { - var stoppedActivity = item.Arguments[0] as Activity; - return stoppedActivity.OperationName == HttpInListener.ActivityNameByHttpInListener; - }); + // Only the sibling activity is sent to Processor.OnEnd + Assert.Contains(stoppedActivities, item => + { + var stoppedActivity = item.Arguments[0] as Activity; + return stoppedActivity.OperationName == HttpInListener.ActivityNameByHttpInListener; + }); - var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; - Assert.Equal(ActivityKind.Server, activity.Kind); - Assert.True(activity.Duration != TimeSpan.Zero); - Assert.Equal("api/Values/{id}", activity.DisplayName); - Assert.Equal("/api/values/2", activity.GetTagValue(SpanAttributeConstants.HttpPathKey) as string); + var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; + Assert.True(activity.Duration != TimeSpan.Zero); + Assert.Equal("api/Values/{id}", activity.DisplayName); - Assert.Equal(expectedTraceId, activity.Context.TraceId); - Assert.Equal(expectedSpanId, activity.ParentSpanId); - Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[] + Assert.Equal(expectedTraceId, activity.Context.TraceId); + Assert.Equal(expectedSpanId, activity.ParentSpanId); + + ValidateAspNetCoreActivity(activity, "/api/values/2"); + } + finally { - new TraceContextPropagator(), - new BaggagePropagator(), - })); + Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[] + { + new TraceContextPropagator(), + new BaggagePropagator(), + })); + } } [Fact] @@ -322,8 +331,7 @@ void ConfigureTestServices(IServiceCollection services) Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; - Assert.Equal(ActivityKind.Server, activity.Kind); - Assert.Equal("/api/values", activity.GetTagValue(SpanAttributeConstants.HttpPathKey) as string); + ValidateAspNetCoreActivity(activity, "/api/values"); } [Fact] @@ -383,8 +391,62 @@ void ConfigureTestServices(IServiceCollection services) Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; - Assert.Equal(ActivityKind.Server, activity.Kind); - Assert.Equal("/api/values", activity.GetTagValue(SpanAttributeConstants.HttpPathKey) as string); + ValidateAspNetCoreActivity(activity, "/api/values"); + } + + [Fact] + public async Task PropagateContextIrrespectiveOfSamplingDecision() + { + var activityProcessor = new Mock>(); + + var expectedTraceId = ActivityTraceId.CreateRandom(); + var expectedParentSpanId = ActivitySpanId.CreateRandom(); + + // Arrange + using (var testFactory = this.factory + .WithWebHostBuilder(builder => + builder.ConfigureTestServices(services => + { + this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder() + .SetSampler(new AlwaysOffSampler()) + .AddAspNetCoreInstrumentation() + .AddProcessor(activityProcessor.Object) + .Build(); + }))) + { + using var client = testFactory.CreateClient(); + + // Test TraceContext Propagation + var expectedTraceState = "rojo=1,congo=2"; + var request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityTraceContext"); + request.Headers.Add("traceparent", $"00-{expectedTraceId}-{expectedParentSpanId}-01"); + request.Headers.Add("tracestate", expectedTraceState); + + var response = await client.SendAsync(request); + var childActivityTraceContext = JsonConvert.DeserializeObject>(response.Content.ReadAsStringAsync().Result); + + response.EnsureSuccessStatusCode(); + + Assert.Equal(expectedTraceId.ToString(), childActivityTraceContext["TraceId"]); + Assert.Equal(expectedTraceState, childActivityTraceContext["TraceState"]); + Assert.NotEqual(expectedParentSpanId.ToString(), childActivityTraceContext["ParentSpanId"]); // there is a new activity created in instrumentation therefore the ParentSpanId is different that what is provided in the headers + + // Test Baggage Context Propagation + request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityBaggageContext"); + request.Headers.Add("Baggage", "key1=value1,key2=value2"); + response = await client.SendAsync(request); + var childActivityBaggageContext = JsonConvert.DeserializeObject>(response.Content.ReadAsStringAsync().Result); + + response.EnsureSuccessStatusCode(); + + Assert.Single(childActivityBaggageContext, item => item.Key == "key1" && item.Value == "value1"); + Assert.Single(childActivityBaggageContext, item => item.Key == "key2" && item.Value == "value2"); + + WaitForProcessorInvocations(activityProcessor, 1); + } + + // Processor.OnStart and Processor.OnEnd are not called as activity.IsAllDataRequested is set to false + Assert.Equal(1, activityProcessor.Invocations.Count); // Only Processor.SetParentProvider is called } public void Dispose() @@ -409,6 +471,8 @@ private static void WaitForProcessorInvocations(Mock> ac private static void ValidateAspNetCoreActivity(Activity activityToValidate, string expectedHttpPath) { Assert.Equal(ActivityKind.Server, activityToValidate.Kind); + Assert.Equal(HttpInListener.ActivitySourceName, activityToValidate.Source.Name); + Assert.Equal(HttpInListener.Version.ToString(), activityToValidate.Source.Version); Assert.Equal(expectedHttpPath, activityToValidate.GetTagValue(SpanAttributeConstants.HttpPathKey) as string); } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj index d054ac6dc85..d9263133869 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj @@ -7,6 +7,7 @@ + all diff --git a/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs new file mode 100644 index 00000000000..96169ba2433 --- /dev/null +++ b/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs @@ -0,0 +1,49 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System.Collections.Generic; +using System.Diagnostics; +using Microsoft.AspNetCore.Mvc; +using OpenTelemetry; + +namespace TestApp.AspNetCore._2._1.Controllers +{ + public class ChildActivityController : Controller + { + [Route("api/GetChildActivityTraceContext")] + public Dictionary GetChildActivityTraceContext() + { + var result = new Dictionary(); + var activity = new Activity("ActivityInsideHttpRequest"); + activity.Start(); + result["TraceId"] = activity.Context.TraceId.ToString(); + result["ParentSpanId"] = activity.ParentSpanId.ToString(); + result["TraceState"] = activity.Context.TraceState; + activity.Stop(); + return result; + } + + [Route("api/GetChildActivityBaggageContext")] + public IReadOnlyDictionary GetChildActivityBaggageContext() + { + var activity = new Activity("ActivityInsideHttpRequest"); + activity.Start(); + var result = Baggage.Current.GetBaggage(); + activity.Stop(); + return result; + } + } +} diff --git a/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs new file mode 100644 index 00000000000..297a546e6f0 --- /dev/null +++ b/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs @@ -0,0 +1,49 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System.Collections.Generic; +using System.Diagnostics; +using Microsoft.AspNetCore.Mvc; +using OpenTelemetry; + +namespace TestApp.AspNetCore._3._1.Controllers +{ + public class ChildActivityController : Controller + { + [Route("api/GetChildActivityTraceContext")] + public Dictionary GetChildActivityTraceContext() + { + var result = new Dictionary(); + var activity = new Activity("ActivityInsideHttpRequest"); + activity.Start(); + result["TraceId"] = activity.Context.TraceId.ToString(); + result["ParentSpanId"] = activity.ParentSpanId.ToString(); + result["TraceState"] = activity.Context.TraceState; + activity.Stop(); + return result; + } + + [Route("api/GetChildActivityBaggageContext")] + public IReadOnlyDictionary GetChildActivityBaggageContext() + { + var activity = new Activity("ActivityInsideHttpRequest"); + activity.Start(); + var result = Baggage.Current.GetBaggage(); + activity.Stop(); + return result; + } + } +} diff --git a/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs new file mode 100644 index 00000000000..8d7a3b469aa --- /dev/null +++ b/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs @@ -0,0 +1,49 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System.Collections.Generic; +using System.Diagnostics; +using Microsoft.AspNetCore.Mvc; +using OpenTelemetry; + +namespace TestApp.AspNetCore._5._0.Controllers +{ + public class ChildActivityController : Controller + { + [Route("api/GetChildActivityTraceContext")] + public Dictionary GetChildActivityTraceContext() + { + var result = new Dictionary(); + var activity = new Activity("ActivityInsideHttpRequest"); + activity.Start(); + result["TraceId"] = activity.Context.TraceId.ToString(); + result["ParentSpanId"] = activity.ParentSpanId.ToString(); + result["TraceState"] = activity.Context.TraceState; + activity.Stop(); + return result; + } + + [Route("api/GetChildActivityBaggageContext")] + public IReadOnlyDictionary GetChildActivityBaggageContext() + { + var activity = new Activity("ActivityInsideHttpRequest"); + activity.Start(); + var result = Baggage.Current.GetBaggage(); + activity.Stop(); + return result; + } + } +} From 63b1a7f9dde41016c51c6478e1bd039828d82899 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 11 Mar 2021 10:16:53 -0800 Subject: [PATCH 3/8] Update src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Co-authored-by: Cijo Thomas --- .../Implementation/HttpInListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 6860a1bcceb..bcb38076c68 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -57,7 +57,7 @@ public HttpInListener(string name, AspNetCoreInstrumentationOptions options) [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")] public override void OnStartActivity(Activity activity, object payload) { - // The overall flow of what HttpClient library does is as below: + // The overall flow of what AspNetCore library does is as below: // Activity.Start() // DiagnosticSource.WriteEvent("Start", payload) // DiagnosticSource.WriteEvent("Stop", payload) From 1582a5637c84187c25a6711bfd0a505e076348d7 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 11 Mar 2021 10:17:06 -0800 Subject: [PATCH 4/8] Update src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Co-authored-by: Cijo Thomas --- .../Implementation/HttpInListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index bcb38076c68..d1be83c94f3 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -79,7 +79,7 @@ public override void OnStartActivity(Activity activity, object payload) return; } - // Enusre context propagation irrespective of sampling decision + // Ensure context propagation irrespective of sampling decision var request = context.Request; var textMapPropagator = Propagators.DefaultTextMapPropagator; if (!this.hostingSupportsW3C || !(textMapPropagator is TraceContextPropagator)) From 1c366004340f83a68f06cdd8a150492bf38bc203 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 11 Mar 2021 14:28:36 -0800 Subject: [PATCH 5/8] Addressed PR comments --- .../Implementation/HttpInListener.cs | 2 +- .../BasicTests.cs | 168 +++++++++++++----- ...ry.Instrumentation.AspNetCore.Tests.csproj | 1 - .../Controllers/ChildActivityController.cs | 2 - .../Controllers/ChildActivityController.cs | 2 - .../Controllers/ChildActivityController.cs | 2 - 6 files changed, 128 insertions(+), 49 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index d1be83c94f3..467f628dcbb 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -79,7 +79,7 @@ public override void OnStartActivity(Activity activity, object payload) return; } - // Ensure context propagation irrespective of sampling decision + // Ensure context extraction irrespective of sampling decision var request = context.Request; var textMapPropagator = Propagators.DefaultTextMapPropagator; if (!this.hostingSupportsW3C || !(textMapPropagator is TraceContextPropagator)) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index cbdf6e03723..c34f7e7fe94 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -394,59 +394,106 @@ void ConfigureTestServices(IServiceCollection services) ValidateAspNetCoreActivity(activity, "/api/values"); } - [Fact] - public async Task PropagateContextIrrespectiveOfSamplingDecision() + [Theory] + [InlineData(SamplingDecision.Drop)] + [InlineData(SamplingDecision.RecordOnly)] + [InlineData(SamplingDecision.RecordAndSample)] + public async Task ExtractContextIrrespectiveOfSamplingDecision(SamplingDecision samplingDecision) { - var activityProcessor = new Mock>(); - - var expectedTraceId = ActivityTraceId.CreateRandom(); - var expectedParentSpanId = ActivitySpanId.CreateRandom(); - - // Arrange - using (var testFactory = this.factory - .WithWebHostBuilder(builder => - builder.ConfigureTestServices(services => - { - this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder() - .SetSampler(new AlwaysOffSampler()) - .AddAspNetCoreInstrumentation() - .AddProcessor(activityProcessor.Object) - .Build(); - }))) + try { - using var client = testFactory.CreateClient(); - - // Test TraceContext Propagation + var expectedTraceId = ActivityTraceId.CreateRandom(); + var expectedParentSpanId = ActivitySpanId.CreateRandom(); var expectedTraceState = "rojo=1,congo=2"; - var request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityTraceContext"); - request.Headers.Add("traceparent", $"00-{expectedTraceId}-{expectedParentSpanId}-01"); - request.Headers.Add("tracestate", expectedTraceState); + var activityContext = new ActivityContext(expectedTraceId, expectedParentSpanId, ActivityTraceFlags.Recorded, expectedTraceState); + var expectedBaggage = Baggage.SetBaggage("key1", "value1").SetBaggage("key2", "value2"); + Sdk.SetDefaultTextMapPropagator(new ExtractOnlyPropagator(activityContext, expectedBaggage)); - var response = await client.SendAsync(request); - var childActivityTraceContext = JsonConvert.DeserializeObject>(response.Content.ReadAsStringAsync().Result); + // Arrange + using (var testFactory = this.factory + .WithWebHostBuilder(builder => + builder.ConfigureTestServices(services => + { + this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder() + .SetSampler(new TestSampler(samplingDecision)) + .AddAspNetCoreInstrumentation() + .Build(); + }))) + { + using var client = testFactory.CreateClient(); + + // Test TraceContext Propagation + var request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityTraceContext"); + var response = await client.SendAsync(request); + var childActivityTraceContext = JsonConvert.DeserializeObject>(response.Content.ReadAsStringAsync().Result); + + response.EnsureSuccessStatusCode(); - response.EnsureSuccessStatusCode(); + Assert.Equal(expectedTraceId.ToString(), childActivityTraceContext["TraceId"]); + Assert.Equal(expectedTraceState, childActivityTraceContext["TraceState"]); + Assert.NotEqual(expectedParentSpanId.ToString(), childActivityTraceContext["ParentSpanId"]); // there is a new activity created in instrumentation therefore the ParentSpanId is different that what is provided in the headers - Assert.Equal(expectedTraceId.ToString(), childActivityTraceContext["TraceId"]); - Assert.Equal(expectedTraceState, childActivityTraceContext["TraceState"]); - Assert.NotEqual(expectedParentSpanId.ToString(), childActivityTraceContext["ParentSpanId"]); // there is a new activity created in instrumentation therefore the ParentSpanId is different that what is provided in the headers + // Test Baggage Context Propagation + request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityBaggageContext"); - // Test Baggage Context Propagation - request = new HttpRequestMessage(HttpMethod.Get, "/api/GetChildActivityBaggageContext"); - request.Headers.Add("Baggage", "key1=value1,key2=value2"); - response = await client.SendAsync(request); - var childActivityBaggageContext = JsonConvert.DeserializeObject>(response.Content.ReadAsStringAsync().Result); + response = await client.SendAsync(request); + var childActivityBaggageContext = JsonConvert.DeserializeObject>(response.Content.ReadAsStringAsync().Result); - response.EnsureSuccessStatusCode(); + response.EnsureSuccessStatusCode(); - Assert.Single(childActivityBaggageContext, item => item.Key == "key1" && item.Value == "value1"); - Assert.Single(childActivityBaggageContext, item => item.Key == "key2" && item.Value == "value2"); + Assert.Single(childActivityBaggageContext, item => item.Key == "key1" && item.Value == "value1"); + Assert.Single(childActivityBaggageContext, item => item.Key == "key2" && item.Value == "value2"); + } + } + finally + { + Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[] + { + new TraceContextPropagator(), + new BaggagePropagator(), + })); + } + } - WaitForProcessorInvocations(activityProcessor, 1); + [Theory] + [InlineData(SamplingDecision.Drop, false, false)] + [InlineData(SamplingDecision.RecordOnly, true, true)] + [InlineData(SamplingDecision.RecordAndSample, true, true)] + public async Task FilterAndEnrichAreOnlyCalledWhenSampled(SamplingDecision samplingDecision, bool shouldFilterBeCalled, bool shouldEnrichBeCalled) + { + bool filterCalled = false; + bool enrichCalled = false; + void ConfigureTestServices(IServiceCollection services) + { + this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder() + .SetSampler(new TestSampler(samplingDecision)) + .AddAspNetCoreInstrumentation(options => + { + options.Filter = (context) => + { + filterCalled = true; + return true; + }; + options.Enrich = (activity, methodName, request) => + { + enrichCalled = true; + }; + }) + .Build(); } - // Processor.OnStart and Processor.OnEnd are not called as activity.IsAllDataRequested is set to false - Assert.Equal(1, activityProcessor.Invocations.Count); // Only Processor.SetParentProvider is called + // Arrange + using var client = this.factory + .WithWebHostBuilder(builder => + builder.ConfigureTestServices(ConfigureTestServices)) + .CreateClient(); + + // Act + var response = await client.GetAsync("/api/values"); + + // Assert + Assert.Equal(shouldFilterBeCalled, filterCalled); + Assert.Equal(shouldEnrichBeCalled, enrichCalled); } public void Dispose() @@ -493,5 +540,44 @@ private static void ActivityEnrichment(Activity activity, string method, object break; } } + + private class ExtractOnlyPropagator : TextMapPropagator + { + private readonly ActivityContext activityContext; + private readonly Baggage baggage; + + public ExtractOnlyPropagator(ActivityContext activityContext, Baggage baggage) + { + this.activityContext = activityContext; + this.baggage = baggage; + } + + public override ISet Fields => throw new NotImplementedException(); + + public override PropagationContext Extract(PropagationContext context, T carrier, Func> getter) + { + return new PropagationContext(this.activityContext, this.baggage); + } + + public override void Inject(PropagationContext context, T carrier, Action setter) + { + throw new NotImplementedException(); + } + } + + private class TestSampler : Sampler + { + private SamplingDecision samplingDecision; + + public TestSampler(SamplingDecision samplingDecision) + { + this.samplingDecision = samplingDecision; + } + + public override SamplingResult ShouldSample(in SamplingParameters samplingParameters) + { + return new SamplingResult(this.samplingDecision); + } + } } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj index d9263133869..d054ac6dc85 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj @@ -7,7 +7,6 @@ - all diff --git a/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs index 96169ba2433..daa1cfcae1e 100644 --- a/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs +++ b/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs @@ -40,9 +40,7 @@ public Dictionary GetChildActivityTraceContext() public IReadOnlyDictionary GetChildActivityBaggageContext() { var activity = new Activity("ActivityInsideHttpRequest"); - activity.Start(); var result = Baggage.Current.GetBaggage(); - activity.Stop(); return result; } } diff --git a/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs index 297a546e6f0..c7b777ff816 100644 --- a/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs +++ b/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs @@ -40,9 +40,7 @@ public Dictionary GetChildActivityTraceContext() public IReadOnlyDictionary GetChildActivityBaggageContext() { var activity = new Activity("ActivityInsideHttpRequest"); - activity.Start(); var result = Baggage.Current.GetBaggage(); - activity.Stop(); return result; } } diff --git a/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs index 8d7a3b469aa..4c4c867e955 100644 --- a/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs +++ b/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs @@ -40,9 +40,7 @@ public Dictionary GetChildActivityTraceContext() public IReadOnlyDictionary GetChildActivityBaggageContext() { var activity = new Activity("ActivityInsideHttpRequest"); - activity.Start(); var result = Baggage.Current.GetBaggage(); - activity.Stop(); return result; } } From c1955d5944a04a2bc992aa75a84127463c61e701 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 11 Mar 2021 14:31:47 -0800 Subject: [PATCH 6/8] Update TestAspNetCore controller --- .../Controllers/ChildActivityController.cs | 1 - .../Controllers/ChildActivityController.cs | 1 - .../Controllers/ChildActivityController.cs | 1 - 3 files changed, 3 deletions(-) diff --git a/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs index daa1cfcae1e..dab0b28f667 100644 --- a/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs +++ b/test/TestApp.AspNetCore.2.1/Controllers/ChildActivityController.cs @@ -39,7 +39,6 @@ public Dictionary GetChildActivityTraceContext() [Route("api/GetChildActivityBaggageContext")] public IReadOnlyDictionary GetChildActivityBaggageContext() { - var activity = new Activity("ActivityInsideHttpRequest"); var result = Baggage.Current.GetBaggage(); return result; } diff --git a/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs index c7b777ff816..911f9bae6a3 100644 --- a/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs +++ b/test/TestApp.AspNetCore.3.1/Controllers/ChildActivityController.cs @@ -39,7 +39,6 @@ public Dictionary GetChildActivityTraceContext() [Route("api/GetChildActivityBaggageContext")] public IReadOnlyDictionary GetChildActivityBaggageContext() { - var activity = new Activity("ActivityInsideHttpRequest"); var result = Baggage.Current.GetBaggage(); return result; } diff --git a/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs index 4c4c867e955..39c556d775c 100644 --- a/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs +++ b/test/TestApp.AspNetCore.5.0/Controllers/ChildActivityController.cs @@ -39,7 +39,6 @@ public Dictionary GetChildActivityTraceContext() [Route("api/GetChildActivityBaggageContext")] public IReadOnlyDictionary GetChildActivityBaggageContext() { - var activity = new Activity("ActivityInsideHttpRequest"); var result = Baggage.Current.GetBaggage(); return result; } From 14634098bcf1529a4defb940aca45c92b05d4653 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 18 Mar 2021 13:08:06 -0700 Subject: [PATCH 7/8] Updated CHANGELOG.md --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 1c18f0094f6..d6d6eb2b932 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -6,6 +6,9 @@ and ActivityProcessors. Samplers, ActivityProcessor.OnStart will now get the Activity before any enrichment done by the instrumentation. ([#1836](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1836)) +* Performance optimization by leveraging sampling decision and short circuiting + activity enrichment. + ([#1899](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1899)) ## 1.0.0-rc2 From b88ba572b2efc228d900c8cbf17401c14c6c90d5 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 18 Mar 2021 13:14:56 -0700 Subject: [PATCH 8/8] Updated CHANGELOG.md --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index d6d6eb2b932..97681fd0553 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -7,7 +7,8 @@ Activity before any enrichment done by the instrumentation. ([#1836](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1836)) * Performance optimization by leveraging sampling decision and short circuiting - activity enrichment. + activity enrichment. `Filter` and `Enrich` are now only called if + `activity.IsAllDataRequested` is `true` ([#1899](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1899)) ## 1.0.0-rc2