Add request path to http-instrumentation metric histogram

[derekdowling]

NB: Before opening a feature request against this repo, consider whether the feature should/could be implemented in the other OpenTelemetry client libraries. If so, please open an issue on opentelemetry-specification first.

Is your feature request related to a problem? Please describe.

I want to have an OTEL histogram that measures HTTP response timings for my server by method, path, and status code as these are very useful operational telemetry to collect.

Describe the solution you'd like

For the http-instrumentation histogram here:

Add the ability to include the request path, stripped of query params, to the histogram to make the data more useful. Could be a flag, a custom hook, or by making it a default field. Right now the histogram data isn't very actionable without it:

http_server_duration_bucket{http_scheme="http",http_method="GET",net_host_name="localhost",http_flavor="1.1",http_status_code="200",net_host_port="3000",le="0"} 0

Describe alternatives you've considered

Creating a custom histogram / counter, but looking into the implementation of _closeHttpSpan, there are lots of little details that are difficult to account for (errors, closed connections, etc).

Additional context

Add any other context or screenshots about the feature request here.

[pichlermarc]

FWIW it's already possible to do so when using the @opentelemetry/instrumenation-http package in conjunction with @opentelemetry/instrumentation-express for instance.

what that instrumentation does is this:

const rpcMetadata = getRPCMetadata(context.active()); // retrieve rpc metadata from the active context
if (rpcMetadata?.type === RPCType.HTTP) {
    rpcMetadata.route = route || '/'; // set the route, http instrumentation will pick that up and attach it to the span/metric as the `http.route` attribute
}

During the request there will be an active context and the data which can be modified.

getRPCMetadata() is available in @opentelemetry/core. This will only work for server metrics though.
But one has to be very careful. If you end up with high-cardinality data on that metric, it will break:

• (for sure) your app, as it will run out of memory
• (possibly) your telemetry collection pipeline
• (likely) your bank account because observability vendors usually charge for each metrics stream, and every different attribute value costs money

We usually don't advertise this possibility because it's super easy to shoot yourself in the foot with adding attributes to a metric if one does not know exactly how these things work (I see time and time again that people add full URLs including IDs in a REST request path, timestamps, etc). We don't have a flag as there's no one good way of writing a function that will get rid of high cardinality data.

So if you use a server framework (like express), consider using one of the instrumentation that exists for it, too. It will likely add that info automatically (properly scrubbed, because it actually has info about how the route is constructed) and you won't have to worry about possibly costly mistakes.

[ethanmick]

I arrived here from vercel/next.js#16205, where people are working on exporting Prometheus metrics from the Open Telemetry collection built in to Next.js. In that case, being able to add the route to exported metrics would increase their usefulness.

[pichlermarc]

@ethanmick as I said earlier, this is already possible. You can do it on the request hook, or even if the span is currently active while handling your request in your own app, or in a third-party instrumentation.

• Here's an demo of how to do it in the request hook: https://github.com/pichlermarc/demo-4980, the code that does that within that example is here
• Here's how the express instrumentation does it: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/138c6c5cfb186e1b853aa9ab75cddf42e4b76b5e/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts#L188-L191

You can, alternatively, simply do it when actually handling your request like so, just needs the http span to be active:

const server = http.createServer((req, res) => {
    const rpcMetadata = getRPCMetadata(context.active()); // retrieve rpc metadata from the active context
    if (rpcMetadata?.type === RPCType.HTTP) {
        rpcMetadata.route = 'my-custom-route'
     }
    res.statusCode = 200;
    res.setHeader('Content-Type', 'text/plain');
    res.end('Hello, World!\n');
});

No matter which approach you choose, one MUST ensure that the route that's used is safe as a metric attribute (low-cardinality). Simply dropping the query params form a path does not suffice.

[pichlermarc]

Hi all - I'm closing this request in favor of #5051 - this does NOT mean that the request is rejected. I'm just trying to move all requests to that new, more comprehensive tracking issue (there's currently a lot of duplicates of this issue floating around).

I took quite a bit of time summarizing everything on #5051. I also reached out to the semantic conventions SIG to see if this would even be allowed. They are now adding a clarifying text that this is an okay feature to add. Also the .NET SIG currently offers a similar feature already in the ASP.NET Core instrumentation, so we've covered our bases on that side.

We, as the maintainers group have not made a decision yet if we want to add this, but I'll continue to bring this up in discussions with the rest of the maintainers. In one of these conversations, @dyladan came up with an idea on how we could make that API a bit safer to use, so I've attached that proposed API to #5051

In any case: if we decide to implement this feature request we will have to implement #4095 and #4096 as safeguards first. I'll continue bringing this up in regular meetings until we have a decision on this - please keep in mind that since this is a feature request we'll continue to prioritize other topics such as bugfixes and reducing technical debt over adding this feature.