Skip to content

Commit

Permalink
Fix Activity http.route and name (#5026)
Browse files Browse the repository at this point in the history
  • Loading branch information
@alanwest
alanwest authored Nov 17, 2023
1 parent 2a228f9 commit 68eabf0
Show file tree
Hide file tree
Showing 13 changed files with 277 additions and 435 deletions.
1 change: 0 additions & 1 deletion src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
};
Expand Down
11 changes: 11 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
98 changes: 26 additions & 72 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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";

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
{
Expand Down
13 changes: 3 additions & 10 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
26 changes: 26 additions & 0 deletions src/Shared/RequestMethodHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
}
79 changes: 6 additions & 73 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -865,12 +861,6 @@ void ConfigureTestServices(IServiceCollection services)
numberofSubscribedEvents++;
}

break;
case HttpInListener.OnMvcBeforeActionEvent:
{
numberofSubscribedEvents++;
}

break;
default:
{
Expand Down Expand Up @@ -900,7 +890,7 @@ void ConfigureTestServices(IServiceCollection services)
}

Assert.Equal(0, numberOfUnSubscribedEvents);
Assert.Equal(3, numberofSubscribedEvents);
Assert.Equal(2, numberofSubscribedEvents);
}

[Fact]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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();
Expand Down
Loading

0 comments on commit 68eabf0

Please sign in to comment.