Fix Activity http.route and name

src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs

@@ -27,7 +27,6 @@ internal sealed class AspNetCoreInstrumentation : IDisposable
         "Microsoft.AspNetCore.Hosting.HttpRequestIn",
         "Microsoft.AspNetCore.Hosting.HttpRequestIn.Start",
         "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop",
-        "Microsoft.AspNetCore.Mvc.BeforeAction",
         "Microsoft.AspNetCore.Diagnostics.UnhandledException",
         "Microsoft.AspNetCore.Hosting.UnhandledException",
     };

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -41,6 +41,17 @@ exception. The attribute value will be set to full name of exception type.
   path.
   ([#5044](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5044))
 
+* Fixes the `http.route` attribute for scenarios in which it was
+  previously missing or incorrect. Additionally, the `http.route` attribute
+  is now the same for both the metric and `Activity` emitted for a request.
+  Lastly, the `Activity.DisplayName` has been adjusted to have the format
+  `{http.request.method} {http.route}` to conform with [the specification](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#name).
+  There remain scenarios when using conventional routing or Razor pages where
+  `http.route` is still incorrect. See [#5056](https://github.com/open-telemetry/opentelemetry-dotnet/issues/5056)
+  and [#5057](https://github.com/open-telemetry/opentelemetry-dotnet/issues/5057)
+  for more details.
+  ([#5026](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5026))
+
 * Removed `network.protocol.name` from `http.server.request.duration` metric as
   per spec.
   ([#5049](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5049))

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

@@ -23,8 +23,8 @@
 using System.Runtime.CompilerServices;
 #endif
 using Microsoft.AspNetCore.Http;
-#if NET6_0_OR_GREATER
-using Microsoft.AspNetCore.Mvc.Diagnostics;
+#if !NETSTANDARD
+using Microsoft.AspNetCore.Routing;
 #endif
 using OpenTelemetry.Context.Propagation;
 #if !NETSTANDARD2_0
@@ -41,7 +41,6 @@ internal class HttpInListener : ListenerHandler
     internal const string ActivityOperationName = "Microsoft.AspNetCore.Hosting.HttpRequestIn";
     internal const string OnStartEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Start";
     internal const string OnStopEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop";
-    internal const string OnMvcBeforeActionEvent = "Microsoft.AspNetCore.Mvc.BeforeAction";
     internal const string OnUnhandledHostingExceptionEvent = "Microsoft.AspNetCore.Hosting.UnhandledException";
     internal const string OnUnHandledDiagnosticsExceptionEvent = "Microsoft.AspNetCore.Diagnostics.UnhandledException";
 
@@ -97,12 +96,6 @@ public override void OnEventWritten(string name, object payload)
                     this.OnStopActivity(Activity.Current, payload);
                 }
 
-                break;
-            case OnMvcBeforeActionEvent:
-                {
-                    this.OnMvcBeforeAction(Activity.Current, payload);
-                }
-
                 break;
             case OnUnhandledHostingExceptionEvent:
             case OnUnHandledDiagnosticsExceptionEvent:
@@ -202,7 +195,7 @@ public void OnStartActivity(Activity activity, object payload)
 #endif
 
             var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
-            activity.DisplayName = path;
+            activity.DisplayName = this.GetDisplayName(request.Method);
 
             // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md
             if (this.emitOldAttributes)
@@ -252,17 +245,7 @@ public void OnStartActivity(Activity activity, object payload)
                     activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value);
                 }
 
-                if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method, out var httpMethod))
-                {
-                    activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, httpMethod);
-                }
-                else
-                {
-                    // Set to default "_OTHER" as per spec.
-                    // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes
-                    activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, "_OTHER");
-                    activity.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, request.Method);
-                }
+                RequestMethodHelper.SetHttpMethodTag(activity, request.Method);
 
                 activity.SetTag(SemanticConventions.AttributeUrlScheme, request.Scheme);
                 activity.SetTag(SemanticConventions.AttributeUrlPath, path);
@@ -302,6 +285,15 @@ public void OnStopActivity(Activity activity, object payload)
 
             var response = context.Response;
 
+#if !NETSTANDARD
+            var routePattern = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
+            if (!string.IsNullOrEmpty(routePattern))
+            {
+                activity.DisplayName = this.GetDisplayName(context.Request.Method, routePattern);
+                activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern);
+            }
+#endif
+
             if (this.emitOldAttributes)
             {
                 activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode));
@@ -363,57 +355,6 @@ public void OnStopActivity(Activity activity, object payload)
         }
     }
 
