Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hook support for adding custom dynamic metric attributes/ dimensions #4943

Closed
alexa-gt opened this issue Aug 22, 2024 · 1 comment
Closed

Comments

@alexa-gt
Copy link

Is your feature request related to a problem? Please describe

I am currently looking at implementing the following but running into an issue of not being able to add custom attributes/dimension, namely peer.service to the http.server.duration/ http.server.request.duration metric.

Describe the solution you'd like to see

The ability to add custom attributes and dimensions as a hook to the http.server.duration/ http.server.request.duration metric emitted by @opentelemetry/instrumentation-http, with something of the following signature:

export interface HttpRequestCustomMetricAttributeFunction {
    (request: ClientRequest, response: ServerResponse, incomingMessage: IncomingMessage): Attributes;
}

Describe alternatives you've considered

  • Stick strictly to the semconv metric
    • allows no deviation or possibility of deviation without completely ditching the autoinstrumentation library, or writing a custom middleware.
  • spanmetrics connector in the OTEL connector with the selected dimension
    • this is not possible for my use case as I am unable to deploy otel-col in my architecture in the near term. It has to be an app-solution.
  • this hack that embeds additional info in http.route.
    • seems very strange to build an app's routing system around a workaround

Additional context

None.

@pichlermarc
Copy link
Member

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

@pichlermarc pichlermarc closed this as not planned Won't fix, can't repro, duplicate, stale Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants