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

Add instrumentation for fetch API #4333

Closed
david-luna opened this issue Nov 29, 2023 · 8 comments
Closed

Add instrumentation for fetch API #4333

david-luna opened this issue Nov 29, 2023 · 8 comments
Assignees

Comments

@david-luna
Copy link
Contributor

david-luna commented Nov 29, 2023

Is your feature request related to a problem? Please describe.

NodeJS's global fetch API is not instrumented therefore we do not get traces for requests made with it and also the tracecontext is not propagated. The lack of this instrumentation may raise more issues like this one #4247 and switching from fetch to http or any packages which has an available instrumentation like undici may be a no go for the devs

There's a package @opentelemetry/instrumentation-fetch but is specific for web platform since its depending @opentelemetry/sdk-trace-web so we need an instrumetation that fits for nodejs' fetch. There are some questions though.

  • should we make the current instrumentation agnostic of the platform (web/node)?
  • if not how should we name this new package?
    • @opentelemetry/instrumentation-node-fetch could make users think is for node-fetch package
    • is there any way to deduct from the name if the package is targeted for web or node? or if its for instrumenting a package or a platform API?

Trying to answer myself to the 1st question I'd say we can not forecast what are the instrumentation needs for each platform. As an example we see in the current code that is taking in consideration CORS preflight requests which is very common in web but not so much in node. So I'd prefer to have a specific instrumentation for nodejs' fetch

About the naming I do not know if there is a convention so feedback from @dyladan or @pichlermarc would be appreciated :)

Refs: #4247

Describe the solution you'd like

Create a new instrumentation package for nodejs' fetch global API and place it in this repository. There is a mention in #3346 of a couple of instrumentations that we could take as a good stating point

IMO 2nd implementation is the way to go since fetch is hooking internally to undici and it won't detected by our require/import hooks. See section below.

Describe alternatives you've considered

  • Refactor the app to use http module or a package that could be instrumented.
  • install opentelemetry-instrumentation-undici to check if the internal module is instrumented

See #4247 (comment)

Additional context

@david-luna
Copy link
Contributor Author

@dyladan @pichlermarc could you assign this one to me? Thanks :)

@trentm
Copy link
Contributor

trentm commented Nov 30, 2023

@david-luna Perhaps clarify that this is about Node.js's global fetch(). IIUC https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-instrumentation-fetch/ instruments fetch() for web usage. I wonder if that instrumentation might be close to also being able to instrument Node.js's fetch(). When enabled it shims globalThis.fetch. That said, the undici diagnostics_channel events from Node's implementation of fetch() might provide more interesting details for tracing.

@david-luna
Copy link
Contributor Author

yeah, it was very short. I've updated with more info :)

@david-luna
Copy link
Contributor Author

I've reached the dev who shared his fetch implementation in the same conversation
#3346 (reply in thread)

I've try another channel if there is no answer in few days

@david-luna
Copy link
Contributor Author

@djMax thanks on answering on the question about donating the implementation of https://github.com/gas-buddy/service/blob/main/src/telemetry/fetchInstrumentation.ts to this repo.

This is the issue we're using to track it's progress. As discussed in the JavaScript SIG (see Dec 13 notes) the goal would be to have a new package named @opentelemetry/instrumentation-undici which will serve to instrument undici package and also the global fetch API. This new package would be placed within experimental/packages folder along with HTTP & GRPC instrumentations.

Tasks to be done

  • Port the code
  • Create tests for global fetch
  • Create tests for undici package
  • Add documentation

I can create a draft PR to start so you can give a 1st review or we could do it the other way around. Up to you. :)

@djMax
Copy link

djMax commented Dec 20, 2023

Happy to review a PR, and to try it out in a place with tests that verify it's function from a consumer perspective.

@trentm
Copy link
Contributor

trentm commented Feb 21, 2024

This was discussed in the OTel JS SIG today (https://docs.google.com/document/d/1tCyoQK49WVcE-x8oryZOTTToFm7sIeUhxFPm9g-qL1k/edit#heading=h.ry29ajruzxyg). It was concluded to move this instrumentation to the contrib repo.

@david-luna
Copy link
Contributor Author

Link to the contrib PR: open-telemetry/opentelemetry-js-contrib#1951

maryliag pushed a commit to maryliag/opentelemetry-js-contrib that referenced this issue Apr 9, 2024
…` and global `fetch` API (open-telemetry#1951)

Closes: open-telemetry/opentelemetry-js#4333
Co-authored-by: Jamie Danielson <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants