Update ASP.NET / MVC / WebApi2 Resource Names

[andrewlock]

Currently the resource names for our ASP.NET/WebAPI spans are inconsistently named, and do not make best use of routing information available. This PR makes the naming consistent across MVC/WebAPI2, and moves to using the route information where available.. These changes are currently opt-in, behind a feature flag.

Current Resource Names

Conventional routed MVC (e.g. /Home/Get/0 ) use URL obfuscation for both the "root IIS span" and the "child MVC span", and start with a /:

Attribute routed MVC (e.g. /delay/10) uses URL obfuscation for the "root IIS span" and the route for the "child MVC span", prefixing both with a /:

Conventional routed WebAPI2 (e.g. /api2/delay/3) uses URL obfuscation for the "root IIS span" and the route for the "child MVC span". Only the root span is prefixed with a /:

Attribute routed WebApi2 (e.g. /api/delay/3) is the same as the convential routed WebAPI 2: URL obfuscation for the "root IIS span" and the route for the "child MVC span". Only the root span is prefixed with a /:

New Unified Resource Names

If you enable the feature flag, using DD_TRACE_ASPNET_ROUTE_TEMPLATE_RESOURCE_NAMES_ENABLED=true, then all these approaches give the same span pattern.

• Instead of URL obfuscation, the route template is used where available (i.e. everywhere except 404s)
• All resource names start with /
• Both spans have the same resource name
• Where a route is not found (i.e. it's a 404) we fallback to simple URL obfuscation to remove Ids and Guids
• As with the existing WebApi integration, we replace the common tokens area, controller and action in route names.
• For conventional routing, if a route has unused route parameters, these are removed from the resource name. This means that paths matching the common (default) route {controller}/{action}/{id?} (where id is optional), e.g. /home/action would have resource name GET /home/action instead of GET /home/action/{id}.
• The aspnet.route tag is set both on the root span and the mvc span

Conventional routed MVC (e.g. /Home/Get/0 )

Attribute routed MVC (e.g. /delay/10)

Conventional routed WebAPI2 (e.g. /api2/delay/3)

Attribute routed WebApi2 (e.g. /api/delay/3)

Unknown routes with IDs are still obfuscated as before:

Testing

As these changes could break customer metrics relying on the old behaviour, the new behaviour is behind an opt-in feature flag. Integration tests confirm both the new behaviour, and the old behaviour when the feature flag is disabled.

@DataDog/apm-dotnet

[lucaspimentel]

I'm not done yet. Gotta come back and finish later. Looks great so far!

[lucaspimentel]

Don't bother with routes defined using attribute routing

Why not?

[andrewlock]

IMO, it's a difference in philosophy between the two routing types (and will have a slight performance benefit to not doing the extra work).

Conventional routes (in particular the default route) often have a placeholder {id?} (or similar) parameter, as they're trying to match a single route to multiple actions. So displaying /home/index/{id} wouldn't make any sense when the URL is /home/index and the action is Index().

In contrast, attribute routes are 1-1 tied to an action. So stripping off the (in my experience, rare) optional parameter would give the appearance of two different endpoints when they aren't, so doesn't make sense to me I think. For example, given the action:

[HttpGet, Route("api/optional-delay/{value}")]
public IActionResult OptionalDelay(int value = 5) { /* */}

I think it's more beneficial to have /api/optional-delay and /api/optional-delay/5 both use the route api/optional-delay/{value}, instead of two separate routes for the same endpoint. As I say, the nature of the 1-1 mapping means optional parameters are pretty rare in these cases anyway.

Now, you could make an argument that for the conventional routing, if the user calls /home/index/123, we will use the route /home/index/{id} and they will still be routed to the same Index() action. In that case we have two resources with different resource name routes (one with {id} and one without), but in my experience this is very rarely used in conventional routing, and more often will point to a bug. And the alternative is never stripping the optional routes, will just look weird most of the time.

</essay>

[lucaspimentel]

Your logic makes sense to me. Thanks for the essay 😉

[lucaspimentel]

Finished my review. Mostly just questions about the new feature flag.