-    public void OnMvcBeforeAction(Activity activity, object payload)
-    {
-        // We cannot rely on Activity.Current here
-        // There could be activities started by middleware
-        // after activity started by framework resulting in different Activity.Current.
-        // so, we need to first find the activity started by Asp.Net Core.
-        // For .net6.0 onwards we could use IHttpActivityFeature to get the activity created by framework
-        // var httpActivityFeature = context.Features.Get<IHttpActivityFeature>();
-        // activity = httpActivityFeature.Activity;
-        // However, this will not work as in case of custom propagator
-        // we start a new activity during onStart event which is a sibling to the activity created by framework
-        // So, in that case we need to get the activity created by us here.
-        // we can do so only by looping through activity.Parent chain.
-        while (activity != null)
-        {
-            if (string.Equals(activity.OperationName, ActivityOperationName, StringComparison.Ordinal))
-            {
-                break;
-            }
-
-            activity = activity.Parent;
-        }
-
-        if (activity == null)
-        {
-            return;
-        }
-
-        if (activity.IsAllDataRequested)
-        {
-#if !NET6_0_OR_GREATER
-            _ = this.beforeActionActionDescriptorFetcher.TryFetch(payload, out var actionDescriptor);
-            _ = this.beforeActionAttributeRouteInfoFetcher.TryFetch(actionDescriptor, out var attributeRouteInfo);
-            _ = this.beforeActionTemplateFetcher.TryFetch(attributeRouteInfo, out var template);
-#else
-            var beforeActionEventData = payload as BeforeActionEventData;
-            var template = beforeActionEventData.ActionDescriptor?.AttributeRouteInfo?.Template;
-#endif
-            if (!string.IsNullOrEmpty(template))
-            {
-                // override the span name that was previously set to the path part of URL.
-                activity.DisplayName = template;
-                activity.SetTag(SemanticConventions.AttributeHttpRoute, template);
-            }
-
-            // TODO: Should we get values from RouteData?
-            // private readonly PropertyFetcher beforeActionRouteDataFetcher = new PropertyFetcher("routeData");
-            // var routeData = this.beforeActionRouteDataFetcher.Fetch(payload) as RouteData;
-        }
-    }
-
     public void OnException(Activity activity, object payload)
     {
         if (activity.IsAllDataRequested)
@@ -509,7 +450,20 @@ 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 = this.emitNewAttributes
+            ? RequestMethodHelper.GetNormalizedHttpMethod(httpMethod)
+            : httpMethod;
+
+        return string.IsNullOrEmpty(httpRoute)
+            ? normalizedMethod
+            : $"{normalizedMethod} {httpRoute}";
+    }
 
+#if !NETSTANDARD2_0
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     private void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context)
     {

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

@@ -191,16 +191,9 @@ public void OnEventWritten_New(string name, object payload)
         tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol)));
         tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, context.Request.Scheme));
         tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode)));
-        if (RequestMethodHelper.KnownMethods.TryGetValue(context.Request.Method, out var httpMethod))
-        {
-            tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, httpMethod));
-        }
-        else
-        {
-            // Set to default "_OTHER" as per spec.
-            // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes
-            tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, "_OTHER"));
-        }
+
+        var httpMethod = RequestMethodHelper.GetNormalizedHttpMethod(context.Request.Method);
+        tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, httpMethod));
 
 #if NET6_0_OR_GREATER
         var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;

src/Shared/RequestMethodHelper.cs

@@ -17,11 +17,17 @@
 #if NET8_0_OR_GREATER
 using System.Collections.Frozen;
 #endif
+using System.Diagnostics;
+using OpenTelemetry.Trace;
 
 namespace OpenTelemetry.Internal;
 
 internal static class RequestMethodHelper
 {
+    // The value "_OTHER" is used for non-standard HTTP methods.
+    // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes
+    public const string OtherHttpMethod = "_OTHER";
+
 #if NET8_0_OR_GREATER
     internal static readonly FrozenDictionary<string, string> KnownMethods;
 #else
@@ -50,4 +56,24 @@ static RequestMethodHelper()
         KnownMethods = knownMethodSet;
 #endif
     }
+
+    public static string GetNormalizedHttpMethod(string method)
+    {
+        return KnownMethods.TryGetValue(method, out var normalizedMethod)
+            ? normalizedMethod
+            : OtherHttpMethod;
+    }
+
+    public static void SetHttpMethodTag(Activity activity, string method)
+    {
+        if (KnownMethods.TryGetValue(method, out var normalizedMethod))
+        {
+            activity?.SetTag(SemanticConventions.AttributeHttpRequestMethod, normalizedMethod);
+        }
+        else
+        {
+            activity?.SetTag(SemanticConventions.AttributeHttpRequestMethod, OtherHttpMethod);
+            activity?.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, method);
+        }
+    }
 }

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

@@ -189,7 +189,6 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
         var activity = exportedItems[0];
 
         Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", activity.OperationName);
-        Assert.Equal("api/Values/{id}", activity.DisplayName);
 
         Assert.Equal(expectedTraceId, activity.Context.TraceId);
         Assert.Equal(expectedSpanId, activity.ParentSpanId);
@@ -251,7 +250,6 @@ public async Task CustomPropagator(bool addSampler)
             var activity = exportedItems[0];
 
             Assert.True(activity.Duration != TimeSpan.Zero);
