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

Update fetch instrumentation to be runtime agnostic #4063

Merged

Conversation

drewcorlin1
Copy link
Contributor

@drewcorlin1 drewcorlin1 commented Aug 15, 2023

Which problem is this PR solving?

This PR addresses #3413, but updating the experimental fetch instrumentation to not depend on browser specific constructs (window, location).

The motivation for this change is so I can instrument my NodeJS applications using fetch() with the @opentelemetry/instrumentation-fetch package.

My take is that this is much more of a bandaid than a true fix, as the fetch instrumentation still depends on the opentelemetry-sdk-trace-web, but it provides significant value in that I can use the instrumentation out of the box with no workarounds (like patching the global object globalThis.location = {}). I think the proper fix is to move some of the logic in opentelemetry-sdk-trace-web to opentelemetry-sdk-trace-base, but I am very new to this codebase and wanted to hold off on a larger refactor, in the interest of being able to contribute a targeted fix as quickly as possible.

Fixes # (issue)

Short description of the changes

Check for the existence of browser-specific constructs before referencing them, if they are not present return undefined/don't evaluate the expression.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have run the tests and linting steps, and also tested this locally in a NodeJS application (fastify server that uses fetch).

My steps to test locally

  • For both experimental/packages/opentelemetry-instrumentation-fetch and packages/opentelemetry-sdk-trace-web
  • cd into that directory
  • Build the package npm run compile
  • Package it npm pack
  • Install that tarball in my application npm i ../../Desktop/opentelemetry-js/<path to tarball>.tgz
  • Run the application and jaeger locally
  • Cause it to make a fetch call
  • See spans show up from the fetch instrumentation

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@drewcorlin1 drewcorlin1 requested a review from a team August 15, 2023 13:41
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 15, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@drewcorlin1
Copy link
Contributor Author

Working on getting the CLA set up for my organization now

@drewcorlin1
Copy link
Contributor Author

Ah I had only run npm run test, not all the other test suites. I see those failures locally now and will start addressing them

@drewcorlin1 drewcorlin1 marked this pull request as draft August 18, 2023 11:47
@drewcorlin1 drewcorlin1 force-pushed the note-fetch-instrumentation branch 3 times, most recently from f91b440 to 8b77815 Compare August 20, 2023 17:16
@drewcorlin1
Copy link
Contributor Author

drewcorlin1 commented Aug 20, 2023

I've updated this PR to be a much smaller fix, avoiding any refactor. I've updated the description accordingly.

@drewcorlin1 drewcorlin1 marked this pull request as ready for review August 20, 2023 20:16
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #4063 (777d51b) into main (10f6c46) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 777d51b differs from pull request most recent head 421f01e. Consider uploading reports for the commit 421f01e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4063      +/-   ##
==========================================
+ Coverage   92.31%   92.33%   +0.02%     
==========================================
  Files         330      329       -1     
  Lines        9421     9382      -39     
  Branches     1996     1985      -11     
==========================================
- Hits         8697     8663      -34     
+ Misses        724      719       -5     
Files Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 96.53% <100.00%> (+0.02%) ⬆️
packages/opentelemetry-sdk-trace-web/src/utils.ts 93.67% <100.00%> (+0.07%) ⬆️

... and 2 files with indirect coverage changes

Copy link

@pckilgore pckilgore left a comment

Choose a reason for hiding this comment

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

This seems much saner (especially in the short term) than monkey patching globalThis, which is what I'm currently doing to get fetch telemetry!

@drewcorlin1 drewcorlin1 force-pushed the note-fetch-instrumentation branch from 8b77815 to 03cecdd Compare September 29, 2023 22:17
@drewcorlin1 drewcorlin1 force-pushed the note-fetch-instrumentation branch 2 times, most recently from 531fb38 to e055b21 Compare October 14, 2023 14:17
@drewcorlin1 drewcorlin1 force-pushed the note-fetch-instrumentation branch from e055b21 to 42c71c9 Compare October 18, 2023 12:49
@drewcorlin1 drewcorlin1 force-pushed the note-fetch-instrumentation branch from 42c71c9 to d24e132 Compare October 18, 2023 12:52
@diogotorres97
Copy link

@pckilgore @drewcorlin1 @pichlermarc any news on this?

@drewcorlin1
Copy link
Contributor Author

@diogotorres97 just waiting for a codeowner to review

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.

