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

feat(instrumentation-undici): add undici and fetch instrumentations #4393

Conversation

david-luna
Copy link
Contributor

@david-luna david-luna commented Dec 28, 2023

Which problem is this PR solving?

This PR adds a new instrumentation that subscribes to the diagnostic channels used by undici pacakge. The instrumentation will get traces from:

  • undici package
  • global fetch API since this is implemented using undici internally and publishes the same messages on the same diagnostic channels.

Code was donated by @djMax from https://github.com/gas-buddy/service/blob/main/src/telemetry/fetchInstrumentation.ts

NOTE: the PR is marked as draft since there are some todos (to be discussed in comments)

  • wether to have a configuration similar to the HTTP one
  • make sure is not mixed with HttpInstrumentation
  • differentiate between a req made with fetch or using undici explicitly? decided it's not necessary (HTTP instrumentation doesn't for http livs like axios, got,...)
  • adjust node versions supported

Fixes #4333

Short description of the changes

  • add a new package with an instrumentation for undici

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

By unit tests.

Checklist:

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

@david-luna
Copy link
Contributor Author

1st discussion point

wether to have a configuration similar to the HTTP one

IMHO I think we should have the configuration that applies to fetch & undici which is the one concerning outgoing requests. You can check the options in https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/README.md?plain=1#L51

@david-luna
Copy link
Contributor Author

about 3rd point

differentiate between a req made with fetch or using undici explicitly?

I think it could be misleading if a customer is using fetch but the traces extracted refer only to undici. We may check if there is info on the request that could help us to figure it out or otherwise instrument undici's dispatch method to add this info on the request.

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Merging #4393 (092b9c1) into main (f4b681d) will decrease coverage by 1.87%.
Report is 37 commits behind head on main.
The diff coverage is n/a.

❗ Current head 092b9c1 differs from pull request most recent head db97899. Consider uploading reports for the commit db97899 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4393      +/-   ##
==========================================
- Coverage   92.22%   90.36%   -1.87%     
==========================================
  Files         333      163     -170     
  Lines        9459     3799    -5660     
  Branches     2009      845    -1164     
==========================================
- Hits         8724     3433    -5291     
+ Misses        735      366     -369     

see 177 files with indirect coverage changes

@louislatreille-cb
Copy link

@david-luna Seems like you have done a lot of work so far!

I've just started using OpenTelemetry (which is awesome) and stumbled upon this issue in our project: #4247. I'd be happy to help test the new instrumentations whenever you need help.

@david-luna
Copy link
Contributor Author

@david-luna david-luna closed this Feb 22, 2024
@david-luna david-luna deleted the feat/4333-add-undici-and-fetch-instrumentations branch February 22, 2024 15:44
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.

Add instrumentation for fetch API
2 participants