From 541dbe576663f78f8eb22ab37dd598e14ceff550 Mon Sep 17 00:00:00 2001
From: Godfrey Chan <godfreykfc@gmail.com>
Date: Tue, 17 Dec 2024 14:49:47 -0800
Subject: [PATCH] feat(instrumentation-fetch)!: passthrough original response
 to `applyCustomAttributes` hook

Previously, the fetch instrumentation code unconditionally clones
every `fetch()` response in order to preserve the ability for the
`applyCustomAttributes` hook to consume the response body. This is
fundamentally unsound, as it forces the browser to buffer and
retain the response body until it is fully received and read, which
crates unnecessary memory pressure on large or long-running response
streams. In extreme cases, this is effectively a memory leak and can
cause the browser tab to crash.

Fixes #4888
---
 experimental/CHANGELOG.md                     |  2 +
 .../src/fetch.ts                              |  3 +-
 .../test/fetch.test.ts                        | 41 +++++++++++--------
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
index 83471e2416..9787243b31 100644
--- a/experimental/CHANGELOG.md
+++ b/experimental/CHANGELOG.md
@@ -19,6 +19,8 @@ All notable changes to experimental packages in this project will be documented
 
 ### :boom: Breaking Change
 
+* feat(instrumentation-fetch)!: passthrough original response to `applyCustomAttributes` hook [#5281](https://github.com/open-telemetry/opentelemetry-js/pull/5281) @chancancode
+  * Previously, the fetch instrumentation code unconditionally clones every `fetch()` response in order to preserve the ability for the `applyCustomAttributes` hook to consume the response body. This is fundamentally unsound, as it forces the browser to buffer and retain the response body until it is fully received and read, which crates unnecessary memory pressure on large or long-running response streams. In extreme cases, this is effectively a memory leak and can cause the browser tab to crash. If your use case for `applyCustomAttributes` requires access to the response body, TODO...
 * feat(otlp-exporter-base)!: collapse base classes into one [#5031](https://github.com/open-telemetry/opentelemetry-js/pull/5031) @pichlermarc
   * `OTLPExporterNodeBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase`)
   * `OTLPExporterBrowserBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase`)
diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts b/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
index e5d9a84bde..8088cdae21 100644
--- a/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
+++ b/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
@@ -368,7 +368,6 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
         ): void {
           try {
             const resClone = response.clone();
-            const resClone4Hook = response.clone();
             const body = resClone.body;
             if (body) {
               const reader = body.getReader();
@@ -376,7 +375,7 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
                 reader.read().then(
                   ({ done }) => {
                     if (done) {
-                      endSpanOnSuccess(span, resClone4Hook);
+                      endSpanOnSuccess(span, response);
                     } else {
                       read();
                     }
diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
index 1c6a865c6d..d0b2d2e41c 100644
--- a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
+++ b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
@@ -931,24 +931,33 @@ describe('fetch', () => {
 
       await prepare(url, applyCustomAttributes);
       assert.ok(request.method === 'GET');
-      assert.ok(response.status === 200);
-    });
 
-    it('get response body from callback arguments response', async () => {
-      let response: any;
-      const applyCustomAttributes: FetchCustomAttributeFunction = async (
-        span,
-        req,
-        res
-      ) => {
-        if (res instanceof Response) {
-          response = res;
-        }
-      };
+      assert.ok(lastResponse !== undefined);
+      assert.strictEqual(response, lastResponse);
+      assert.ok(response.status === 200);
 
-      await prepare(url, applyCustomAttributes);
-      const rsp = await response.json();
-      assert.strictEqual(rsp.isServerResponse, true);
+      /*
+         Note: this confirms that nothing *in the instrumentation code*
+         consumed the response body; it doesn't guarantee that the response
+         object passed to the `applyCustomAttributes` hook will always have
+         a consumable body – in fact, this is typically *not* the case:
+
+         ```js
+         // user code:
+         let response = await fetch("foo");
+         let json = await response.json(); // <- user code consumes the body on `response`
+         // ...
+
+         {
+           // ...this is called sometime later...
+           applyCustomAttributes(span, request, response) {
+             // too late!
+             response.bodyUsed // => true
+           }
+         }
+         ```
+      */
+      assert.strictEqual(response.bodyUsed, false);
     });
   });