[Instrumentation.AspNetCore] Fix http.route for HTTP server spans - #3338

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Codecov Report

❌ Patch coverage is 78.04878% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.78%. Comparing base (a5f3fc0) to head (5bfe512).
⚠️ Report is 470 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3338      +/-   ##
==========================================
+ Coverage   68.61%   68.78%   +0.17%     
==========================================
  Files         431      422       -9     
  Lines       16429    16410      -19     
==========================================
+ Hits        11272    11287      +15     
+ Misses       5157     5123      -34     

I'll recheck http route tests readme and update the changelog. So after that, this change should be ready to go.

My opinion about these topics:

• He would like to avoid differences in the http.route related parameters between metrics and the traces. The main reason behind this, is the fact that some backends are using it to correlate these signals based on this attribute.

That is already so broken and off the spec that I'm not sure which parts could be trusted for machine processing.
There is practically no cardinality at all for some apps.

• @JamesNK, is there any chance that you can advice how to fix http.route spans for metrics? Do you see any options to modify these attributes on OTel instrumentation side? If not, can you apply changes directly in the AspNetCore?

Since SDK is exporting them, then additional processor should be available for API users as well as SDK users already have that option.

• Not directly related, but AspNetCore is creating activities, but without needed attributes, so we here recreating such activities in this package. Again, @JamesNK, do you see any option to fully manage these activities+attributes by the AspNetCore directly?

Same as above.

This PR was marked stale due to lack of activity. It will be closed in 7 days.

I’m away. I’ll reply next week

This PR was marked stale due to lack of activity. It will be closed in 7 days.

I updated OTEL http.route usage in ASP.NET Core to replace static parameters. It's currently added to metrics, and will be used with Activity tags soon.

The PR to created improved routes is here: dotnet/aspnetcore#64854

I think the approach I used (using RequiredValues collection) is better. The rest of the route unchanged except the replaced parameter. And it doesn't rely on hardcoded parameter names, e.g. it won't try to replace a "controller" parameter in a minimal API route.

I know the spec is quite weak about framework specifics in the http.route. The goal is only to get a "statistically interesting unique collection" of routes.

I would personally hate to see "unnecessary paddings" like :int, ?, {id?} when id is missing. I'm not sure that they give much value contextually as statistically interesting tokens. ASP.NET Fx (like mvc4) does not do those, so not adding them would unify values further.

src: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.50.0/specification/trace/api.md#span

The span name SHOULD be the most general string that identifies a (statistically) interesting class of Spans, rather than individual Span instances while still being human-readable.

Secondly those tokens should be present then also in the Activity DisplayName, which makes it even more, not making sense.
Also because The span name SHOULD be the most general string would not be true.

Thirdly, those tokens can add cardinality is some cases.

src: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name
HTTP span names SHOULD be {method} {target} if there is a (low-cardinality) target available. If there is no (low-cardinality) {target} available, HTTP span names SHOULD be {method}.

CC: @JamesNK @alanwest @martincostello @Kielek

I would personally hate to see "unnecessary paddings" like :int, ?, {id?} when id is missing. I'm not sure that they give much value contextually as statistically interesting tokens. ASP.NET Fx (like mvc4) does not do those, so not adding them would unify values further.

People will expect http.route to be the route they've definied in their app. I'm ok with modifying conventional routes with constant values because in that case you're defining a route convention that applies to many actions. But rewriting the route that is in an attribute or in MapGet violates the principal of least surprise. People expect http.route == [HttpGet("my-route")] or MapGet("my-route", () => {}).

And things like constraints are important information. This is possible:

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/product/{id:alpha}", (string id) => "alpha");
app.MapGet("/product/{id:int}", (string id) => "int");

app.Run();

Stripping the constraint would make the matched route ambigious.

The span name SHOULD be the most general string that identifies a (statistically) interesting class of Spans, rather than individual Span instances while still being human-readable.

Constraints are important information. And optional parameters are part of the route, even if they're not matched.

Even if we wanted to optionally add optional parameters to the route string, it would mean a performance cost because it couldn't be statically determined. The change I made to ASP.NET Core means that the final route template is generated when the endpoint is created. There is no per-request overhead.

This basically means that spec must separate HTTP span name from including http.route. I do agree it's important information (as of http.route) but it also clutters HTTP span name. This way there is no reason to replace "static partials" in the http.route, but HTTP span name for sure requires it.

@RassK Should we close this PR? It's been open for over 6 months with zero forward progress.

@RassK Should we close this PR? It's been open for over 6 months with zero forward progress.

Yes, I can update it if there is any progress. So far seems it's stuck in spec and even if there is going to be any progress, then most probably it needs native support to avoid doing performance hitting work in hot paths.