Enhance the instrumentation-http hooks or add new ones to allow users to add their own attributes to the server and client metrics

[mkrudele]

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.

A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you'd like

This is related to #3683.
Our need is to add extra attributes to the http_server and http_client metrics provided by the HTTP instrumentation.
That's not actually possible because of this #3683 (comment).
Ideally we would use the requestHook https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_instrumentation_http.HttpInstrumentationConfig.html#requestHook to inject custom attributes to those metrics, but any additional hook would work as far as we can add additional attributes to http server and client duration metrics.

Describe alternatives you've considered

The alternative is actually using the instrumentation-express, which adds the http_route to the server metric. However, we have the need to add additional custom attributes to both, server and client metrics.

Additional context

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

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[mivanilov]

I have also run into this issue of missing http_route attribute on the http_server_duration metric.
I want to only export metrics and if traces export is disabled OTEL_TRACES_EXPORTER: none then express instrumentation actually doesn't work - http_route attribute is not added to the http_server_duration metric.
Looks like a bug considering the similar setup (exporting only metrics) e.g. in Java instrumentation adds http_route to the http_server_duration metric.
@pichlermarc Not sure if it's worth raising a bug or if we get this feature-request fulfilled then it could help to workaround it?

[pichlermarc]

I have also run into this issue of missing http_route attribute on the http_server_duration metric. I want to only export metrics and if traces export is disabled OTEL_TRACES_EXPORTER: none then express instrumentation actually doesn't work - http_route attribute is not added to the http_server_duration metric. Looks like a bug considering the similar setup (exporting only metrics) e.g. in Java instrumentation adds http_route to the http_server_duration metric.

@mivanilov I can see that this wouldn't work when tracing is disabled, as the info is passed around on the context 🤔

@pichlermarc Not sure if it's worth raising a bug or if we get this feature-request fulfilled then it could help to workaround it?

It's worth opening a bug issue for this. If you can, please also include a minimal reproducer.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[JohanBjoerklund]

Is this being looked at? Looks like maciejnawrocki#1 would solve this?

[pichlermarc]

@JohanBjoerklund I couldn't find this PR on this repo, it looks like this PR is not for this repo but their fork of it. 🤔

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[mkrudele]

@pichlermarc, is this being looked at? Can we have a final word of whether it will be implemented?

[pichlermarc]

@mkrudele it's a very complex question. I opened another issue (#5051) to collect my thoughts and other requests in a single place. I raised actually this question last week with the other maintainers as it tends to be a recurring theme, but I was lacking enough research about previous requests.

I put all of this research now into #5051. Long story short: if we make the decision to add it we will have to reach out to the SemConv SIG first. We also need to implement two other things before we can add the feature (memory limiting for attributes, not exporting metrics that have not been written in the collection timeframe) to guard users who may misuse the API due to an unfamiliarity with how metrics work.

I'll bring it up again with the other maintainers this week. I've also reached out to a maintainer from the SemConv SIG to discuss the topic.

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