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

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Feb 21, 2024

A side issue from #4496 was the mistaken use of instrumentation-fetch for Node.js's fetch(). instrumentation-fetch does not intend to handle Node.js's fetch.

This change makes it so instrumentation-fetch will noop when used in Node.js.

Refs: #4496


Theoretically I think instrumentation-fetch should be able instrument Node.js fetch, but currently it is written with dependencies on a web otel package and its tests assume web/browser. See #4333 for discussion on instrumenting Node.js' fetch().

@trentm trentm self-assigned this Feb 21, 2024
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Merging #4498 (6d0aeb9) into main (89caef9) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4498      +/-   ##
==========================================
- Coverage   92.43%   92.40%   -0.03%     
==========================================
  Files         330      330              
  Lines        9525     9531       +6     
  Branches     2033     2036       +3     
==========================================
+ Hits         8804     8807       +3     
- Misses        721      724       +3     
Files Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 95.05% <50.00%> (-1.54%) ⬇️

@trentm trentm marked this pull request as ready for review February 21, 2024 19:03
@trentm trentm requested a review from a team February 21, 2024 19:03
@weyert
Copy link
Contributor

weyert commented Feb 21, 2024

There is this PR that is relevant:
#4393

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of that. 👍

@pichlermarc pichlermarc merged commit aff48a1 into open-telemetry:main Feb 23, 2024
19 of 20 checks passed
@trentm trentm deleted the tm-instr-fetch-refuse-node branch February 23, 2024 18:05
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…r is for web fetch only (open-telemetry#4498)

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

* add a changelog entry

* add a diagnostic warning if attempting to use instr-fetch in Node.js

* fixup! add a diagnostic warning if attempting to use instr-fetch in Node.js

---------

Co-authored-by: Marc Pichler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants