Skip to content

Add diagnostics_channel based instrumentation to supported Node libraries #15107

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

Open
mydea opened this issue Jan 21, 2025 · 0 comments
Open

Comments

@mydea
Copy link
Member

mydea commented Jan 21, 2025

Description

Today, we use import-in-the-middle and require-in-the-middle based instrumentation for Node libraries in our Node SDK, e.g. for express, mysql, etc.

This has some downsides:

  1. For ESM, to have import-in-the-middle work reliably it should be configured via --import in the Node CLI
  2. import-in-the-middle is flawed in a couple of ways that means it can't be used to wrap all libraries
  3. Neither import-in-the-middle or require-in-the-middle work with bundled code (as it instruments import/require, basically)

To "fix" this, we want to migrate to using diagnostics_channel for the packages we instrument. With this, both of these downsides disappear.

In order to achieve this, we need to actually PR the libraries themselves (e.g. express, mysql, ...) and make them emit diagnostics_channel (or a tracing channel). Then, the OTEL instrumentation can be updated to use this instead of using import-in-the-middle.

This will be a gradual approach, we can update libraries as we go, starting with more important ones.

### Libraries to update
- [ ] http
- [ ] express
- [ ] fastify
- [ ] graphql
- [ ] mongo
- [ ] mongoose
- [ ] mysql
- [ ] mysql2
- [ ] redis
- [ ] postgres
- [ ] hapi
- [ ] koa
- [ ] connect
- [ ] tedious
- [ ] genericPool
- [ ] kafka
- [ ] amqlib
- [ ] lruMemoizer
- [ ] vercelAIIntergation (?)
AbhiPrasad pushed a commit that referenced this issue Feb 20, 2025
The goal is for Express to eventually support publishing events to
`diagnostics_channel` (#15107) so that Open Telemetry can instrument it
without monkey-patching internal code. However, this might take a while
and it would be great to support Express v5 now. This PR is a stop-gap
solution until that work is complete and published.

This PR vendors the code added in my otel PR: 
- open-telemetry/opentelemetry-js-contrib#2437 
- Adds a new instrumentation specifically for hooking express v5
- Copies the Express v4 integration tests to test v5
- The only changes in the tests is the removal of a couple of complex
regex tests where the regexes are no longer supported by Express.
- Modifies the NestJs v11 tests which now support full Express spans
@linear linear bot added the SDKs (New) label Apr 7, 2025
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

No branches or pull requests

2 participants