From faa70be1b3ac75617d450d7f34e8b477ad9ed450 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Mon, 23 Oct 2023 15:44:15 -0700 Subject: [PATCH] Remove Enrich and Filter for ASP.NET Core metrics (#4981) Co-authored-by: Cijo Thomas Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Co-authored-by: Mikel Blanchard --- .../.publicApi/PublicAPI.Unshipped.txt | 10 -- ...AspNetCoreMetricsInstrumentationOptions.cs | 36 +----- .../CHANGELOG.md | 23 ++++ .../AspNetCoreInstrumentationEventSource.cs | 6 - .../Implementation/HttpInMetricsListener.cs | 50 -------- .../MeterProviderBuilderExtensions.cs | 46 +------- .../DependencyInjectionConfigTests.cs | 33 ------ .../MetricTests.cs | 111 +----------------- 8 files changed, 28 insertions(+), 287 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/.publicApi/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.AspNetCore/.publicApi/PublicAPI.Unshipped.txt index 88cc5073da4..b3b24c45be9 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/.publicApi/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/.publicApi/PublicAPI.Unshipped.txt @@ -12,19 +12,9 @@ OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.Filter OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.Filter.set -> void OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.RecordException.get -> bool OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.RecordException.set -> void -OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions -OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.AspNetCoreMetricEnrichmentFunc -OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.AspNetCoreMetricsInstrumentationOptions() -> void -OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.Enrich.get -> OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.AspNetCoreMetricEnrichmentFunc -OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.Enrich.set -> void -OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.Filter.get -> System.Func -OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.Filter.set -> void OpenTelemetry.Metrics.MeterProviderBuilderExtensions OpenTelemetry.Trace.TracerProviderBuilderExtensions static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder) -> OpenTelemetry.Metrics.MeterProviderBuilder -static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder, string name, System.Action configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Metrics.MeterProviderBuilder -static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Action configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Metrics.MeterProviderBuilder static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder) -> OpenTelemetry.Trace.TracerProviderBuilder static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, string name, System.Action configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Trace.TracerProviderBuilder static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Trace.TracerProviderBuilder -virtual OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.AspNetCoreMetricEnrichmentFunc.Invoke(string name, Microsoft.AspNetCore.Http.HttpContext context, ref System.Diagnostics.TagList tags) -> void diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetricsInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetricsInstrumentationOptions.cs index 0710a9e4fe9..2ec3ff8a92c 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetricsInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetricsInstrumentationOptions.cs @@ -15,7 +15,6 @@ // using System.Diagnostics; -using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Configuration; using static OpenTelemetry.Internal.HttpSemanticConventionHelper; @@ -24,7 +23,7 @@ namespace OpenTelemetry.Instrumentation.AspNetCore; /// /// Options for metrics requests instrumentation. /// -public class AspNetCoreMetricsInstrumentationOptions +internal sealed class AspNetCoreMetricsInstrumentationOptions { internal readonly HttpSemanticConvention HttpSemanticConvention; @@ -42,38 +41,5 @@ internal AspNetCoreMetricsInstrumentationOptions(IConfiguration configuration) this.HttpSemanticConvention = GetSemanticConventionOptIn(configuration); } - - /// - /// Delegate for enrichment of recorded metric with additional tags. - /// - /// The name of the metric being enriched. - /// : the HttpContext object. Both Request and Response are available. - /// : List of current tags. You can add additional tags to this list. - public delegate void AspNetCoreMetricEnrichmentFunc(string name, HttpContext context, ref TagList tags); - - /// - /// Gets or sets a filter function that determines whether or not to - /// collect telemetry on a per request basis. - /// - /// - /// Notes: - /// - /// The first parameter is the name of the metric being - /// filtered. - /// The return value for the filter function is interpreted as: - /// - /// If filter returns , the request is - /// collected. - /// If filter returns or throws an - /// exception the request is NOT collected. - /// - /// - /// - public Func Filter { get; set; } - - /// - /// Gets or sets an function to enrich a recorded metric with additional custom tags. - /// - public AspNetCoreMetricEnrichmentFunc Enrich { get; set; } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 10e074b856c..fbd1a0f3293 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -86,6 +86,29 @@ metric. This change only affects users setting `OTEL_SEMCONV_STABILITY_OPT_IN` to `http` or `http/dup`. ([#4934](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4934)) +* **Breaking**: Removed `Enrich` and `Filter` support for **metrics** + instrumentation. With this change, `AspNetCoreMetricsInstrumentationOptions` + is no longer available. + ([#4981](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4981)) + + * `Enrich` migration: + + An enrichment API for the `http.server.request.duration` metric is available + inside AspNetCore for users targeting .NET 8.0 (or newer). For details see: + [Enrich the ASP.NET Core request + metric](https://learn.microsoft.com/aspnet/core/log-mon/metrics/metrics?view=aspnetcore-8.0#enrich-the-aspnet-core-request-metric). + + * `Filter` migration: + + There is no comparable filter mechanism currently available for any .NET + version. Please [share your + feedback](https://github.com/open-telemetry/opentelemetry-dotnet/issues/4982) + if you are impacted by this feature gap. + + > **Note** + > The [View API](https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/metrics/customizing-the-sdk#select-specific-tags) + may be used to drop dimensions. + ## 1.5.1-beta.1 Released 2023-Jul-20 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/AspNetCoreInstrumentationEventSource.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/AspNetCoreInstrumentationEventSource.cs index f8c846cba9f..3f67896c167 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/AspNetCoreInstrumentationEventSource.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/AspNetCoreInstrumentationEventSource.cs @@ -92,10 +92,4 @@ public void UnknownErrorProcessingEvent(string handlerName, string eventName, st { this.WriteEvent(5, handlerName, eventName, ex); } - - [Event(6, Message = "'{0}' is not supported for .NET8.0 and above targets", Level = EventLevel.Warning)] - public void UnsupportedOption(string optionName) - { - this.WriteEvent(6, optionName); - } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index db7fa2a5272..741c81b475c 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -88,20 +88,6 @@ public void OnEventWritten_Old(string name, object payload) return; } - try - { - if (this.options.Filter?.Invoke(HttpServerDurationMetricName, context) == false) - { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); - return; - } - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); - return; - } - // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. @@ -134,17 +120,6 @@ public void OnEventWritten_Old(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); } #endif - if (this.options.Enrich != null) - { - try - { - this.options.Enrich(HttpServerDurationMetricName, context, ref tags); - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); - } - } // We are relying here on ASP.NET Core to set duration before writing the stop event. // https://github.com/dotnet/aspnetcore/blob/d6fa351048617ae1c8b47493ba1abbe94c3a24cf/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L449 @@ -161,20 +136,6 @@ public void OnEventWritten_New(string name, object payload) return; } - try - { - if (this.options.Filter?.Invoke(HttpServerRequestDurationMetricName, context) == false) - { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); - return; - } - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); - return; - } - // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. @@ -199,17 +160,6 @@ public void OnEventWritten_New(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); } #endif - if (this.options.Enrich != null) - { - try - { - this.options.Enrich(HttpServerRequestDurationMetricName, context, ref tags); - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); - } - } // We are relying here on ASP.NET Core to set duration before writing the stop event. // https://github.com/dotnet/aspnetcore/blob/d6fa351048617ae1c8b47493ba1abbe94c3a24cf/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L449 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs index 80c071a2304..71f00d1b5bf 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs @@ -17,9 +17,9 @@ #if !NET8_0_OR_GREATER using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; -#endif using OpenTelemetry.Instrumentation.AspNetCore; using OpenTelemetry.Instrumentation.AspNetCore.Implementation; +#endif using OpenTelemetry.Internal; namespace OpenTelemetry.Metrics; @@ -42,52 +42,11 @@ public static MeterProviderBuilder AddAspNetCoreInstrumentation( #if NET8_0_OR_GREATER return builder.ConfigureMeters(); #else - return AddAspNetCoreInstrumentation(builder, name: null, configureAspNetCoreInstrumentationOptions: null); -#endif - } - - /// - /// Enables the incoming requests automatic data collection for ASP.NET Core. - /// - /// being configured. - /// Callback action for configuring . - /// The instance of to chain the calls. - public static MeterProviderBuilder AddAspNetCoreInstrumentation( - this MeterProviderBuilder builder, - Action configureAspNetCoreInstrumentationOptions) - => AddAspNetCoreInstrumentation(builder, name: null, configureAspNetCoreInstrumentationOptions); - - /// - /// Enables the incoming requests automatic data collection for ASP.NET Core. - /// - /// being configured. - /// Name which is used when retrieving options. - /// Callback action for configuring . - /// The instance of to chain the calls. - public static MeterProviderBuilder AddAspNetCoreInstrumentation( - this MeterProviderBuilder builder, - string name, - Action configureAspNetCoreInstrumentationOptions) - { - Guard.ThrowIfNull(builder); - -#if NET8_0_OR_GREATER - AspNetCoreInstrumentationEventSource.Log.UnsupportedOption(nameof(AspNetCoreMetricsInstrumentationOptions)); - return builder.ConfigureMeters(); -#else - // Note: Warm-up the status code mapping. _ = TelemetryHelper.BoxedStatusCodes; - name ??= Options.DefaultName; - builder.ConfigureServices(services => { - if (configureAspNetCoreInstrumentationOptions != null) - { - services.Configure(name, configureAspNetCoreInstrumentationOptions); - } - services.RegisterOptionsFactory(configuration => new AspNetCoreMetricsInstrumentationOptions(configuration)); }); @@ -95,12 +54,11 @@ public static MeterProviderBuilder AddAspNetCoreInstrumentation( builder.AddInstrumentation(sp => { - var options = sp.GetRequiredService>().Get(name); + var options = sp.GetRequiredService>().Get(Options.DefaultName); // TODO: Add additional options to AspNetCoreMetricsInstrumentationOptions ? // RecordException - probably doesn't make sense for metric instrumentation // EnableGrpcAspNetCoreSupport - this instrumentation will also need to also handle gRPC requests - return new AspNetCoreMetrics(options); }); diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/DependencyInjectionConfigTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/DependencyInjectionConfigTests.cs index 0d9acad0ca9..ec78facc67e 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/DependencyInjectionConfigTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/DependencyInjectionConfigTests.cs @@ -64,37 +64,4 @@ void ConfigureTestServices(IServiceCollection services) Assert.True(optionsPickedFromDI); } - -#if !NET8_0_OR_GREATER - [Theory] - [InlineData(null)] - [InlineData("CustomName")] - public void TestMetricsOptionsDIConfig(string name) - { - name ??= Options.DefaultName; - - bool optionsPickedFromDI = false; - void ConfigureTestServices(IServiceCollection services) - { - services.AddOpenTelemetry() - .WithMetrics(builder => builder - .AddAspNetCoreInstrumentation(name, configureAspNetCoreInstrumentationOptions: null)); - - services.Configure(name, options => - { - optionsPickedFromDI = true; - }); - } - - // Arrange - using (var client = this.factory - .WithWebHostBuilder(builder => - builder.ConfigureTestServices(ConfigureTestServices)) - .CreateClient()) - { - } - - Assert.True(optionsPickedFromDI); - } -#endif } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 1d15fa4d9d2..ff11b5e1225 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -14,22 +14,18 @@ // limitations under the License. // -#if !NET8_0_OR_GREATER -using System.Diagnostics; -#endif #if NET8_0_OR_GREATER using System.Threading.RateLimiting; using Microsoft.AspNetCore.Builder; #endif using Microsoft.AspNetCore.Hosting; +#if NET8_0_OR_GREATER using Microsoft.AspNetCore.Http; +#endif using Microsoft.AspNetCore.Mvc.Testing; #if NET8_0_OR_GREATER using Microsoft.AspNetCore.RateLimiting; #endif -#if !NET8_0_OR_GREATER -using Microsoft.AspNetCore.TestHost; -#endif using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -360,109 +356,6 @@ public async Task RequestMetricIsCaptured_Dup() expectedRoutes: new List { "api/Values", "api/Values/{id}" }, expectedTagsCount: 6); } - - [Fact] - public async Task MetricNotCollectedWhenFilterIsApplied() - { - var metricItems = new List(); - - void ConfigureTestServices(IServiceCollection services) - { - this.meterProvider = Sdk.CreateMeterProviderBuilder() - .AddAspNetCoreInstrumentation(opt => opt.Filter = (name, ctx) => ctx.Request.Path != "/api/values/2") - .AddInMemoryExporter(metricItems) - .Build(); - } - - using (var client = this.factory - .WithWebHostBuilder(builder => - { - builder.ConfigureTestServices(ConfigureTestServices); - builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); - }) - .CreateClient()) - { - using var response1 = await client.GetAsync("/api/values").ConfigureAwait(false); - using var response2 = await client.GetAsync("/api/values/2").ConfigureAwait(false); - - response1.EnsureSuccessStatusCode(); - response2.EnsureSuccessStatusCode(); - } - - // We need to let End callback execute as it is executed AFTER response was returned. - // In unit tests environment there may be a lot of parallel unit tests executed, so - // giving some breezing room for the End callback to complete - await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); - - this.meterProvider.Dispose(); - - var requestMetrics = metricItems - .Where(item => item.Name == "http.server.duration") - .ToArray(); - - var metric = Assert.Single(requestMetrics); - - // Assert single because we filtered out one route - var metricPoint = Assert.Single(GetMetricPoints(metric)); - AssertMetricPoint_Old(metricPoint); - } - - [Fact] - public async Task MetricEnrichedWithCustomTags() - { - var tagsToAdd = new KeyValuePair[] - { - new("custom_tag_1", 1), - new("custom_tag_2", "one"), - }; - - var metricItems = new List(); - - void ConfigureTestServices(IServiceCollection services) - { - this.meterProvider = Sdk.CreateMeterProviderBuilder() - .AddAspNetCoreInstrumentation(opt => opt.Enrich = (string _, HttpContext _, ref TagList tags) => - { - foreach (var keyValuePair in tagsToAdd) - { - tags.Add(keyValuePair); - } - }) - .AddInMemoryExporter(metricItems) - .Build(); - } - - using (var client = this.factory - .WithWebHostBuilder(builder => - { - builder.ConfigureTestServices(ConfigureTestServices); - builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); - }) - .CreateClient()) - { - using var response = await client.GetAsync("/api/values").ConfigureAwait(false); - response.EnsureSuccessStatusCode(); - } - - // We need to let End callback execute as it is executed AFTER response was returned. - // In unit tests environment there may be a lot of parallel unit tests executed, so - // giving some breezing room for the End callback to complete - await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); - - this.meterProvider.Dispose(); - - var requestMetrics = metricItems - .Where(item => item.Name == "http.server.duration") - .ToArray(); - - var metric = Assert.Single(requestMetrics); - var metricPoint = Assert.Single(GetMetricPoints(metric)); - - var tags = AssertMetricPoint_Old(metricPoint, expectedTagsCount: StandardTagsCount + 2); - - Assert.Contains(tagsToAdd[0], tags); - Assert.Contains(tagsToAdd[1], tags); - } #endif public void Dispose()