Add request path to http-instrumentation metric histogram

derekdowling commented 2 months ago

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 commented 4 days ago

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 commented 3 days ago

I arrived here from https://github.com/vercel/next.js/discussions/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 commented 3 days ago

@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.

open-telemetry / opentelemetry-js