diff --git a/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs index cdd5d92cd07..6cc238b8c23 100644 --- a/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs +++ b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs @@ -77,6 +77,32 @@ public static object GetTagValue(this Activity activity, string tagName) return state.Value; } + /// + /// Checks if the user provided tag name is the first tag of the and retrieves the tag value. + /// + /// Activity instance. + /// Tag name. + /// Tag value. + /// if the first tag of the supplied Activity matches the user provide tag name. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool TryCheckFirstTag(this Activity activity, string tagName, out object tagValue) + { + Debug.Assert(activity != null, "Activity should not be null"); + + ActivityFirstTagEnumerator state = new ActivityFirstTagEnumerator(tagName); + + ActivityTagsEnumeratorFactory.Enumerate(activity, ref state); + + if (state.Value == null) + { + tagValue = null; + return false; + } + + tagValue = state.Value; + return true; + } + /// /// Enumerates all the key/value pairs on an without performing an allocation. /// @@ -199,6 +225,29 @@ public bool ForEach(KeyValuePair item) } } + private struct ActivityFirstTagEnumerator : IActivityEnumerator> + { + public object Value; + + private readonly string tagName; + + public ActivityFirstTagEnumerator(string tagName) + { + this.tagName = tagName; + this.Value = null; + } + + public bool ForEach(KeyValuePair item) + { + if (item.Key == this.tagName) + { + this.Value = item.Value; + } + + return false; + } + } + private static class ActivityTagsEnumeratorFactory where TState : struct, IActivityEnumerator> { diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 9f062cec753..2e6374632ec 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +* Fixes bug + [#1740](https://github.com/open-telemetry/opentelemetry-dotnet/issues/1740): + Instrumentation.AspNetCore for gRPC services omits ALL rpc.* attributes under + certain conditions + ([#1879](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1879)) + ## 1.0.0-rc4 Released 2021-Apr-23 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 9cfeffde1a7..b9ff96a4edf 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -31,7 +31,6 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation internal class HttpInListener : ListenerHandler { internal const string ActivityOperationName = "Microsoft.AspNetCore.Hosting.HttpRequestIn"; - internal const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener"; internal static readonly AssemblyName AssemblyName = typeof(HttpInListener).Assembly.GetName(); internal static readonly string ActivitySourceName = AssemblyName.Name; internal static readonly Version Version = AssemblyName.Version; @@ -92,10 +91,12 @@ public override void OnStartActivity(Activity activity, object payload) // 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); + Activity newOne = new Activity(ActivityOperationName); newOne.SetParentId(ctx.ActivityContext.TraceId, ctx.ActivityContext.SpanId, ctx.ActivityContext.TraceFlags); newOne.TraceStateString = ctx.ActivityContext.TraceState; + newOne.SetTag("IsCreatedByInstrumentation", bool.TrueString); + // Starting the new activity make it the Activity.Current one. newOne.Start(); @@ -214,10 +215,11 @@ public override void OnStopActivity(Activity activity, object payload) } } - if (activity.OperationName.Equals(ActivityNameByHttpInListener, StringComparison.Ordinal)) + if (activity.TryCheckFirstTag("IsCreatedByInstrumentation", out var tagValue) && ReferenceEquals(tagValue, bool.TrueString)) { // If instrumentation started a new Activity, it must // be stopped here. + activity.SetTag("IsCreatedByInstrumentation", null); activity.Stop(); // After the activity.Stop() code, Activity.Current becomes null. diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index faba8be5714..8d48fa00a33 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -57,7 +57,6 @@ private static TracerProviderBuilder AddAspNetCoreInstrumentation(TracerProvider var instrumentation = new AspNetCoreInstrumentation(options); builder.AddSource(HttpInListener.ActivitySourceName); builder.AddLegacySource(HttpInListener.ActivityOperationName); // for the activities created by AspNetCore - builder.AddLegacySource(HttpInListener.ActivityNameByHttpInListener); // for the sibling activities created by the instrumentation library return builder.AddInstrumentation(() => instrumentation); } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index c55f5f02da4..9288cf323f7 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -177,22 +177,14 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext() // List of invocations // 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 + // 3. OnStart for the sibling activity created by the instrumentation library with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn and the first tag that is added is (IsCreatedByInstrumentation, bool.TrueString) + // 4. OnEnd for the sibling activity created by the instrumentation library with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn and the first tag that is added is (IsCreatedByInstrumentation, bool.TrueString) - // we should only call Processor.OnEnd once for the sibling activity with the OperationName ActivityCreatedByHttpInListener + // we should only call Processor.OnEnd once for the sibling activity Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; -#if !NETCOREAPP2_1 - // ASP.NET Core after 2.x is W3C aware and hence Activity created by it - // must be used. Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", activity.OperationName); -#else - // ASP.NET Core before 3.x is not W3C aware and hence Activity created by it - // is always ignored and new one is created by the Instrumentation - Assert.Equal("ActivityCreatedByHttpInListener", activity.OperationName); -#endif Assert.Equal("api/Values/{id}", activity.DisplayName); Assert.Equal(expectedTraceId, activity.Context.TraceId); @@ -242,8 +234,8 @@ public async Task CustomPropagator() // 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 + // 3. OnStart for the sibling activity created by the instrumentation library with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn and the first tag that is added is (IsCreatedByInstrumentation, bool.TrueString) + // 4. OnEnd for the sibling activity created by the instrumentation library with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn and the first tag that is added is (IsCreatedByInstrumentation, bool.TrueString) Assert.Equal(4, activityProcessor.Invocations.Count); var startedActivities = activityProcessor.Invocations.Where(invo => invo.Method.Name == "OnStart"); @@ -252,24 +244,14 @@ public async Task CustomPropagator() Assert.Single(stoppedActivities); // The activity created by the framework and the sibling activity are both sent to Processor.OnStart - Assert.Contains(startedActivities, item => + Assert.Equal(2, startedActivities.Count(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; - }); + })); - // 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; - }); + // we should only call Processor.OnEnd once for the sibling activity + Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; Assert.True(activity.Duration != TimeSpan.Zero); diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs index c8f313352ba..bfe23c2c71c 100644 --- a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs +++ b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs @@ -20,12 +20,15 @@ using System.Net; using System.Threading; using Greet; +using Grpc.Core; using Grpc.Net.Client; using Moq; +using OpenTelemetry.Context.Propagation; using OpenTelemetry.Instrumentation.Grpc.Tests.Services; using OpenTelemetry.Instrumentation.GrpcNetClient; using OpenTelemetry.Trace; using Xunit; +using Status = OpenTelemetry.Trace.Status; namespace OpenTelemetry.Instrumentation.Grpc.Tests { @@ -109,6 +112,91 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(bool? enableGrpcA Assert.StartsWith("grpc-dotnet", activity.GetTagValue(SemanticConventions.AttributeHttpUserAgent) as string); } + [Theory] + [InlineData(null)] + [InlineData(true)] + [InlineData(false)] + public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewActivity(bool? enableGrpcAspNetCoreSupport) + { + try + { + // B3Propagator along with the headers passed to the client.SayHello ensure that the instrumentation creates a sibling activity + Sdk.SetDefaultTextMapPropagator(new B3Propagator()); + var processor = new Mock>(); + var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder(); + + if (enableGrpcAspNetCoreSupport.HasValue) + { + tracerProviderBuilder.AddAspNetCoreInstrumentation(options => + { + options.EnableGrpcAspNetCoreSupport = enableGrpcAspNetCoreSupport.Value; + }); + } + else + { + tracerProviderBuilder.AddAspNetCoreInstrumentation(); + } + + using var tracerProvider = tracerProviderBuilder + .AddProcessor(processor.Object) + .Build(); + + var clientLoopbackAddresses = new[] { IPAddress.Loopback.ToString(), IPAddress.IPv6Loopback.ToString() }; + var uri = new Uri($"http://localhost:{this.server.Port}"); + + using var channel = GrpcChannel.ForAddress(uri); + var client = new Greeter.GreeterClient(channel); + var headers = new Metadata(); + headers.Add("traceparent", "00-120dc44db5b736468afb112197b0dbd3-5dfbdf27ec544544-01"); + headers.Add("x-b3-traceid", "120dc44db5b736468afb112197b0dbd3"); + headers.Add("x-b3-spanid", "b0966f651b9e0126"); + headers.Add("x-b3-sampled", "1"); + client.SayHello(new HelloRequest(), headers); + + WaitForProcessorInvocations(processor, 4); + + Assert.Equal(4, processor.Invocations.Count); // SetParentProvider, OnStart (framework activity), OnStart (instrumentation activity), OnStop (instrumentation activity) + var activity = GetActivityFromProcessorInvocation(processor, nameof(processor.Object.OnEnd), OperationNameHttpRequestIn); + + Assert.Equal(ActivityKind.Server, activity.Kind); + + if (!enableGrpcAspNetCoreSupport.HasValue || enableGrpcAspNetCoreSupport.Value) + { + Assert.Equal("grpc", activity.GetTagValue(SemanticConventions.AttributeRpcSystem)); + Assert.Equal("greet.Greeter", activity.GetTagValue(SemanticConventions.AttributeRpcService)); + Assert.Equal("SayHello", activity.GetTagValue(SemanticConventions.AttributeRpcMethod)); + Assert.Contains(activity.GetTagValue(SemanticConventions.AttributeNetPeerIp), clientLoopbackAddresses); + Assert.NotEqual(0, activity.GetTagValue(SemanticConventions.AttributeNetPeerPort)); + Assert.Null(activity.GetTagValue(GrpcTagHelper.GrpcMethodTagName)); + Assert.Null(activity.GetTagValue(GrpcTagHelper.GrpcStatusCodeTagName)); + Assert.Equal(0, activity.GetTagValue(SemanticConventions.AttributeRpcGrpcStatusCode)); + } + else + { + Assert.NotNull(activity.GetTagValue(GrpcTagHelper.GrpcMethodTagName)); + Assert.NotNull(activity.GetTagValue(GrpcTagHelper.GrpcStatusCodeTagName)); + } + + Assert.Equal(Status.Unset, activity.GetStatus()); + + // The following are http.* attributes that are also included on the span for the gRPC invocation. + Assert.Equal($"localhost:{this.server.Port}", activity.GetTagValue(SemanticConventions.AttributeHttpHost)); + Assert.Equal("POST", activity.GetTagValue(SemanticConventions.AttributeHttpMethod)); + Assert.Equal("/greet.Greeter/SayHello", activity.GetTagValue(SpanAttributeConstants.HttpPathKey)); + Assert.Equal($"http://localhost:{this.server.Port}/greet.Greeter/SayHello", activity.GetTagValue(SemanticConventions.AttributeHttpUrl)); + Assert.StartsWith("grpc-dotnet", activity.GetTagValue(SemanticConventions.AttributeHttpUserAgent) as string); + } + finally + { + // Set the SDK to use the default propagator for other unit tests + Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[] + { + new TraceContextPropagator(), + new BaggagePropagator(), + })); + } + } + public void Dispose() { this.server.Dispose(); diff --git a/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs b/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs index 35e55ec5be8..a0590ebccd6 100644 --- a/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs +++ b/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs @@ -171,6 +171,29 @@ public void GetTagValue() Assert.Null(activity.GetTagValue("Tag2")); } + [Theory] + [InlineData("Key", "Value", true)] + [InlineData("CustomTag", null, false)] + public void TryCheckFirstTag(string tagName, object expectedTagValue, bool expectedResult) + { + Activity activity = new Activity("Test"); + activity.SetTag("Key", "Value"); + + var result = activity.TryCheckFirstTag(tagName, out var tagValue); + Assert.Equal(expectedResult, result); + Assert.Equal(expectedTagValue, tagValue); + } + + [Fact] + public void TryCheckFirstTagReturnsFalseForActivityWithNoTags() + { + Activity activity = new Activity("Test"); + + var result = activity.TryCheckFirstTag("Key", out var tagValue); + Assert.False(result); + Assert.Null(tagValue); + } + [Fact] public void EnumerateTagValuesEmpty() {