LGTM % changelog text; I think this looks like a good step forward. 🙂 Thank you for working on this.

A side note: as you mentioned I also think some of the code needs to probably be moved in a follow up. We've recently started working on SDK 2.0 over at the next branch, so now is a good time to get breaking changes in if they're required. If you have specific action-items in mind, please open an issue so that we can plan it accordingly. 🙂

If the change requires breaking a stable interface, please tag @open-telemetry/javascript-approvers on the issue and state that it is a breaking change that should be considered for 2.0, we'll then review the proposal and add it to the scope for 2.0 if it is accepted.

CHANGELOG.md Outdated Show resolved Hide resolved
@drewcorlin1
Copy link
Contributor Author

@pichlermarc could you merge whenever you get a chance?

@pichlermarc pichlermarc merged commit 5ed54c8 into open-telemetry:main Nov 15, 2023
17 checks passed
dyladan added a commit that referenced this pull request Nov 15, 2023
* chore: track package-lock.json (#4238)

* chore: track package-lock.json

* Pin to old versions for node 14

* Use version range

* Remove unused cached directories

* Temporarily disable other tests

* Temporarily enable only api test

* Enable only some packages

* Test only api packages

* Test trace exporters

* Fix line ordering

* Test all packages except otlp exporters

* Add trace http exporter

* Add trace proto exporter

* Test all but grpc exporters

* chore: use npm workspaces and degrade lerna to v6

* chore: get rid of lerna bootstrap

* chore: use npx

* chore: allow install scripts to setup buf

* chore: fix w3c-integration-test cache key

* chore: fix cache key

* chore: disable resource compat test

* chore: fix node_modules assumptions

* chore: fix hoisted karma issue

* chore: fix markdown linter complaints

* chore: lock @grpc/grpc-js to v1.8.21

* Break caches

* chore: remove cache

* chore: fixup inline commands

---------

Co-authored-by: Daniel Dyla <[email protected]>

* docs: fixed link to benchmark results (#4233)

Co-authored-by: Chengzhong Wu <[email protected]>

* chore(deps): update all patch versions (#4215)

* fix: otlp json encoding (#4220)

Co-authored-by: Marc Pichler <[email protected]>

* fix: remove duplicate export star from version.ts (#4225)

Co-authored-by: Marc Pichler <[email protected]>

* docs: fix sdk-node config instructions (#4249)

Co-authored-by: Marc Pichler <[email protected]>

* feat(api): publish api esnext target (#4231)

* chore: release API 1.7.0/Core 1.18.0/Experimental 0.45.0 (#4254)

* fix(sdk-metrics): hand-roll MetricAdvice type as older API versions do not include it (#4260)

* chore: prepare release 1.18.1/0.45.1 (#4261)

* chore: no need for 'packages' in "lerna.json" (#4264)

* Benchmark tests for trace OTLP transform and BatchSpanProcessor (#4218)

Co-authored-by: Marc Pichler <[email protected]>

* chore: type reference on zone.js (#4257)

Co-authored-by: Marc Pichler <[email protected]>

* docs: add docker-compose to run prometheus for the experimental example (#4268)

Co-authored-by: Marc Pichler <[email protected]>

* fix(sdk-logs): avoid map attribute set when count limit exceeded (#4195)

Co-authored-by: Marc Pichler <[email protected]>

* chore(deps): update dependency chromedriver to v119 [security] (#4280)

* chore(deps): update actions/setup-node action to v4 (#4236)

* fix(sdk-trace-base): processor onStart called with a span having empty attributes (#4277)

Co-authored-by: artahmetaj <[email protected]>

* Update fetch instrumentation to be runtime agnostic (#4063)

Co-authored-by: Marc Pichler <[email protected]>

---------

Co-authored-by: Chengzhong Wu <[email protected]>
Co-authored-by: Martin Kuba <[email protected]>
Co-authored-by: Mend Renovate <[email protected]>
Co-authored-by: Siim Kallas <[email protected]>
Co-authored-by: Marc Pichler <[email protected]>
Co-authored-by: David Luna <[email protected]>
Co-authored-by: Dinko Osrecki <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: François <[email protected]>
Co-authored-by: Hyun Oh <[email protected]>
Co-authored-by: André Cruz <[email protected]>
Co-authored-by: artahmetaj <[email protected]>
Co-authored-by: drewcorlin1 <[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.

4 participants