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

feat(instrumentation-fetch)! Passthrough original request to applyCustomAttributes #5281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ 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, please chime in on [#5293](https://github.com/open-telemetry/opentelemetry-js/issues/5293).

### :rocket: (Enhancement)

### :bug: (Bug Fix)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -372,15 +372,14 @@ 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();
const read = (): void => {
reader.read().then(
({ done }) => {
if (done) {
endSpanOnSuccess(span, resClone4Hook);
endSpanOnSuccess(span, response);
} else {
read();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -930,25 +930,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;
}
};

await prepare(url, applyCustomAttributes);
const rsp = await response.json();
assert.strictEqual(rsp.isServerResponse, true);
assert.strictEqual(request.method, 'GET');
assert.ok(lastResponse !== undefined);
assert.strictEqual(response, lastResponse);
assert.strictEqual(response.status, 200);

/*
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);
});
});

Expand Down
Loading