From ac56c9f0df9743841ad9f14de0a8e2ee183972c3 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 7 Dec 2023 10:53:37 -0800 Subject: [PATCH 1/4] Update documentation comments for `OpenTelemetryDependencyInjectionMeterProviderBuilderExtensions.ConfigureServices` (#5139) --- .../Metrics/IMeterProviderBuilder.cs | 4 ++-- ...emetryDependencyInjectionMeterProviderBuilderExtensions.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/IMeterProviderBuilder.cs b/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/IMeterProviderBuilder.cs index e52858935f..a7f2bf609f 100644 --- a/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/IMeterProviderBuilder.cs +++ b/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/IMeterProviderBuilder.cs @@ -23,10 +23,10 @@ internal interface IMeterProviderBuilder : IDeferredMeterProviderBuilder /// /// Register a callback action to configure the where metric services are configured. + /// cref="IServiceCollection"/> where metrics services are configured. /// /// - /// Note: Metric services are only available during the application + /// Note: Metrics services are only available during the application /// configuration phase. This method should throw a if services are configured after the /// application has been created. diff --git a/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/OpenTelemetryDependencyInjectionMeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/OpenTelemetryDependencyInjectionMeterProviderBuilderExtensions.cs index 04ac51efe6..457ddc8766 100644 --- a/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/OpenTelemetryDependencyInjectionMeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/OpenTelemetryDependencyInjectionMeterProviderBuilderExtensions.cs @@ -112,10 +112,10 @@ public static MeterProviderBuilder AddInstrumentation( /// /// Register a callback action to configure the where tracing services are configured. + /// cref="IServiceCollection"/> where metrics services are configured. /// /// - /// Note: Tracing services are only available during the application + /// Note: Metrics services are only available during the application /// configuration phase. /// /// . From ff3956350efc39ada6ba5c246272ff8fc0e92d29 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 7 Dec 2023 11:06:27 -0800 Subject: [PATCH 2/4] Bump actions/stale from 8 to 9 (#5142) --- .github/workflows/stale.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index 95287991b1..f5657aa9f5 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -10,7 +10,7 @@ jobs: stale: runs-on: ubuntu-latest steps: - - uses: actions/stale@v8 + - uses: actions/stale@v9 with: stale-pr-message: 'This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.' close-pr-message: 'Closed as inactive. Feel free to reopen if this PR is still being worked on.' From 58e8f24782dc014ec82c03658b335f60583b4857 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Thu, 7 Dec 2023 14:58:57 -0800 Subject: [PATCH 3/4] 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 9c6f210a71..f66a5e0434 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 d6dc29b65c..5db9434309 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 67a79c50eb..471b131fc1 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 e3782aa277..83b9e4e25b 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 38919cb115..5bf6a77ded 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 6b16c081d1..9ef4eaf9e7 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(); From 93fd7d34a718278abfa533fa3e9436eb205cab88 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Thu, 7 Dec 2023 17:57:16 -0800 Subject: [PATCH 4/4] Doc fix for RecordException (#5143) --- src/OpenTelemetry.Api/Trace/ActivityExtensions.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs b/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs index 036d8c11a7..d8ac769b9c 100644 --- a/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs +++ b/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs @@ -57,20 +57,26 @@ public static Status GetStatus(this Activity activity) } /// - /// Adds an activity event containing information from the specified exception. + /// Adds an containing information from the specified exception. /// /// Activity instance. /// Exception to be recorded. + /// The exception is recorded as per specification. + /// "exception.stacktrace" is represented using the value of Exception.ToString. + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static void RecordException(this Activity activity, Exception? ex) => RecordException(activity, ex, default); /// - /// Adds an activity event containing information from the specified exception and additional tags. + /// Adds an containing information from the specified exception and additional tags. /// /// Activity instance. /// Exception to be recorded. /// Additional tags to record on the event. + /// The exception is recorded as per specification. + /// "exception.stacktrace" is represented using the value of Exception.ToString. + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static void RecordException(this Activity activity, Exception? ex, in TagList tags) {