[instrumentation-fetch] Support adding attributes to http spans based on the response body

[chancancode]

Background

opentelemetry-instrumentation-fetch exposes a applyCustomAttributes(span, request, response) hook that can be used to add additional custom attributes to the span resulting from a fetch() request.

In v0.57 and below, it was possible to read from the response body in this hook and apply custom attributes to the span, like so:

new FetchInstrumentation({
  applyCustomAttributes(span, request, response) {
    response.json().then(responseJson => {
      span.setAttribute('some.custom.attribute', responseJson.someData);
    });
  },
});

In v0.58/v.next, this is no longer supported – tests coverage for this type of usage has been removed, and there are no guarantees that it will continue to work. (See #5281.) Specifically:

1. Setting an attribute asynchronously, as shown in the example above, is not guaranteed to work – it may succeed (if the span hasn't been finalized/exported yet), it may be silently dropped, or it may throw.
2. It is not guaranteed that response will have a consumable body – if some other code has already consumed it (which is likely), attempting to read the response body again in applyCustomAttributes, the browser will throw an error (promise rejection)

While this is conceivably a useful feature to have, it was determined that the current design and implementation is intrinsically problematic, and it is not tenable to support it in the as-is. If you have a use case for this feature, please chime in below with details about your use case and whether the proposed alternatives would work for you.

Problems with the current design

This feature was introduced in #2497 back in 2021, and was available as far back as v0.27. There are two problems with the current design.

Asynchronous operation in a synchronous hook

applyCustomAttributes is meant to be a synchronous hook. On the other hand, by design, any methods that touches the body in the Response object is asynchronous.

This presents a fundamental conflict – to read from the response body, it would involve awaiting some promise, and then, using the result from the promise resolution, operate on the span (e.g. setting an attribute) at a later time.

However, nothing in the instrumentation code actually expects this – no one is "holding open" the span/trace while waiting for the async operations to finish, and it's a complete coincidence today that this works at all, most of the time, unreliably. Future changes to the internals (or in theory, a really busy browser) can result in the span being frozen/finalized/exported before the async code had a chance to set the attributes, resulting in the assignment failing silently or with some nondescript error deep inside the SDK internals.

Memory Issues

Today, applyCustomAttributes is only called after the response body has been fully read (which is itself subject to change). Typically, in between the response object becoming available (response headers fully received) and the end of the response body stream, some application code would have already consumed the response body:

   let response = await fetch("/foo");
//                ~~~~~~~~~~~~~~~~~~~ Response object available when this resolves

   let body = await response.json();
//                          ~~~~~~~ Response body consumed here

// ... applyCustomAttributes gets called sometime later...

   applyCustomAttributes(span, request, response) {
     response.json().then( ... );
//           ~~~~~~~ too late to read from the response body
   }

This was the issue raised in #2497. At that time, the solution was to call .clone() on the response object returned by fetch() to reserve the rights for the hook to read from the response body. Essentially this looks something like this:

   let response = await fetch("/foo");
//                ~~~~~~~~~~~~~~~~~~~ Response object available when this resolves

   let responseClone = response.clone();

   let body = await response.json();
//                          ~~~~~~~ Response body consumed here

// ... applyCustomAttributes gets called sometime later...

   applyCustomAttributes(span, request, responseClone) {
     responseClone.json().then( ... );
//                ~~~~~~~ now this works
   }

While this does solve the problem, it does so at the cost of extra memory usage. By cloning every response unconditionally, it forces the browser to buffer and hold on to the response body, possibly making extra copies along that way that could otherwise be avoided.

This creates a lot of extra memory pressure when the body stream is large. In the extreme case (see #4888), the body stream may be effectively infinite, in which case the browser would have hold on to the memory forever, essentially leaking that memory, until it runs out and crashes the tab.

Alternative Design

I can see why this feature can be useful – there may be application-specific data in the response body that is worth including in the instrumentation span. Again, if you have the use cases for that, please comment below.

If someone wants to champion for a replacement for this feature, here are some design constraints to avoid the problems we have with the current design:

1. The new hooks must be asynchronous – and the internals of the instrumentation needs to "hold the span open" for modifications until the returned promise resolves (or rejects).

2. The application must be the one to decide to clone a Response object and await its data – the instrumentation framework cannot make assumption about the response body (that there is a body at all, that the body is JSON, that the body is small/finite, etc).

Concretely, a plausible design would be something like:

new FetchInstrumentation({
  async customAttributesAsync(request, response): Record<string, AttributeValue> | void {
    // The application would want to bound this by *some* conditions – it is likely incorrect to do this unconditionally
    if (request.method === "POST" && request.url.pathname === '/some/endpoint') {
      let responseClone = response.clone();
      let responseJson = await responseClone.json();
      return { 'some.custom.attribute', responseJson.someData };
    }
  },
});

This isn't a concrete proposal, but it shows the elements of the constraints in action. To be clear, I am not proposing the feature myself, and I don't personally have the use case for this. I'm just writing this down as I am the one remove the current broken version of this feature in #5281, so I'm writing down these context for someone who does have the use case and is looking to champion for the return of a revised version.