Ability to add custom metric attributes to http_client_duration and http_server_duration

[jurabek]

trafficstars

Discussed in https://github.com/open-telemetry/opentelemetry-js/discussions/4313

^{Originally posted by jurabek November 21, 2023} The current version of opentelemetry-instrumentation-http has a limitation where it removes attributes from outgoing-request and incoming-request metrics due to high cardinality issues. However, it would be useful to have the capability of adding other attributes to the HTTP metrics according to the user's requirements. For instance, if I want to filter and monitor specific downstream requests, adding http_target to the outgoing-request metric does not create high cardinality issues for me or other attributes e.g http_status_family: 2xx

Options would be by passing metricAttributes into applyCustomAttributesOnSpan

export interface HttpCustomAttributeFunction {
  (
    span: Span,
    metricAttributes: Attributes,
    request: ClientRequest | IncomingMessage,
    response: IncomingMessage | ServerResponse
  ): void;
}

cc: @hectorhdzg @legendecas for opinions and feedback 😊 Thank you 🙏

[github-actions[bot]]

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.

[jurabek]

remove stale label

[jurabek]

keep it live please 🙏

[github-actions[bot]]

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.

[mbrevda]

no

[hwo411]

Up, we also want to have this.

Our primary case is that we want to set http.route to be able to differentiate by different endpoints. Currently the only way of doing so for us is to patch the library using patch-package. Would be great to have a proper way of doing such changes.

[evan361425]

Added the PR #4884 for it.

Notice that interface didn't have request nor response.

This is because _closeHttpSpan has only these arguments (Span and SpanKind).

If we really needed the request, more codebase need to change and verify.

However, response is undefined when request failed, which means it is not always present.

[github-actions[bot]]

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.

[mbrevda]

not stale

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