Rollback additional metric attributes

open-telemetry · Nov 8, 2023 · 50e1aed · 50e1aed
1 parent 303a580
commit 50e1aed
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 106 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md
@@ -5,17 +5,9 @@
 * Added enrich functionality to metric instrumentation
   ([#1407](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1407)).
 
-  * New overload of `AddAspnetInstrumentation` now accepts a configuration delegate.
+  * New overload of `AddAspNetInstrumentation` now accepts a configuration delegate.
   * The `Enrich` can be used to add additional metric attributes.
 
-* Additional metric attributes will now be emitted for `http.server.duration`
-  ([#1407](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1407)):
-
-  * `net.host.name`
-  * `net.host.port`
-  * `http.flavor`
-  * `http.route`
-
 ## 1.6.0-beta.2
 
 Released 2023-Nov-06

diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs
@@ -17,6 +17,7 @@
 using System;
 using System.Diagnostics;
 using System.Web;
+using System.Web.Routing;
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Internal;
 using OpenTelemetry.Trace;
@@ -25,7 +26,8 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
 
 internal sealed class HttpInListener : IDisposable
 {
-    private readonly HttpRequestRouteHelper routeHelper = new();
+    private readonly PropertyFetcher<object> routeFetcher = new("Route");
+    private readonly PropertyFetcher<string> routeTemplateFetcher = new("RouteTemplate");
     private readonly AspNetInstrumentationOptions options;
 
     public HttpInListener(AspNetInstrumentationOptions options)
@@ -131,7 +133,27 @@ private void OnStopActivity(Activity activity, HttpContext context)
                 activity.SetStatus(SpanHelper.ResolveActivityStatusForHttpStatusCode(activity.Kind, response.StatusCode));
             }
 
-            var template = this.routeHelper.GetRouteTemplate(context.Request);
+            var routeData = context.Request.RequestContext.RouteData;
+
+            string? template = null;
+            if (routeData.Values.TryGetValue("MS_SubRoutes", out object msSubRoutes))
+            {
+                // WebAPI attribute routing flows here. Use reflection to not take a dependency on microsoft.aspnet.webapi.core\[version]\lib\[framework]\System.Web.Http.
+
+                if (msSubRoutes is Array attributeRouting && attributeRouting.Length == 1)
+                {
+                    var subRouteData = attributeRouting.GetValue(0);
+
+                    _ = this.routeFetcher.TryFetch(subRouteData, out var route);
+                    _ = this.routeTemplateFetcher.TryFetch(route, out template);
+                }
+            }
+            else if (routeData.Route is Route route)
+            {
+                // MVC + WebAPI traditional routing & MVC attribute routing flow here.
+                template = route.Url;
+            }
+
             if (!string.IsNullOrEmpty(template))
             {
                 // Override the name that was previously set to the path part of URL.

diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInMetricsListener.cs
@@ -24,15 +24,12 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
 
 internal sealed class HttpInMetricsListener : IDisposable
 {
-    internal const string HttpServerDurationMetricName = "http.server.duration";
-
-    private readonly HttpRequestRouteHelper routeHelper = new();
     private readonly Histogram<double> httpServerDuration;
     private readonly AspNetMetricsInstrumentationOptions options;
 
     public HttpInMetricsListener(Meter meter, AspNetMetricsInstrumentationOptions options)
     {
-        this.httpServerDuration = meter.CreateHistogram<double>(HttpServerDurationMetricName, "ms", "Measures the duration of inbound HTTP requests.");
+        this.httpServerDuration = meter.CreateHistogram<double>("http.server.duration", "ms", "Measures the duration of inbound HTTP requests.");
         TelemetryHttpModule.Options.OnRequestStoppedCallback += this.OnStopActivity;
         this.options = options;
     }
@@ -42,38 +39,17 @@ public void Dispose()
         TelemetryHttpModule.Options.OnRequestStoppedCallback -= this.OnStopActivity;
     }
 
-    private static string GetHttpProtocolVersion(HttpRequest request)
-    {
-        var protocol = request.ServerVariables["SERVER_PROTOCOL"];
-        return protocol switch
-        {
-            "HTTP/1.1" => "1.1",
-            "HTTP/2" => "2",
-            "HTTP/3" => "3",
-            _ => protocol,
-        };
-    }
-
     private void OnStopActivity(Activity activity, HttpContext context)
     {
-        var request = context.Request;
-        var url = request.Url;
+        // TODO: This is just a minimal set of attributes. See the spec for additional attributes:
+        // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server
         var tags = new TagList
         {
-            { SemanticConventions.AttributeNetHostName, url.Host },
-            { SemanticConventions.AttributeNetHostPort, url.Port },
-            { SemanticConventions.AttributeHttpFlavor, GetHttpProtocolVersion(request) },
-            { SemanticConventions.AttributeHttpMethod, request.HttpMethod },
-            { SemanticConventions.AttributeHttpScheme, url.Scheme },
+            { SemanticConventions.AttributeHttpMethod, context.Request.HttpMethod },
+            { SemanticConventions.AttributeHttpScheme, context.Request.Url.Scheme },
             { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode },
         };
 
-        var template = this.routeHelper.GetRouteTemplate(request);
-        if (!string.IsNullOrEmpty(template))
-        {
-            tags.Add(SemanticConventions.AttributeHttpRoute, template);
-        }
-
         if (this.options.Enrich is not null)
         {
             try

diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpRequestRouteHelper.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpRequestRouteHelper.cs
diff --git a/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInMetricsListenerTests.cs b/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInMetricsListenerTests.cs
@@ -115,7 +115,7 @@ public void AspNetMetricTagsAreCollectedSuccessfully(
         Assert.Equal(duration, sum);
         Assert.True(duration > 0, "Metric duration should be set.");
 
-        var expectedTagCount = string.IsNullOrEmpty(expectedRoute) ? 6 : 7;
+        var expectedTagCount = 3;
         if (enrichMode == "enrich")
         {
             expectedTagCount++;
@@ -134,11 +134,8 @@ public void AspNetMetricTagsAreCollectedSuccessfully(
         }
 
         ExpectTag("GET", SemanticConventions.AttributeHttpMethod);
-        ExpectTag(200, SemanticConventions.AttributeHttpStatusCode); // TODO: Correct?
+        ExpectTag(200, SemanticConventions.AttributeHttpStatusCode);
         ExpectTag(expectedScheme, SemanticConventions.AttributeHttpScheme);
-        ExpectTag(expectedRoute, SemanticConventions.AttributeHttpRoute);
-        ExpectTag(expectedHost, SemanticConventions.AttributeNetHostName);
-        ExpectTag(expectedPort, SemanticConventions.AttributeNetHostPort);
 
         void ExpectTag<T>(T? expected, string tagName)
         {