Allow root action rewrites

[PascalSenn]

Explanation

Since #5026 the activity name of the asp net core span is set on activity end. This means that any change that was done to the name of this span will be overwritten. Many monitoring solutions ( elastic, sentry, datadog et al) use the name of the root span to group similar spans. A tool to control this was to rename the root activity, which is the asp net core activity if you have it enabled. As #5026 overwrites any changes to the root activity on transaction end, this control is gone.

Example

The path (or route pattern) for a GraphQL request is always the same. For example /graphql. The open telemetry integration of HotChocolate rewrites the name of the root span to the operation that you are execution. Specficially into something like query FindUserById { userById }, mutation updateUserName { updateUserName } ... . As of #5026, all graphql operations are reported as POST /graphql/{**slug} which makes it impossible to properly trace different operations.

The specific treatment of GRPC in the HttpInListener shows that it would have the same issue, it just happens to be included by default.

Why the hooks is not enough

There is a hook which you can use to change the properties of the span. Frameworks like HotChocolate (or others) that want to overwrite the naming of the AspNetCore trace could just use this hook. But this is not a good solution:

1. It requires all frameworks to have a dependency on OpenTelemetry.Instrumentation.AspNetCore.
2. It would require the user to add additional configuration to the AddAspNetCoreInstrumentation registration, which leads to confusion.

Changes

Before overwriting the activity name on activity end, the DisplayName is compared to the one that is set on activity start. If the values are not the same, the user ( or a library), is interested in renaming the name of the span. In this case the name of the span in not modified on activity end.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[vishweshbankwar]

@PascalSenn Do you mind elaborating bit more on

The specific treatment of GRPC in the HttpInListener shows that it would have the same issue, it just happens to be included by default.

I would like to understand what other issues you are running into.

[PascalSenn]

@vishweshbankwar

The DisplayName of the activity is set on activity end here:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L244

After this code, there is a special treatment for GRPC:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L251-L254

Which then overwrites the activity name to the RPC spectific semantic convention:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L351-L354

If GRPC would not be handled in OpenTelemetry.Instrumentation.AspNetCore there would be no way for a GRPC component further down the middleware pipeline to set correct root span name for the RPC semantic convention unless you would register a specific enricher withEnrichWithHttpResponse on the AspNetCore instrumentation

[vishweshbankwar]

@vishweshbankwar

The DisplayName of the activity is set on activity end here: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L244

After this code, there is a special treatment for GRPC: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L251-L254

Which then overwrites the activity name to the RPC spectific semantic convention: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L351-L354

If GRPC would not be handled in OpenTelemetry.Instrumentation.AspNetCore there would be no way for a GRPC component further down the middleware pipeline to set correct root span name for the RPC semantic convention unless you would register a specific enricher withEnrichWithHttpResponse on the AspNetCore instrumentation

@PascalSenn - Thanks for explaining. I think it's acceptable to implement this minor adjustment in the instrumentation library. However, it's important to note that the library cannot handle any other sequencing issues comprehensively. It primarily enhances activities according to the OTel specification. For any other requirements, users can utilize the Enrich API to customize existing activities.

[PascalSenn]

@vishweshbankwar
Do you mean with

I think it's acceptable to implement this minor adjustment in the instrumentation library

the GRPC code which is already in place or the change in this PR?

[vishweshbankwar]

@vishweshbankwar Do you mean with

I think it's acceptable to implement this minor adjustment in the instrumentation library

the GRPC code which is already in place or the change in this PR?

I meant the change in this PR.

[PascalSenn]

@vishweshbankwar

Very well, i would assume it's also what most frameworks with custom semantic conventions (like RPC) would want to do. Additional properties are also not a problem. They most important part is the name of the root activity.

[vishweshbankwar]

@PascalSenn - Could you add a unit test for this? These tests are more focused on http.route tag value.

[PascalSenn]

You mean instead of the one in RoutingTests?

[vishweshbankwar]

ok to add this in addition to unit test. @alanwest - Do you have any preference?

[alanwest]

I think it makes sense to test this new behavior separate from the tests for http.route.

[alanwest]

Before we move forward with this PR, there's a few things we should discuss.

I captured some general thoughts/questions on #5417 which may be best to discuss in our next SIG meeting.

With respect specifically to this AspNetCore instrumentation, if we move forward with this PR, we should confirm how and if this can be solved in the event that ASP.NET 9 introduces native trace instrumentation (see dotnet/aspnetcore#52439)

[OptimusPi]

Fix typo here to satisfy PR misspell bot.

[OptimusPi]

replace typo overriden with overridden to satisfy ms-misspell PR check.

[vishweshbankwar]

@PascalSenn - We discussed this during yesterday's SIG meeting. Before proceeding with committing this change, we'd like to wait and gather more information on how dotnet/aspnetcore#52439 will address these scenarios. This is to ensure that we don't encounter similar issues when users upgrade to a new .NET version where instrumentation is natively implemented within ASP.NET Core.

[vishweshbankwar]

Closing this for now. See #5402 (comment)

We will revisit this once we have more details on dotnet/aspnetcore#52439