[instrumentation-http] custom client/server metric attributes

[pichlermarc]

Description

Important

The existence of this issue does not reflect the wish of the author for this to be implemented
This is a feature request that tracks issues created over time and provides details from the viewpoint of a maintainer which may not be immediately obvious to end-users. It serves as a basis for further discussions.

Status quo

Over time we've added quite a few so-called "hooks" to the HttpInstrumentation that allow for customization of the emitted telemetry. This allows the addition of custom attributes the Spans created by the instrumentation.

A downside of this is that the more we add these hooks, the more it allows the user to change the telemetry that's being emitted from what is defined in the semantic conventions. Another downside of this is increased maintainance burden for maintainers, approvers and triagers - as the code becomes more complex, more bugs become possible and bug reports become harder and harder to work with. The interface also becomes more and more complex for users, and it leads users down a path where it becomes an expectation for the user that application-specific instrumentation happens in instrumentation-hooks and through the addition of attributes instead of creating their own child spans using the @opentelemetry/api which contain the needed data.

For metrics, though, no such hooks exist, they would come with the same risks and challenges as the existing hooks in HttpInstrumentation.

Common Feature request

A common request is that the same treatment that's given to Spans be given to metrics emitted by the HttpInstrumentation.

Here the case seems easy enough to make: There's no easy way to instrument all http calls made through the node:http and node:https and re-create the metrics in a different way, with added attributes. It stands therefore to reason that such a hook is necessary to allow the addition of custom-non-standard attributes to this metric.

The most common request, is the one of adding http.route. http.route is not collected by default as the HttpInstrumentation on it's own cannot safely determine a low-cardinality route, since it does not have access to the routing information of a higher-level package. Without http.route the emitted metrics become essentially useless, unless only a single endpoint is served by the service.

Instrumentations, such as @opentelemetry/instrumentation-express work around this limitation by using a feature of @opentelemetry/core that allows instrumentation on the server framework to attach metadata to the context. This metadata is then read back by @opentelemetry/instrumentation-http to and attached to both the Span's attributes and the Histogram datapoint that is being recorded.

Example 1 (excerpt from instrumentation-express):

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
}

Here @opentelemetry/instrumentation-express and similar instrumentations do ensure that the route value is low-cardinality.

Users may take a similar approach as @opentelemetry/instrumentation-express does, by setting the metadata on the context themselves, either during the handling of the HTTP request on the server-side or in one of the hook offered by @opentelemetry/instrumentation-http, which comes out to be the same thing. While doing so, users have to be careful to ensure that the values provided as that metadata actually reasonable match that of a route - but in essence, this feature already exists for the http.route attribute. It does however, need a global ContextManager to be registered, which may not be the case when only exporting metrics from an app, and enabling the ContextManager introduces overhead.

Example 2 (adding metadata when handling the request):

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');
});

The second most common request is the one of adding arbitrary attributes to client and server metrics. For that, there is currently no workaround. The reasons for that can be found in the following section, "Risks".

Risks

Currently this feature being hard to use for http.route and not available at all for custom metrics has prevented less-metrics-savvy users from breaking their apps through high-cardinality metrics.

The question comes up from time-to-time to add attributes such as http.url (which can really be any URL) to a metric (example). Such a metric can cause significant problems in the user's app and downstream. Any attribute value creates a new metrics stream, and any metrics stream has to stay in-memory over the lifetime of the app to ensure that cumulative metrics work as-intended. This means that failure of the user to select an appropriate attribute will cause their app to run out of memory and crash. Further, their app will suffer an signficant performance impact from many newly created metrics streams.

To alleviate this issue, we will need to implement #4095 first. This will solve the problem of the crash, but the significant performance impact will remain if this interface is misused. Regardless of implementing this issue, the effects on downstream system can also be catatstropic, from out-of-memory OpenTelemetry Collectors and possible scaling problems of collector pipelines, down to excessive cost to the user for storing these high-cardinality metrics.

A possible path to implementing the feature

At first, we (@open-telemetry/javascript-maintainers) need to make the following decision:

Do we even want to support adding custom attributes for metrics?

Note

Update 06 Nov 2024, we've decided to go with this approach do not start working on implementing this actual feature. For now contributions are welcome on the prerequisites as they are more general features required by the specification. Only then we will allow the actual feature to be added.

