Skip to content

fix(instr-fetch): avoid unwrap fetch API #6575

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.

Sign up for GitHub

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

Merged
pichlermarc merged 10 commits into from
Apr 17, 2026
+34 −8
Merged

fix(instr-fetch): avoid unwrap fetch API #6575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
402f56e
fix(instr-fetch): avoid unwrap fetch API
david-luna Apr 9, 2026
03ac4dc
tests: restore original fetch after each test
david-luna Apr 9, 2026
05cf192
fix: use original function if instrumentation is disabled
david-luna Apr 9, 2026
fad4bfe
test: restore global fetch correctly in tests
david-luna Apr 10, 2026
3291938
chore: fix lint issue
david-luna Apr 13, 2026
847e9c4
chore: update changelog
david-luna Apr 13, 2026
57855d1
chore: fix changelog entry
david-luna Apr 13, 2026
44c4bf7
chore: add comment about the new private props
david-luna Apr 15, 2026
861a4bf
Update experimental/packages/opentelemetry-instrumentation-fetch/src/…
david-luna Apr 15, 2026
29ded9a
Merge branch 'main' into fetch-xhr-apis-patch
pichlermarc Apr 17, 2026
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
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2

* fix(instrumentation-fetch): preserve init overrides when input is a Request object [#6421](https://github.com/open-telemetry/opentelemetry-js/issues/6421) @akandic47
* fix(otlp-exporter-base): limit Node.js HTTP transport response body to 4 MiB [#6552](https://github.com/open-telemetry/opentelemetry-js/pull/6552) @kartikgola
* fix(instrumentation-fetch): avoid unwrapping fetch API when disabling [#6575](https://github.com/open-telemetry/opentelemetry-js/pull/6575) @david-luna
* fix(web-common): add check for possible unsafe json parse [#6589](https://github.com/open-telemetry/opentelemetry-js/pull/6589) @maryliag

### :books: Documentation
Expand Down
33 changes: 27 additions & 6 deletions experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
import {
SemconvStability,
semconvStabilityFromStr,
isWrapped,
InstrumentationBase,
safeExecuteInTheMiddle,
} from '@opentelemetry/instrumentation';
Expand Down Expand Up @@ -111,6 +110,15 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati

private _semconvStability: SemconvStability;

// Note: Intentionally *not* using `_enabled` as the field name to avoid
// any possible confusion with the `_enabled` field used on the *Node.js*
// InstrumentationBase class.
// Also not initializing the fields to `false` because the base class
// constructor already call `enable` modifying their values and it will
// set the instrumentaitons in a bas state (enabled, patched but with flags set to false)
declare private _isEnabled: boolean;
declare private _isFetchPatched: boolean;
Comment thread
overbalance marked this conversation as resolved.

constructor(config: FetchInstrumentationConfig = {}) {
super('@opentelemetry/instrumentation-fetch', VERSION, config);
this._semconvStability = semconvStabilityFromStr(
Expand Down Expand Up @@ -382,6 +390,9 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
this: typeof globalThis,
...args: Parameters<typeof fetch>
): Promise<Response> {
if (!plugin._isEnabled) {
return original.apply(this, args);
}
const self = this;
const url = web.parseUrl(
args[0] instanceof Request ? args[0].url : String(args[0])
Expand Down Expand Up @@ -611,21 +622,31 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
);
return;
}
if (isWrapped(fetch)) {
Copy link
Copy Markdown
Contributor

@overbalance overbalance Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guard still needed if there are two instances of fetch instrumentation loaded? I think it will double wrap and produce double the data if that happens

Copy link
Copy Markdown
Contributor

@overbalance overbalance Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do we care if another piece of code on the page manages to unwrap fetch? We could re-apply the patch here if so

Copy link
Copy Markdown
Contributor Author

@david-luna david-luna Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good questions :)

Is this guard still needed if there are two instances of fetch instrumentation loaded? I think it will double wrap and produce double the data if that happens

the isWrapped function only tells if something has wrapped the method 1st. I could be the same instrumentation or another one. I'm not sure if there is a way to distinguish. So unwrapping is potentially breaking another instrumentation that wraps fetch.

Also, do we care if another piece of code on the page manages to unwrap fetch? We could re-apply the patch here if so

What comes to mind is another agent or library patching the function. If it uses shimmer I think we are okay but if not we are not 100% sure that the wrap was removed.

If the following code executes after enabling the instrumentation

const nativeFetch = globalThis.fetch; // this is the wrapped function
globalThis.fetch = function (...args) {
  console.log('Fetch');
  return nativeFetch.apply(this, args);
}

now the global fetch function does not have the shimmer props (_wrapped, __original, __unwrap) but its still calling our wrapped transitively.

I'm thinking that maybe we should patch in a way that it cannot be undone and use the instrumentation state/config to control the behavior (which this PR does except it can be unpatched) and also that we cannot protect the users for their wrong doing like creating 2 instances of the same instrumentation.

this._unwrap(globalThis, 'fetch');
this._diag.debug('removing previous patch for constructor');

if (this._isEnabled) {
return;
}
this._isEnabled = true;

if (this._isFetchPatched) {
this._diag.debug('fetch constructor already patched');
return;
}
this._isFetchPatched = true;
this._wrap(globalThis, 'fetch', this._patchConstructor());
}

/**
* implements unpatch function
* deactivates fetch instrumentation
*/
override disable(): void {
if (!hasBrowserPerformanceAPI) {
return;
}
this._unwrap(globalThis, 'fetch');
if (!this._isEnabled) {
return;
}
this._isEnabled = false;
this._usedResources = new WeakSet<PerformanceResourceTiming>();
Copy link
Copy Markdown
Contributor

@overbalance overbalance Apr 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a small race condition here if calls are in flight because of the setTimeout above. Probably rare 🤷

Copy link
Copy Markdown
Contributor Author

@david-luna david-luna Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this is already happening. The 1st that comes to mind is to reuse _clearResources private method which has a check for pending tasks... or just let the resources be cleared when called by the setTimeout callback.

Also it seems clearTimingResources has no default value which becomes undefined (falsy). So if not configured otherwise the instrumentation accumulates resources and I worry that this might lead to memory issues for web apps running for a long period of time. 🤔

}
}
8 changes: 6 additions & 2 deletions experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ function waitFor(timeout: number): Promise<void> {
}

describe('fetch', () => {
const originalFetch = globalThis.fetch;
let workerStarted = false;

const startWorker = async (
Expand Down Expand Up @@ -202,15 +203,18 @@ describe('fetch', () => {
);
} finally {
sinon.restore();
globalThis.fetch = originalFetch;
}
});

describe('enabling/disabling', () => {
// const originalFetch = globalThis.fetch;
let fetchInstrumentation: FetchInstrumentation | undefined;

afterEach(() => {
fetchInstrumentation?.disable();
fetchInstrumentation = undefined;
// globalThis.fetch = originalFetch;
});

it('should wrap global fetch when instantiated', () => {
Expand All @@ -227,11 +231,11 @@ describe('fetch', () => {
assert.ok(isWrapped(window.fetch));
});

it('should unwrap global fetch when disabled', () => {
it('should not unwrap global fetch when disabled', () => {
fetchInstrumentation = new FetchInstrumentation();
assert.ok(isWrapped(window.fetch));
fetchInstrumentation.disable();
assert.ok(!isWrapped(window.fetch));
assert.ok(isWrapped(window.fetch));

// Avoids ERROR in the logs when calling `disable()` again during cleanup
fetchInstrumentation = undefined;
Expand Down
Loading