Add EnableGrpcAspNetCoreSupport option for ASP.NET Core instrumentati…

…on (#1423) * Add netstandard2.1 target for AspNetCore instrumentation * Add EnableGrpcAspNetCoreSupport option * Update changelog * Inline methods * Use 3.1 SDK for build stage Co-authored-by: Cijo Thomas <[email protected]>
open-telemetry · Oct 30, 2020 · 7cd1961 · 7cd1961
1 parent a8c5797
commit 7cd1961
Show file tree

Hide file tree

Showing 6 changed files with 86 additions and 18 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs
@@ -61,5 +61,15 @@ public class AspNetCoreInstrumentationOptions
         /// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md.
         /// </remarks>
         public bool RecordException { get; set; }
+
+#if NETSTANDARD2_1
+        /// <summary>
+        /// Gets or sets a value indicating whether RPC attributes are added to an Activity when using Grpc.AspNetCore. Default is true.
+        /// </summary>
+        /// <remarks>
+        /// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/rpc.md.
+        /// </remarks>
+        public bool EnableGrpcAspNetCoreSupport { get; set; } = true;
+#endif
     }
 }
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
@@ -5,6 +5,11 @@
 * Record `Exception` in AspNetCore instrumentation based on `RecordException` in
   `AspNetCoreInstrumentationOptions`
   ([#1408](https://github.com/open-telemetry/opentelemetry-dotnet/issues/1408))
+* Added configuration option `EnableGrpcAspNetCoreSupport` to enable or disable
+  support for adding OpenTelemetry RPC attributes when using
+  [Grpc.AspNetCore](https://www.nuget.org/packages/Grpc.AspNetCore/).
+  This option is enabled by default.
+  ([#1423](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1423))
 
 ## 0.7.0-beta.1
 

diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
@@ -18,6 +18,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
+using System.Runtime.CompilerServices;
 using System.Text;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
@@ -165,15 +166,18 @@ public override void OnStopActivity(Activity activity, object payload)
 
                 activity.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode);
 
-                if (TryGetGrpcMethod(activity, out var grpcMethod))
+#if NETSTANDARD2_1
+                if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod))
                 {
                     AddGrpcAttributes(activity, grpcMethod, context);
                 }
                 else
                 {
-                    Status status = SpanHelper.ResolveSpanStatusForHttpStatusCode(response.StatusCode);
-                    activity.SetStatus(status);
+                    SetStatusFromHttpStatusCode(activity, response.StatusCode);
                 }
+#else
+                SetStatusFromHttpStatusCode(activity, response.StatusCode);
+#endif
             }
 
             if (activity.OperationName.Equals(ActivityNameByHttpInListener, StringComparison.Ordinal))
@@ -289,12 +293,22 @@ private static string GetUri(HttpRequest request)
             return builder.ToString();
         }
 
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static void SetStatusFromHttpStatusCode(Activity activity, int statusCode)
+        {
+            var status = SpanHelper.ResolveSpanStatusForHttpStatusCode(statusCode);
+            activity.SetStatus(status);
+        }
+
+#if NETSTANDARD2_1
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private static bool TryGetGrpcMethod(Activity activity, out string grpcMethod)
         {
             grpcMethod = GrpcTagHelper.GetGrpcMethodFromActivity(activity);
             return !string.IsNullOrEmpty(grpcMethod);
         }
 
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private static void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context)
         {
             activity.SetTag(SemanticConventions.AttributeRpcSystem, GrpcTagHelper.RpcSystemGrpc);
@@ -306,11 +320,19 @@ private static void AddGrpcAttributes(Activity activity, string grpcMethod, Http
 
                 // Remove the grpc.method tag added by the gRPC .NET library
                 activity.SetTag(GrpcTagHelper.GrpcMethodTagName, null);
+
+                // TODO: The grpc.status_code attribute added by the library is not currently
+                // removed because the tracing spec for span status is no longer based on
+                // gRPC status codes. Ultimately, the grpc.status_code tag should be replaced
+                // by whatever semantic convention is settled on in the RPC spec.
+                // See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/1345
+                // activity.SetTag(GrpcTagHelper.GrpcStatusCodeTagName, null);
             }
 
             activity.SetTag(SemanticConventions.AttributeNetPeerIp, context.Connection.RemoteIpAddress.ToString());
             activity.SetTag(SemanticConventions.AttributeNetPeerPort, context.Connection.RemotePort);
             activity.SetStatus(GrpcTagHelper.GetGrpcStatusCodeFromActivity(activity));
         }
+#endif
     }
 }
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj b/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj
@@ -1,6 +1,6 @@
 <Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
-    <TargetFrameworks>netstandard2.0</TargetFrameworks>
+    <TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks>
     <Description>ASP.NET Core instrumentation for OpenTelemetry .NET</Description>
     <PackageTags>$(PackageTags);distributed-tracing;AspNetCore</PackageTags>
   </PropertyGroup>

diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs
@@ -38,13 +38,29 @@ public GrpcTests()
             this.server = new GrpcServer<GreeterService>();
         }
 
-        [Fact]
-        public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes()
+        [Theory]
+        [InlineData(null)]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(bool? enableGrpcAspNetCoreSupport)
         {
             var processor = new Mock<BaseProcessor<Activity>>();
 
-            using var tracerProvider = Sdk.CreateTracerProviderBuilder()
-                .AddAspNetCoreInstrumentation()
+            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();
 
@@ -62,11 +78,30 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes()
                 invo.Method.Name == "OnEnd" && (invo.Arguments[0] as Activity).OperationName == "Microsoft.AspNetCore.Hosting.HttpRequestIn").Arguments[0];
 
             Assert.Equal(ActivityKind.Server, activity.Kind);
-            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));
+
+            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));
+
+                // Tags added by the library then removed from the instrumentation
+                // TODO: The grpc.status_code attribute added by the library is not currently
+                // removed because the tracing spec for span status is no longer based on
+                // gRPC status codes. Ultimately, the grpc.status_code tag should be replaced
+                // by whatever semantic convention is settled on in the RPC spec.
+                // See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/1345
+                Assert.Null(activity.GetTagValue(GrpcTagHelper.GrpcMethodTagName));
+                Assert.NotNull(activity.GetTagValue(GrpcTagHelper.GrpcStatusCodeTagName));
+            }
+            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.
@@ -75,10 +110,6 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes()
             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);
-
-            // Tags added by the library then removed from the instrumentation
-            Assert.Null(activity.GetTagValue(GrpcTagHelper.GrpcMethodTagName));
-            Assert.NotNull(activity.GetTagValue(GrpcTagHelper.GrpcStatusCodeTagName));
         }
 
         public void Dispose()

diff --git a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile
@@ -9,7 +9,7 @@ WORKDIR /w3c
 RUN apt-get update && apt-get install -y git
 RUN git clone https://github.com/w3c/trace-context.git
 
-FROM mcr.microsoft.com/dotnet/core/sdk:${SDK_VERSION} AS build
+FROM mcr.microsoft.com/dotnet/core/sdk:3.1 AS build
 ARG PUBLISH_CONFIGURATION=Release
 ARG PUBLISH_FRAMEWORK=netcoreapp3.1
 WORKDIR /repo