From 58e8f24782dc014ec82c03658b335f60583b4857 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Thu, 7 Dec 2023 14:58:57 -0800 Subject: [PATCH] Add grpc instrumentation with experimental feature flag (#5130) --- .../AspNetCoreTraceInstrumentationOptions.cs | 40 ++++++--- .../CHANGELOG.md | 14 +++ .../Implementation/HttpInListener.cs | 46 ++++------ ...elemetry.Instrumentation.AspNetCore.csproj | 5 +- .../TracerProviderBuilderExtensions.cs | 2 + .../GrpcTests.server.cs | 85 ++++++++++--------- 6 files changed, 108 insertions(+), 84 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs index 9c6f210a71d..f66a5e04344 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs @@ -3,6 +3,8 @@ using System.Diagnostics; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Configuration; +using OpenTelemetry.Internal; namespace OpenTelemetry.Instrumentation.AspNetCore; @@ -11,6 +13,24 @@ namespace OpenTelemetry.Instrumentation.AspNetCore; /// public class AspNetCoreTraceInstrumentationOptions { + /// + /// Initializes a new instance of the class. + /// + public AspNetCoreTraceInstrumentationOptions() + : this(new ConfigurationBuilder().AddEnvironmentVariables().Build()) + { + } + + internal AspNetCoreTraceInstrumentationOptions(IConfiguration configuration) + { + Debug.Assert(configuration != null, "configuration was null"); + + if (configuration.TryGetBoolValue("OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION", out var enableGrpcInstrumentation)) + { + this.EnableGrpcAspNetCoreSupport = enableGrpcInstrumentation; + } + } + /// /// Gets or sets a filter function that determines whether or not to /// collect telemetry on a per request basis. @@ -64,17 +84,11 @@ public class AspNetCoreTraceInstrumentationOptions /// public bool RecordException { get; set; } - /* - * Removing for stable release of http instrumentation. - * grpc semantic conventions are not yet stable so this option will not be part of stable package. - #if NET6_0_OR_GREATER - /// - /// Gets or sets a value indicating whether RPC attributes are added to an Activity when using Grpc.AspNetCore. Default is true. - /// - /// - /// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md. - /// - public bool EnableGrpcAspNetCoreSupport { get; set; } = true; - #endif - */ + /// + /// Gets or sets a value indicating whether RPC attributes are added to an Activity when using Grpc.AspNetCore. + /// + /// + /// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md. + /// + internal bool EnableGrpcAspNetCoreSupport { get; set; } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index d6dc29b65c5..5db9434309a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,20 @@ ## Unreleased +* Re-introduced support for gRPC instrumentation as an opt-in experimental + feature. From now onwards, gRPC can be enabled by setting + `OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION` flag to + `True`. `OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION` can + be set as an environment variable or via IConfiguration. The change is + introduced in order to support stable release of `http` instrumentation. + Semantic conventions for RPC is still + [experimental](https://github.com/open-telemetry/semantic-conventions/tree/main/docs/rpc) + and hence the package will only support it as an opt-in experimental feature. + Note that the support was removed in `1.6.0-rc.1` version of the package and + versions released before `1.6.0-rc.1` had gRPC instrumentation enabled by + default. + ([#5130](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5130)) + ## 1.6.0-rc.1 Released 2023-Dec-01 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 67a79c50eb8..471b131fc1e 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -6,17 +6,13 @@ using System.Diagnostics.CodeAnalysis; #endif using System.Reflection; -#if !NETSTANDARD2_0 using System.Runtime.CompilerServices; -#endif using Microsoft.AspNetCore.Http; #if !NETSTANDARD using Microsoft.AspNetCore.Routing; #endif using OpenTelemetry.Context.Propagation; -#if !NETSTANDARD2_0 using OpenTelemetry.Instrumentation.GrpcNetClient; -#endif using OpenTelemetry.Internal; using OpenTelemetry.Trace; @@ -170,7 +166,7 @@ public void OnStartActivity(Activity activity, object payload) #endif var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/"; - activity.DisplayName = this.GetDisplayName(request.Method); + activity.DisplayName = GetDisplayName(request.Method); // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md @@ -233,20 +229,18 @@ public void OnStopActivity(Activity activity, object payload) var routePattern = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; if (!string.IsNullOrEmpty(routePattern)) { - activity.DisplayName = this.GetDisplayName(context.Request.Method, routePattern); + activity.DisplayName = GetDisplayName(context.Request.Method, routePattern); activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern); } #endif activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); - /* - #if !NETSTANDARD2_0 - - if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod)) - { - this.AddGrpcAttributes(activity, grpcMethod, context); - } - */ + + if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod)) + { + AddGrpcAttributes(activity, grpcMethod, context); + } + if (activity.Status == ActivityStatusCode.Unset) { activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, response.StatusCode)); @@ -327,27 +321,15 @@ static bool TryFetchException(object payload, out Exception exc) => ExceptionPropertyFetcher.TryFetch(payload, out exc) && exc != null; } -#if !NETSTANDARD2_0 [MethodImpl(MethodImplOptions.AggressiveInlining)] private static bool TryGetGrpcMethod(Activity activity, out string grpcMethod) { grpcMethod = GrpcTagHelper.GetGrpcMethodFromActivity(activity); return !string.IsNullOrEmpty(grpcMethod); } -#endif - - private string GetDisplayName(string httpMethod, string httpRoute = null) - { - var normalizedMethod = RequestMethodHelper.GetNormalizedHttpMethod(httpMethod); - - return string.IsNullOrEmpty(httpRoute) - ? normalizedMethod - : $"{normalizedMethod} {httpRoute}"; - } -#if !NETSTANDARD2_0 [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context) + private static void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context) { // The RPC semantic conventions indicate the span name // should not have a leading forward slash. @@ -389,5 +371,13 @@ private void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext } } } -#endif + + private static string GetDisplayName(string httpMethod, string httpRoute = null) + { + var normalizedMethod = RequestMethodHelper.GetNormalizedHttpMethod(httpMethod); + + return string.IsNullOrEmpty(httpRoute) + ? normalizedMethod + : $"{normalizedMethod} {httpRoute}"; + } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj b/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj index e3782aa277f..83b9e4e25bc 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj @@ -11,11 +11,13 @@ + + @@ -28,10 +30,11 @@ - + + diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index 38919cb1156..5bf6a77ded7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -62,6 +62,8 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( { services.Configure(name, configureAspNetCoreTraceInstrumentationOptions); } + + services.RegisterOptionsFactory(configuration => new AspNetCoreTraceInstrumentationOptions(configuration)); }); if (builder is IDeferredTracerProviderBuilder deferredTracerProviderBuilder) diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs index 6b16c081d13..9ef4eaf9e79 100644 --- a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs +++ b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs @@ -3,8 +3,19 @@ #if NET6_0_OR_GREATER using System.Diagnostics; +using System.Net; +using Greet; +using Grpc.Core; +using Grpc.Net.Client; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using OpenTelemetry.Context.Propagation; using OpenTelemetry.Instrumentation.Grpc.Services.Tests; +using OpenTelemetry.Instrumentation.GrpcNetClient; +using OpenTelemetry.Trace; using Xunit; +using static OpenTelemetry.Internal.HttpSemanticConventionHelper; +using Status = OpenTelemetry.Trace.Status; namespace OpenTelemetry.Instrumentation.Grpc.Tests; @@ -21,34 +32,26 @@ public GrpcTests() this.server = new GrpcServer(); } - /* [Theory] [InlineData(null)] - [InlineData(true)] - [InlineData(false)] - public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(bool? enableGrpcAspNetCoreSupport) + [InlineData("true")] + [InlineData("false")] + [InlineData("True")] + [InlineData("False")] + public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(string enableGrpcAspNetCoreSupport) { var configuration = new ConfigurationBuilder() - .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http" }) + .AddInMemoryCollection(new Dictionary + { + [SemanticConventionOptInKeyName] = "http", + ["OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION"] = enableGrpcAspNetCoreSupport, + }) .Build(); var exportedItems = new List(); - var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder() - .ConfigureServices(services => services.AddSingleton(configuration)); - - if (enableGrpcAspNetCoreSupport.HasValue) - { - tracerProviderBuilder.AddAspNetCoreInstrumentation(options => - { - options.EnableGrpcAspNetCoreSupport = enableGrpcAspNetCoreSupport.Value; - }); - } - else - { - tracerProviderBuilder.AddAspNetCoreInstrumentation(); - } - - using var tracerProvider = tracerProviderBuilder + using var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddAspNetCoreInstrumentation() .AddInMemoryExporter(exportedItems) .Build(); @@ -66,7 +69,7 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(bool? enableGrpcA Assert.Equal(ActivityKind.Server, activity.Kind); - if (!enableGrpcAspNetCoreSupport.HasValue || enableGrpcAspNetCoreSupport.Value) + if (enableGrpcAspNetCoreSupport != null && enableGrpcAspNetCoreSupport.Equals("true", StringComparison.OrdinalIgnoreCase)) { Assert.Equal("grpc", activity.GetTagValue(SemanticConventions.AttributeRpcSystem)); Assert.Equal("greet.Greeter", activity.GetTagValue(SemanticConventions.AttributeRpcService)); @@ -99,30 +102,28 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(bool? enableGrpcA [Theory(Skip = "Skipping for .NET 6 and higher due to bug #3023")] #endif [InlineData(null)] - [InlineData(true)] - [InlineData(false)] - public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewActivity(bool? enableGrpcAspNetCoreSupport) + [InlineData("true")] + [InlineData("false")] + [InlineData("True")] + [InlineData("False")] + public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewActivity(string enableGrpcAspNetCoreSupport) { try { // B3Propagator along with the headers passed to the client.SayHello ensure that the instrumentation creates a sibling activity Sdk.SetDefaultTextMapPropagator(new Extensions.Propagators.B3Propagator()); var exportedItems = new List(); - var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder(); - - if (enableGrpcAspNetCoreSupport.HasValue) - { - tracerProviderBuilder.AddAspNetCoreInstrumentation(options => - { - options.EnableGrpcAspNetCoreSupport = enableGrpcAspNetCoreSupport.Value; - }); - } - else - { - tracerProviderBuilder.AddAspNetCoreInstrumentation(); - } - - using var tracerProvider = tracerProviderBuilder + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + [SemanticConventionOptInKeyName] = "http", + ["OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION"] = enableGrpcAspNetCoreSupport, + }) + .Build(); + + using var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddAspNetCoreInstrumentation() .AddInMemoryExporter(exportedItems) .Build(); @@ -146,7 +147,7 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewAc Assert.Equal(ActivityKind.Server, activity.Kind); - if (!enableGrpcAspNetCoreSupport.HasValue || enableGrpcAspNetCoreSupport.Value) + if (enableGrpcAspNetCoreSupport != null && enableGrpcAspNetCoreSupport.Equals("true", StringComparison.OrdinalIgnoreCase)) { Assert.Equal("grpc", activity.GetTagValue(SemanticConventions.AttributeRpcSystem)); Assert.Equal("greet.Greeter", activity.GetTagValue(SemanticConventions.AttributeRpcService)); @@ -183,7 +184,7 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewAc })); } } - */ + public void Dispose() { this.server.Dispose();