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

fix(instr-fetch): do not enable in Node.js; clarify in docs this instr is for web fetch only #4498

Merged
merged 5 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ All notable changes to experimental packages in this project will be documented
* fix(instrumentation): normalize paths for internal files in scoped packages [#4467](https://github.com/open-telemetry/opentelemetry-js/pull/4467) @pichlermarc
* Fixes a bug where, on Windows, internal files on scoped packages would not be instrumented.
* fix(otlp-transformer): only use BigInt inside hrTimeToNanos() [#4484](https://github.com/open-telemetry/opentelemetry-js/pull/4484) @pichlermarc
* fix(instrumentation-fetch): do not enable in Node.js; clarify in docs this instr is for web fetch only [#4498](https://github.com/open-telemetry/opentelemetry-js/pull/4498) @trentm

### :books: (Refine Doc)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@

**Note: This is an experimental package under active development. New releases may include breaking changes.**

This module provides auto instrumentation for web using fetch.
This module provides auto instrumentation for web using [fetch](https://developer.mozilla.org/en-US/docs/Web/API/fetch).
(Note: This instrumentation does **not** instrument [Node.js' fetch](https://nodejs.org/api/globals.html#fetch). See [this issue](https://github.com/open-telemetry/opentelemetry-js/issues/4333).)

## Installation

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
// safe enough
const OBSERVER_WAIT_TIME_MS = 300;

const isNode = typeof process === 'object' && process.release?.name === 'node';

export interface FetchCustomAttributeFunction {
(
span: api.Span,
Expand Down Expand Up @@ -465,6 +467,11 @@
* implements enable function
*/
override enable(): void {
if (isNode) {
// Node.js v18+ *does* have a global `fetch()`, but this package does not
// support instrumenting it.
return;

Check warning on line 473 in experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

View check run for this annotation

Codecov / codecov/patch

experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts#L473

Added line #L473 was not covered by tests
pichlermarc marked this conversation as resolved.
Show resolved Hide resolved
}
if (isWrapped(fetch)) {
this._unwrap(_globalThis, 'fetch');
this._diag.debug('removing previous patch for constructor');
Expand All @@ -476,6 +483,9 @@
* implements unpatch function
*/
override disable(): void {
if (isNode) {
return;

Check warning on line 487 in experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

View check run for this annotation

Codecov / codecov/patch

experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts#L487

Added line #L487 was not covered by tests
}
this._unwrap(_globalThis, 'fetch');
this._usedResources = new WeakSet<PerformanceResourceTiming>();
}
Expand Down