Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent Activity DisplayName for AddAspNetCoreInstrumentation spans #1744

Open
xinyi-joffre opened this issue Apr 26, 2023 · 7 comments
Labels
bug Something isn't working comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore

Comments

@xinyi-joffre
Copy link

xinyi-joffre commented Apr 26, 2023

Bug Report

"OpenTelemetry.Audit.Geneva" Version="2.0.0"
"OpenTelemetry.Exporter.Geneva" Version="1.4.1"
"OpenTelemetry.Extensions.Hosting" Version="1.4.0"
"OpenTelemetry.Instrumentation.AspNetCore" Version="1.0.0-rc9.14"
"OpenTelemetry.Instrumentation.Http" Version="1.0.0-rc9.14"
"OpenTelemetry.Instrumentation.Runtime" Version="1.1.0-rc.2"

Runtime version: net6.0

Symptom

With auto-instrumentation, the name of spans (Activities) generated from the library seems to be inconsistent between Options / Failed authentication requests, and successful requests.

For OPTIONS or 401/403 requests, the name has a slash in front of it:
(e.g. /api/chat/completions)

For successful requests, it borrows from the http.route name, which doesn't have a slash in front of it:
(e.g. api/chat/completions)

What is the expected behavior?
The name is consistent between all http calls.

What did you expect to see?
Either all spans/activities with slashes in front or all without slashes in front.

What is the actual behavior?
Some spans have a slash and some don't have a slash in front. This makes querying slightly more difficult and seems to not be intentional difference.

Reproduce

A standard dotnet 6 core WebAPI application with AddAspNetCoreInstrumentation and any kind of authentication or options calls from a browser.

Additional Context

This small difference in name means we cannot use exact matches to query the database later for specific names/paths, and need to consider both with front-slash and without front-slash. We tried adding a front-slash to all of our [Route] attributes, but it did not make a difference.

@xinyi-joffre xinyi-joffre added the bug Something isn't working label Apr 26, 2023
@xinyi-joffre xinyi-joffre changed the title Inconsistent ActivityName for AddAspNetCoreInstrumentation spans Inconsistent Activity DisplayName for AddAspNetCoreInstrumentation spans Apr 26, 2023
@DanielLaberge
Copy link

We have this issue as well. See discussion: open-telemetry/opentelemetry-dotnet#5391

@julealgon
Copy link

@cijothomas can you or anyone on the team look into this? The issue has been opened for around a year now with no acknowledgement.

This will become very important for us as we move into modern OTEL and leverage these route names in Datadog as the resource name value: everything is driven through that, including automatic metric names etc.

If a hack is needed (a manual transformation at the OTEL Collector level for example) I want to be sure that I'm at least transforming to the "supposedly correct" value... the problem now is that no one has clarified whether it should start with the / or whether it shouldn't. If at least that could be answered, I could start creating workarounds on my side before the issue is officially fixed in the instrumentation level.

@vishweshbankwar
Copy link
Member

@julealgon
Copy link

Thanks @vishweshbankwar

From the OTEL spec/semantic conventions perspective, am I misinterpreting it in that it recommends having the leading slash?

@vishweshbankwar
Copy link
Member

vishweshbankwar commented Mar 1, 2024

@julealgon I could not find anything specific to / part in the specification. It says The matched route, that is, the path template in the format used by the respective server framework. [4] --> which is what the instrumentation currently follows.

@julealgon
Copy link

@vishweshbankwar what about the point 5, specifically the last section?

[5]: MUST NOT be populated when this is not supported by the HTTP server framework as the route attribute should have low-cardinality and the URI path can NOT substitute it. SHOULD include the application root if there is one.

Is this talking about something else? The link doesn't seem to lead to anything useful there. My initial interpretation is that it was referring to the leading slash but I wasn't sure.

If that's not what it is talking about, I'd like to know what it is referring to there either way.

@reyang reyang transferred this issue from open-telemetry/opentelemetry-dotnet May 13, 2024
@reyang reyang added the comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore label May 13, 2024
@Mnior
Copy link

Mnior commented May 27, 2024

it should start with the / or whether it shouldn't

This is a different level of discussion.
The question is not about the slash /, but about the difference in naming mechanisms in the case of successful responses 200 and unsuccessful 401.
For successful 202 requests, http.route (api/client/{clientID:int}) is substituted, and for unsuccessful requests, url (/api/client/123).
Whether there is this slash or not, the DisplayName is still not uniform.

The problem is that in the case of non-authorized (401) requests, Routing does not come to pass; it is immediately cut off.
And the question is, how should OpenTelemetry behave in such cases?

And how to install custom processing, so that in such cases you can install what is convenient:
A) / - all unauthorized requests are conditionally identical
B) http.route - all commands must be identical, regardless of whether they are authorized or not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore
Projects
None yet
Development

No branches or pull requests

6 participants