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.

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

Jump to bottom

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f0e1b15
fix(instr-fetch): do not enable in Node.js; clarify in docs this inst…
trentm Feb 21, 2024
4dc49ae
add a changelog entry
trentm Feb 21, 2024
6211772
add a diagnostic warning if attempting to use instr-fetch in Node.js
trentm Feb 22, 2024
cc8a0e5
fixup! add a diagnostic warning if attempting to use instr-fetch in N…
pichlermarc Feb 23, 2024
6d0aeb9
Merge branch 'main' into tm-instr-fetch-refuse-node
pichlermarc Feb 23, 2024
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 @@ -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:/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:/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:/open-telemetry/opentelemetry-js/pull/4498) @trentm

### :books: (Refine Doc)

Expand Down
3 changes: 2 additions & 1 deletion experimental/packages/opentelemetry-instrumentation-fetch/README.md
View file
Open in desktop
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:/open-telemetry/opentelemetry-js/issues/4333).)

## Installation

Expand Down
13 changes: 13 additions & 0 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 @@ -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,14 @@
* implements enable function
*/
override enable(): void {
if (isNode) {
// Node.js v18+ *does* have a global `fetch()`, but this package does not
// support instrumenting it.
this._diag.warn(

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
"this instrumentation is intended for web usage only, it does not instrument Node.js's fetch()"
);
return;

Check warning on line 476 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#L476 

Added line #L476 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 +486,9 @@
* implements unpatch function
*/
override disable(): void {
if (isNode) {
return;

Check warning on line 490 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#L490 

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