ASP.NET Core - Add HTTP method to activity name

[aelij]

Feature Request

Is your feature request related to a problem?

Currently the Activity's display name is set to the MVC template (aka route pattern), which can be confusing as the HTTP method is an integral part of the endpoint's identity.

Describe the solution you'd like:

Include the method in the display name, e.g. PUT keys/{keyid}

[alanwest]

The name of the activity is inspired by the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#name

In case of HTTP servers, these endpoints are often mapped by the server frameworks to more concise HTTP routes, e.g. /api/users/{user_id}, which are recommended as the low cardinality span names.

While the span naming guidance here is not written as a hard requirement, I'd be curious if any HTTP server instrumentation for any other languages deviate much from this. I think it would be worth pursuing this ask with the spec since I think some level of consistency across languages may be desirable.

That said, there's also this statement

Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.

You might use the ASP.NET Core instrumentation's Enrich function to customize the name. Based on the example in the docs, I think you'd perform this under the condition eventName.Equals("OnStopActivity") because the display name will have been set to the route by that point.

[aelij]

@alanwest the guidance you referred to actually gives an example of using the HTTP method:

Therefore, HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}"

I know we can use Enrich, but to me it makes much more sense the name includes the method by default, as different methods represent logically different operations.

[ohadschn]

You might use the ASP.NET Core instrumentation's Enrich function to customize the name. Based on the example in the docs, I think you'd perform this under the condition eventName.Equals("OnStopActivity") because the display name will have been set to the route by that point.

If the only thing we cared about was the activity stop telemetry, this would have sufficed. But in many cases, including ours, all telemetry fired under the activity is enriched with its properties (display/operation/span name being chief among them). Enriching OnStartActivity would have worked were it not for the MVC override (Microsoft.AspNetCore.Mvc.BeforeAction) which cannot be enriched:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Line 275 in d5bfce8

activity.DisplayName = template;

The flow is as below:

1. Request activity is started
2. OpenTelemetry sets the activity's display (operation) name to the request's path
3. OpenTelemetry OnStartActivity enrichment is invoked
4. MVC middleware is invoked, overriding the display (operation) name to the MVC template (via the Microsoft.AspNetCore.Mvc.BeforeAction override)
5. Controller code with service logic is invoked
6. OpenTelemetry OnStopActivity enrichment is invoked

The most important telemetry would arguably be fired between (4) and (6) above, where there is no OpenTelemetry enrichment hook point. I guess an MVC filter would work though.

[alanwest]

the guidance you referred to actually gives an example of using the HTTP method

@aelij The spec makes a distinction between HTTP client and HTTP server conventions. Spans generated by ASP.NET Core instrumentation are considered server side spans.

The recommendation for server side spans is to name it based on the route. The recommended convention for naming client side spans is HTTP {METHOD_NAME} because route information is not generally available client side.

If you'd like to suggest different naming guidance for HTTP server spans, I'd recommend opening an issue with the opentelemetry-specification repo.

[alanwest]

@ohadschn

Enriching OnStartActivity would have worked were it not for the MVC override

Yes, the presence of the MVC BeforeAction event is why I suggested the enrichment hook for OnStopActivity for performing any additional change to the activity name. For example, prefixing the HTTP method to the existing name set by the BeforeAction event. This is a very simple use case, so admittedly it may not be a solution for you depending on your needs.

The most important telemetry would arguably be fired between (4) and (6) above, where there is no OpenTelemetry enrichment hook point. I guess an MVC filter would work though.

We're in the process of evaluating the ASP.NET Core events our instrumentation listens to and potentially refactoring the instrumentation a bit. Adding an additional hook to enrich may be something we could consider. Could you open a separate issue and provide more information about what you're trying to do? Are you suggesting that an enrichment hook during the BeforeAction event would solve it for you?

[ohadschn]

@alanwest

We're in the process of evaluating the ASP.NET Core events our instrumentation listens to and potentially refactoring the instrumentation a bit. Adding an additional hook to enrich may be something we could consider. Could you open a separate issue and provide more information about what you're trying to do? Are you suggesting that an enrichment hook during the BeforeAction event would solve it for you?

Well it looks like an MVC filter works too, but I think BeforeAction enrichment would be more elegant and robust. Specifically, it will potentially affect logs fired between BeforeAction and the first MVC filter. Probably more importantly, it would not depend on MVC filter order.

However I think Ideally we would have dedicated options to control operation name construction. Something like:
AspNetCoreInstrumentationOptions::OperationNameTemplate = "{HTTP_METHOD} {REQUEST_PATH}"
AspNetCoreInstrumentationOptions::MvcOperationNameTemplate = "{HTTP_METHOD} {MVC_TEMPLATE}"

And then Open Telemetry would use these templates rather the current hard-coded ones, e.g.:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Line 154 in d5bfce8

activity.DisplayName = path;

would become activity.DisplayName = AspNetCoreInstrumentationOptions.OperationNameTemplate.Replace("{HTTP_METHOD}", httpContext.Request.Method).Replace("{REQUEST_PATH}", httpContext.Request.Path)

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Line 275 in d5bfce8

activity.DisplayName = template;

would become activity.DisplayName = AspNetCoreInstrumentationOptions.OperationNameTemplate.Replace("{HTTP_METHOD}", httpContext.Request.Method).Replace({"MVC_TEMPLATE", actionDescriptor.AttributeRouteInfo.Template})

Of course you could support more template parameters but those would probably be the most important ones.

[alanwest]

And then Open Telemetry would use these templates rather the current hard-coded ones

I think this is a pretty interesting idea. @cijothomas curious of your thoughts here. I suspect this notion of templating span names may be a good thing to suggest at the spec level since it could easily be implemented across languages. That said, our Enrich functionality is a hook unique to OpenTelemetry .NET instrumentation.