Code review changes

src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetricsInstrumentationOptions.cs

@@ -15,7 +15,6 @@
 // </copyright>
 
 using System;
-using System.Collections.Generic;
 using System.Diagnostics;
 using Microsoft.AspNetCore.Http;
 
@@ -26,6 +25,13 @@ namespace OpenTelemetry.Instrumentation.AspNetCore
     /// </summary>
     public class AspNetCoreMetricsInstrumentationOptions
     {
+        /// <summary>
+        /// Delegate for enrichment of recorded metric with additional tags.
+        /// </summary>
+        /// <param name="context"><see cref="HttpContext"/>: the HttpContext object. Both Request and Response are available.</param>
+        /// <param name="tags"><see cref="TagList"/>: List of tags to add. </param>
+        public delegate void AspNetCoreMetricEnrichmentFunc(HttpContext context, out TagList tags);
+
         /// <summary>
         /// Gets or sets a Filter function that determines whether or not to collect telemetry about requests on a per request basis.
         /// The Filter gets the HttpContext, and should return a boolean.
@@ -37,9 +43,6 @@ public class AspNetCoreMetricsInstrumentationOptions
         /// <summary>
         /// Gets or sets an function to enrich an recorded metric with additional custom tags.
         /// </summary>
-        /// <remarks>
-        /// <para><see cref="HttpContext"/>: the HttpContext object.</para>
-        /// </remarks>
-        public Func<HttpContext, IEnumerable<KeyValuePair<string, object>>> EnrichWithCustomTags { get; set; }
+        public AspNetCoreMetricEnrichmentFunc EnrichWithCustomTags { get; set; }
     }
 }

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs

@@ -54,9 +54,17 @@ public override void OnEventWritten(string name, object payload)
                     return;
                 }
 
-                if (this.options.Filter?.Invoke(context) == false)
+                try
                 {
-                    AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(Activity.Current.OperationName);
+                    if (this.options.Filter?.Invoke(context) == false)
+                    {
+                        AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(Activity.Current.OperationName);
+                        return;
+                    }
+                }
+                catch (Exception ex)
+                {
+                    AspNetCoreInstrumentationEventSource.Log.RequestFilterException(ex);
                     return;
                 }
 
@@ -93,11 +101,18 @@ public override void OnEventWritten(string name, object payload)
 #endif
                 if (this.options.EnrichWithCustomTags != null)
                 {
-                    var enrichedTags = this.options.EnrichWithCustomTags(context);
+                    try
+                    {
+                        this.options.EnrichWithCustomTags(context, out var enrichedTags);
 
-                    foreach (var keyValuePair in enrichedTags)
+                        foreach (var keyValuePair in enrichedTags)
+                        {
+                            tags.Add(keyValuePair);
+                        }
+                    }
+                    catch (Exception ex)
                     {
-                        tags.Add(keyValuePair);
+                        AspNetCoreInstrumentationEventSource.Log.EnrichmentException(ex);
                     }
                 }
 

src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs

@@ -72,9 +72,7 @@ public static MeterProviderBuilder AddAspNetCoreInstrumentation(
             {
                 var options = sp.GetRequiredService<IOptionsMonitor<AspNetCoreMetricsInstrumentationOptions>>().Get(name);
 
-                // TODO: Implement an IDeferredMeterProviderBuilder
-
-                // TODO: Add AspNetCoreMetricsInstrumentationOptions ?
+                // 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
 

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs

@@ -16,9 +16,11 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Hosting;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.Testing;
 using Microsoft.AspNetCore.TestHost;
 using Microsoft.Extensions.DependencyInjection;
@@ -141,18 +143,21 @@ void ConfigureTestServices(IServiceCollection services)
         [Fact]
         public async Task MetricEnrichedWithCustomTags()
         {
-            var tagsToAdd = new List<KeyValuePair<string, object>>()
+            var tagsToAdd = new KeyValuePair<string, object>[]
             {
                 new("custom_tag_1", 1),
-                new("custom_tag_2", "one")
+                new("custom_tag_2", "one"),
             };
 
             var metricItems = new List<Metric>();
 
             void ConfigureTestServices(IServiceCollection services)
             {
                 this.meterProvider = Sdk.CreateMeterProviderBuilder()
-                    .AddAspNetCoreInstrumentation(opt => opt.EnrichWithCustomTags = (ctx) => tagsToAdd)
+                    .AddAspNetCoreInstrumentation(opt => opt.EnrichWithCustomTags = (HttpContext _, out TagList tags) =>
+                    {
+                        tags = new TagList(new Span<KeyValuePair<string, object>>(tagsToAdd));
+                    })
                     .AddInMemoryExporter(metricItems)
                     .Build();
             }