Remove Enrich and Filter for ASP.NET Core metrics (#4981)

Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]> Co-authored-by: Mikel Blanchard <[email protected]>
open-telemetry · Oct 23, 2023 · faa70be · faa70be
1 parent d07d030
commit faa70be
Show file tree

Hide file tree

Showing 8 changed files with 28 additions and 287 deletions.
diff --git 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<string, Microsoft.AspNetCore.Http.HttpContext, bool>
-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<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions> configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Metrics.MeterProviderBuilder
-static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions> 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<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Trace.TracerProviderBuilder
 static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> 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
@@ -15,7 +15,6 @@
 // </copyright>
 
 using System.Diagnostics;
-using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Configuration;
 using static OpenTelemetry.Internal.HttpSemanticConventionHelper;
 
@@ -24,7 +23,7 @@ namespace OpenTelemetry.Instrumentation.AspNetCore;
 /// <summary>
 /// Options for metrics requests instrumentation.
 /// </summary>
-public class AspNetCoreMetricsInstrumentationOptions
+internal sealed class AspNetCoreMetricsInstrumentationOptions
 {
     internal readonly HttpSemanticConvention HttpSemanticConvention;
 
@@ -42,38 +41,5 @@ internal AspNetCoreMetricsInstrumentationOptions(IConfiguration configuration)
 
         this.HttpSemanticConvention = GetSemanticConventionOptIn(configuration);
     }
-
-    /// <summary>
-    /// Delegate for enrichment of recorded metric with additional tags.
-    /// </summary>
-    /// <param name="name">The name of the metric being enriched.</param>
-    /// <param name="context"><see cref="HttpContext"/>: the HttpContext object. Both Request and Response are available.</param>
-    /// <param name="tags"><see cref="TagList"/>: List of current tags. You can add additional tags to this list. </param>
-    public delegate void AspNetCoreMetricEnrichmentFunc(string name, HttpContext context, ref TagList tags);
-
-    /// <summary>
-    /// Gets or sets a filter function that determines whether or not to
-    /// collect telemetry on a per request basis.
-    /// </summary>
-    /// <remarks>
-    /// Notes:
-    /// <list type="bullet">
-    /// <item>The first parameter is the name of the metric being
-    /// filtered.</item>
-    /// <item>The return value for the filter function is interpreted as:
-    /// <list type="bullet">
-    /// <item>If filter returns <see langword="true" />, the request is
-    /// collected.</item>
-    /// <item>If filter returns <see langword="false" /> or throws an
-    /// exception the request is NOT collected.</item>
-    /// </list></item>
-    /// </list>
-    /// </remarks>
-    public Func<string, HttpContext, bool> Filter { get; set; }
-
-    /// <summary>
-    /// Gets or sets an function to enrich a recorded metric with additional custom tags.
-    /// </summary>
-    public AspNetCoreMetricEnrichmentFunc Enrich { get; set; }
 }
 
diff --git 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/...lemetry.Instrumentation.AspNetCore/Implementation/AspNetCoreInstrumentationEventSource.cs b/...lemetry.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
@@ -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<string, object>(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<string, object>(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
@@ -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,65 +42,23 @@ public static MeterProviderBuilder AddAspNetCoreInstrumentation(
 #if NET8_0_OR_GREATER
         return builder.ConfigureMeters();
 #else
-        return AddAspNetCoreInstrumentation(builder, name: null, configureAspNetCoreInstrumentationOptions: null);
-#endif
-    }
-
-    /// <summary>
-    /// Enables the incoming requests automatic data collection for ASP.NET Core.
-    /// </summary>
-    /// <param name="builder"><see cref="MeterProviderBuilder"/> being configured.</param>
-    /// <param name="configureAspNetCoreInstrumentationOptions">Callback action for configuring <see cref="AspNetCoreMetricsInstrumentationOptions"/>.</param>
-    /// <returns>The instance of <see cref="MeterProviderBuilder"/> to chain the calls.</returns>
-    public static MeterProviderBuilder AddAspNetCoreInstrumentation(
-        this MeterProviderBuilder builder,
-        Action<AspNetCoreMetricsInstrumentationOptions> configureAspNetCoreInstrumentationOptions)
-        => AddAspNetCoreInstrumentation(builder, name: null, configureAspNetCoreInstrumentationOptions);
-
-    /// <summary>
-    /// Enables the incoming requests automatic data collection for ASP.NET Core.
-    /// </summary>
-    /// <param name="builder"><see cref="MeterProviderBuilder"/> being configured.</param>
-    /// <param name="name">Name which is used when retrieving options.</param>
-    /// <param name="configureAspNetCoreInstrumentationOptions">Callback action for configuring <see cref="AspNetCoreMetricsInstrumentationOptions"/>.</param>
-    /// <returns>The instance of <see cref="MeterProviderBuilder"/> to chain the calls.</returns>
-    public static MeterProviderBuilder AddAspNetCoreInstrumentation(
-        this MeterProviderBuilder builder,
-        string name,
-        Action<AspNetCoreMetricsInstrumentationOptions> 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));
         });
 
         builder.AddMeter(AspNetCoreMetrics.InstrumentationName);
 
         builder.AddInstrumentation(sp =>
         {
-            var options = sp.GetRequiredService<IOptionsMonitor<AspNetCoreMetricsInstrumentationOptions>>().Get(name);
+            var options = sp.GetRequiredService<IOptionsMonitor<AspNetCoreMetricsInstrumentationOptions>>().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
@@ -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<AspNetCoreMetricsInstrumentationOptions>(name, options =>
-            {
-                optionsPickedFromDI = true;
-            });
-        }
-
-        // Arrange
-        using (var client = this.factory
-            .WithWebHostBuilder(builder =>
-                builder.ConfigureTestServices(ConfigureTestServices))
-            .CreateClient())
-        {
-        }
-
-        Assert.True(optionsPickedFromDI);
-    }
-#endif
 }