Use route path as resource name instead of url

[inikolaev]

I would like to propose to use route path as a resource url. This is useful when there are path parameters present in the URL, but it's usually undesirable to report each individual instance.

For example consider the following route - /users/{id}. All requests with different values of id parameter will be reported as distinct resources to DataDog, while in fact it's the same resource. This will also help with calculating different metrics, e.g. latency, for the whole resource instead of individual requests.

Unfortunately there is no populated property route in the Request, but there is a way to retrieve route nevertheless:

path = None
for route in request.app.router.routes:
  match, scope = route.matches(request)
    if match == Match.FULL:
      path = route.path
      break
    elif match == Match.PARTIAL and path is None:
      path = route.path

if path is None:
  path = url.path

This is somewhat similar to the code in Starlette: https://github.com/encode/starlette/blob/master/starlette/routing.py#L544

What do you think? I can provide a PR for this.

[florimondmanca]

Thanks for writing this up!

One important part about this is recognizing that we’re leaving raw ASGI territory to enter framework-specific functionality.

From a maintenance and extensibility perspective I’m not very keen on doing that, as that would mean ddtrace-asgi would have to closely follow the changes of the APIs used for each specific framework. We’d also be very exposed to feature creep as we definitely wouldn’t be able to address the use cases of everyone.

Instead, I think we should try and think of adding an extension entry point to the middleware that would be called to enhance a span.

For instance for this use case, the entry point would go through the routes and find the one that matched — or perhaps retrieve that from the scope if the framework made it available there.

I’m not sure yet how the API would look like. For now I’m thinking of allowing users to provide a callable that takes as input the span and the ASGI scope:

def prepare_trace(span, scope):
    routes = scope['app'].router.routes
    path = ... # Your snippet above
    span.set_tag('url_pattern', path)

(It would be called before calling into the ASGI application, so this wouldn’t allow modifying the span based on the response... if that’s a potential use case then we certainly can find ways to extend the API outlined here.)

Thoughts?

[inikolaev]

I haven't realized that the library was meant to be framework agnostic, but it makes perfect sense.

I think the idea with providing a callable which can enrich span makes sense. I was worried that it wouldn't be possible to change resource name of the span, but it seems working fine.

How do you see the per-route submission - should it be a core feature? I feel like it is, but on the other hand it's framework specific.

On the other hand as a user I would not like to do any configuration - I want per-route submission to work out-of-the box, but this can't be done.

Maybe instead, there should be a separate framework specific library, which would extend TracingMiddleware and implement prepare_trace function or whatever it's called.

What do you think?

[florimondmanca]

Hmm.

On second thought, let’s see if we can have some routing support in core. We already have Starlette as a dependency of this package, so at least we can try to tackle routing support for Starlette-based apps if we detect that we might be running one. If not then we should fallback gracefully to submitting the full URL path.

Would you like to draft a PR so we’d see what this would look like? :)

[inikolaev]

Sure, I can do that. Don't yet know how to detect if we are running Starlette app (probably check app instanceof Starlette?), but let's see how that works out.

[florimondmanca]

I know Starlette injects its app or router directly into the ASGI scope (we’d need to dig in its source code to verify), so we could use a less constrained if "router" in scope: ....

[inikolaev]

Starlette indeed sets router, the only problem is that it's set after this line is executed: https://github.com/florimondmanca/ddtrace-asgi/blob/master/src/ddtrace_asgi/middleware.py#L104

That means that any span manipulations can be only done in finally block.

[florimondmanca]

@inikolaev

I reverted back to my initial intuition on this, i.e.:

Instead, I think we should try and think of adding an extension entry point to the middleware that would be called to enhance a span.

I submitted #35 with an implementation of the extension mechanism proposed in #32. Feel free to take a look. :-)