If the answer to that question is YES (Update 06 Nov 2024, we've decided to go with this approach):

• we need to reach out to the Semantic Conventions SIG and ask for their guidance, if their guidance is that we SHOULD NOT add non-specified attributes to these metrics, we should strongly consider following their advice and change our answer to NO.
  • Update 14 Oct 2024 This has been brought up by the Semantic Conventions SIG and there is precedence for this already. With Make it clear that semconv metric attributes can be extended by users semantic-conventions#1475, they will add that info that this is allowed to the docs.
• (prerequisite) we need to implement [sdk-metrics] Implement experimenal cardinality limit specification #4095 first to avoid high-cardinality metrics causing end-user apps to run out of memory, as such an interface can easily be misused by a user in the ways mentioned in the "Risks" section.
• (prerequisite) we need to implement [sdk-metrics] Do not report metric data for previosuly observed attribute sets that were not observed during the current cycle  #4096 to reduce the impact of high-cardinality metrics on the compute resources needed by the app when the interface is misused
• then we can implement this feature
  • Outdated (06 Nov 2024) the API for adding custom attributes SHOULD be designed in such a way that it is impossible for users to add metric attribute values that they do not know in advance. This can be done, for instance, by requiring a map when instantiating the hook, that contains all possible known keys and values for the attributes that can be produced by the hook. See "Attachment 1 (proposed API)" below. This API prevents high cardinality data, but we expect a metrics attribute hook to set a precedent for other instrumentations (possibly thrid-party instrumentations) to do similar things. Since these other instrumentations may decide to be less careful with what they allow, we still SHOULD implement the prerequisites listed above as a safeguard.
  • Update 06 Nov 2024 the API should follow "Attachment 2 (proposed API without safeguards)" as the cardinality limit specification ([sdk-metrics] Implement experimenal cardinality limit specification #4095) and bugfix for ([sdk-metrics] Do not report metric data for previosuly observed attribute sets that were not observed during the current cycle  #4096) is expected to alleviate issues that come from any misuse of the API.

Outdated (06 Nov 2024) If the answer to that question is NO:

• we need to consider if we want to simplify the way users can add the http.route attribute through a hook
• if yes: we also need to implement [sdk-metrics] Implement experimenal cardinality limit specification #4095 and [sdk-metrics] Do not report metric data for previosuly observed attribute sets that were not observed during the current cycle  #4096 first for the same reasons as stated above.

This issue is considered done when

• Semantic Conventions SIG has given the go ahead for this.
• Maintainers have reached a decision on whether we want this feature to be implemented or not
  • A follow-up type:feature issue that condenses the implementation-relevant information from this issue has been created and linked on this issue. The created issue has been marked as blocked until the prerequisites are done ([sdk-metrics] Implement experimenal cardinality limit specification #4095, [sdk-metrics] Do not report metric data for previosuly observed attribute sets that were not observed during the current cycle  #4096).
    • Update Nov 11, 2024: [instrumentation-http] add hook for custom server and client metrics attributes #5135 has been created to track implementation

Links

Previous requests

• Add request path to http-instrumentation metric histogram #4890 (adding request path stripped of query params, might run into high-cardinality problems)
• Ability to add custom metric attributes to http_client_duration and http_server_duration #4317
• Enhance the instrumentation-http hooks or add new ones to allow users to add their own attributes to the server and client metrics #3694
• PrometheusExporter - HTTP Route Not Being Emitted #4474
• http.target and http.url attributes not set in metrics attributes #3683
• Hook support for adding custom dynamic metric attributes/ dimensions #4943
• Missing "http.route" in custom OpenTelemetry Next.js configuration #5110
• Add http route to outgoing http requests, if available #5129

Resources referencing other implementation of this feature

• https://learn.microsoft.com/en-us/aspnet/core/log-mon/metrics/metrics?view=aspnetcore-8.0#enrich-the-aspnet-core-request-metric

Attachment 1 (proposed API)

Note

outdated, we will use the poposed API from Attachment 2 with no safeguard

new HttpInstrumentation({
  incomingRequestMetricAttributeHook: {
    // required property, not optional.
    possibleAttributes: {
      'http.route': [ 'my/route/foo',  'my/route/bar' ]
      'my.custom.attribute': ['foo', 'bar', 'baz']
    },
    (request) => {
       return {
           'http.route': determineRoute(request.url), // value will be filtered by instrumentation-internal logic if value does not match one of the values above, and replaced by "filtered" or "unknown"
           'my.custom.attribute': 'foo123', // gets filtered because it does not match a value from the possible values for 'my.custom.attribute' above, and replaced by "filtered" or "unknown"
           'my.unknown.attribute': 'foo', //gets removed completely from attributes as the key is not known above.
       }
    }
  } 
});

(this is a proposal I created after @dyladan's suggestion to design the interface in a way that makes it hard/impossible to misuse)

Attachment 2 (proposed API with no safeguards)

new HttpInstrumentation({
  incomingRequestMetricAttributeHook: (request) => { 
      return {
           // it is up to the user to return a reasonable amount of key-value pairs here, this needs to be properly documented.
           'http.route': determineRoute(request.url), // functionality to determine route is up to the user, it's up to the user to ensure that data is not high cardinality
           'my.custom.attribute': 'foo123, // any custom attributes are allowed 
       };
   },
   // same behavior for outgoing hook
});

[pichlermarc]

Hi all, I'm closing this feature-request as ACCEPTED as it has been refined into #5135.

It will be ready for implementation once #4095 and #4096 are implemented. #5135 will track implementation and is blocked until the aforementioned prerequisite issues are done.