Update HttpInListener to add gRPC tags - By creating a new activity w…

…ith the OperationName used by the framework (#1879)

src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs

@@ -77,6 +77,32 @@ public static object GetTagValue(this Activity activity, string tagName)
             return state.Value;
         }
 
+        /// <summary>
+        /// Checks if the user provided tag name is the first tag of the <see cref="Activity"/> and retrieves the tag value.
+        /// </summary>
+        /// <param name="activity">Activity instance.</param>
+        /// <param name="tagName">Tag name.</param>
+        /// <param name="tagValue">Tag value.</param>
+        /// <returns><see langword="true"/> if the first tag of the supplied Activity matches the user provide tag name.</returns>
+        [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<ActivityFirstTagEnumerator>.Enumerate(activity, ref state);
+
+            if (state.Value == null)
+            {
+                tagValue = null;
+                return false;
+            }
+
+            tagValue = state.Value;
+            return true;
+        }
+
         /// <summary>
         /// Enumerates all the key/value pairs on an <see cref="Activity"/> without performing an allocation.
         /// </summary>
@@ -199,6 +225,29 @@ public bool ForEach(KeyValuePair<string, object> item)
             }
         }
 
+        private struct ActivityFirstTagEnumerator : IActivityEnumerator<KeyValuePair<string, object>>
+        {
+            public object Value;
+
+            private readonly string tagName;
+
+            public ActivityFirstTagEnumerator(string tagName)
+            {
+                this.tagName = tagName;
+                this.Value = null;
+            }
+
+            public bool ForEach(KeyValuePair<string, object> item)
+            {
+                if (item.Key == this.tagName)
+                {
+                    this.Value = item.Value;
+                }
+
+                return false;
+            }
+        }
+
         private static class ActivityTagsEnumeratorFactory<TState>
             where TState : struct, IActivityEnumerator<KeyValuePair<string, object>>
         {

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

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.

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);
         }
     }

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);

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<BaseProcessor<Activity>>();
+                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();

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()
         {