-            Assert.Equal("api/Values/{id}", activity.DisplayName);
 
             Assert.Equal(expectedTraceId, activity.Context.TraceId);
             Assert.Equal(expectedSpanId, activity.ParentSpanId);
@@ -644,10 +642,9 @@ void ConfigureTestServices(IServiceCollection services)
         Assert.Equal(activityName, middlewareActivity.OperationName);
         Assert.Equal(activityName, middlewareActivity.DisplayName);
 
-        // tag http.route should be added on activity started by asp.net core
-        Assert.Equal("api/Values/{id}", aspnetcoreframeworkactivity.GetTagValue(SemanticConventions.AttributeHttpRoute) as string);
+        // tag http.method should be added on activity started by asp.net core
+        Assert.Equal("GET", aspnetcoreframeworkactivity.GetTagValue(SemanticConventions.AttributeHttpMethod) as string);
         Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", aspnetcoreframeworkactivity.OperationName);
-        Assert.Equal("api/Values/{id}", aspnetcoreframeworkactivity.DisplayName);
     }
 
     [Theory]
@@ -763,10 +760,9 @@ public async Task ActivitiesStartedInMiddlewareBySettingHostActivityToNullShould
         Assert.Equal(activityName, middlewareActivity.OperationName);
         Assert.Equal(activityName, middlewareActivity.DisplayName);
 
-        // tag http.route should not be added on activity started by asp.net core as it will not be found during OnEventWritten event
-        Assert.DoesNotContain(aspnetcoreframeworkactivity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRoute);
+        // tag http.method should be added on activity started by asp.net core
+        Assert.Equal("GET", aspnetcoreframeworkactivity.GetTagValue(SemanticConventions.AttributeHttpMethod) as string);
         Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", aspnetcoreframeworkactivity.OperationName);
-        Assert.Equal("/api/values/2", aspnetcoreframeworkactivity.DisplayName);
     }
 
 #if NET7_0_OR_GREATER
@@ -865,12 +861,6 @@ void ConfigureTestServices(IServiceCollection services)
                                         numberofSubscribedEvents++;
                                     }
 
-                                    break;
-                                case HttpInListener.OnMvcBeforeActionEvent:
-                                    {
-                                        numberofSubscribedEvents++;
-                                    }
-
                                     break;
                                 default:
                                     {
@@ -900,7 +890,7 @@ void ConfigureTestServices(IServiceCollection services)
         }
 
         Assert.Equal(0, numberOfUnSubscribedEvents);
-        Assert.Equal(3, numberofSubscribedEvents);
+        Assert.Equal(2, numberofSubscribedEvents);
     }
 
     [Fact]
@@ -930,12 +920,6 @@ void ConfigureTestServices(IServiceCollection services)
                                         numberofSubscribedEvents++;
                                     }
 
-                                    break;
-                                case HttpInListener.OnMvcBeforeActionEvent:
-                                    {
-                                        numberofSubscribedEvents++;
-                                    }
-
                                     break;
 
                                 // TODO: Add test case for validating name for both the types
@@ -984,7 +968,7 @@ void ConfigureTestServices(IServiceCollection services)
 
         Assert.Equal(1, numberOfExceptionCallbacks);
         Assert.Equal(0, numberOfUnSubscribedEvents);
-        Assert.Equal(4, numberofSubscribedEvents);
+        Assert.Equal(3, numberofSubscribedEvents);
     }
 
     [Fact(Skip = "https://github.com/open-telemetry/opentelemetry-dotnet/issues/4884")]
@@ -1078,57 +1062,6 @@ static void ThrowException(IApplicationBuilder app)
         await app.DisposeAsync();
     }
 
-    [Fact]
-    public async Task RouteInformationIsNotAddedToRequestsOutsideOfMVC()
-    {
-        var exportedItems = new List<Activity>();
-
-        // configure SDK
-        using var tracerprovider = Sdk.CreateTracerProviderBuilder()
-            .AddAspNetCoreInstrumentation()
-            .AddInMemoryExporter(exportedItems)
-            .Build();
-
-        var builder = WebApplication.CreateBuilder();
-        builder.Logging.ClearProviders();
-        var app = builder.Build();
-
-        app.MapGet("/custom/{name:alpha}", () => "Hello");
-
-        _ = app.RunAsync();
-
-        using var client = new HttpClient();
-        var res = await client.GetStringAsync("http://localhost:5000/custom/abc");
-        Assert.NotNull(res);
-
-        tracerprovider.ForceFlush();
-        for (var i = 0; i < 10; i++)
-        {
-            if (exportedItems.Count > 0)
-            {
-                break;
-            }
-
-            // 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));
-        }
-
-        var activity = exportedItems[0];
-
-        Assert.NotNull(activity);
-
-        // After fix update to Contains http.route
-        Assert.DoesNotContain(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRoute);
-        Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", activity.OperationName);
-
-        // After fix this should be /custom/{name:alpha}
-        Assert.Equal("/custom/abc", activity.DisplayName);
-
-        await app.DisposeAsync();
-    }
-
     public void Dispose()
     {
         this.tracerProvider?.Dispose();