open-telemetry / semantic-conventions

Defines standards for generating consistent, accessible telemetry across a variety of domains
Apache License 2.0
250 stars 162 forks source link

`http.route` convention: broaden definition #1037

Open howardjohn opened 1 year ago

howardjohn commented 1 year ago

What are you trying to achieve?

In https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md, http.route is defined as:

The matched route (path template in the format used by the respective server framework) /users/:userID?; {controller}/{action}/{id?}

In our API, we have named route matches, which have attributes beyond just path. For example:

apiVersion: networking.istio.io/v1beta1
kind: VirtualService
metadata:
  name: reviews-route
spec:
  hosts:
  - reviews.prod.svc.cluster.local
  http:
  - name: "reviews-consumer"
    match:
    - uri:
        prefix: "/consumercatalog"
       headers:
         end-user:
           exact: jason

Intuitively, I would expect http.route=reviews-consumer here. However, this doesn't directly align with the spec which specified path template.

Is this something that could be expanded to be in scope for http.route? Perhaps "The matched route (path template in the format used by the respective server framework, or other route identifier)"?

trask commented 1 year ago

hi @howardjohn! is http.route=/consumercatalog an option in this case?

I think things are generally nicer when it's some kind of "url path template", since that provides more correlation possibilities with client-side telemetry.

but I'm also not sure whether http.route=reviews-consumer violates the current spec, which only says "path template" which could be interpreted as "code-path" template(?)

lmolkova commented 1 year ago

@howardjohn does istio supports other protocols and some other form of routing/bindings there? It might be that similar attribute would be needed for other cases so that users can debug/monitor their protocol-agnostic routing.

It also seems in the long-term you might want to reflect headers match (headers: end-user: exact: jason ) in some way and HTTP spec won't have any answer for it.

Looking at @trask's reply, we might want to capture both: reviews-consumer and /consumercatalog (agree it should be the route), so maybe we need additional (non-http) attributes to capture routing/binding information?

howardjohn commented 1 year ago

Yes, we also have TCP and TLS route matches which have the same naming of a "route" (or "match", depending on what you want to call it). Kubernetes Gateway API will also likely have this eventually I think (https://github.com/kubernetes-sigs/gateway-api/issues/995); they have more protocols as well.

lmolkova commented 1 year ago

@howardjohn would you be able to suggest some general-purpose attribute(s) for it?

pyohannes commented 1 year ago

I think it's a generic pattern to attach "names" to "routes", just two examples of web frameworks who do it: https://laravel.com/docs/10.x/routing#named-routes https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/route-handlers?view=aspnetcore-7.0#named-endpoints-and-link-generation

Any solution that we come up with should also work for those, it seems like a very similar problem to me.