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

coordinate first pass at ESM support for all instrumentations #1942

Open
5 of 37 tasks
trentm opened this issue Feb 15, 2024 · 4 comments
Open
5 of 37 tasks

coordinate first pass at ESM support for all instrumentations #1942

trentm opened this issue Feb 15, 2024 · 4 comments
Assignees
Labels
type:feature-tracking A feature with sub-issues that need to be addressed

Comments

@trentm
Copy link
Contributor

trentm commented Feb 15, 2024

This is a meta issue to coordinate doing a first pass at ECMAScript module (ESM) support and a test for every node.js instrumentation. (This is about working with the import-in-the-middle-based hook so is not relevant to browser instrumentations.) I'm not necessarily volunteering to do all these :), but I'll certainly help.

Hopefully this can also be useful for support/help requests for why instrumentation might not be working for ESM-using code. If the module/instrumentation in question is not checked off here, then ESM instrumentation is not expected to work (it might be luck).

  • instrumentation-grpc (core repo)
  • instrumentation-http (core repo)
  • instrumentation-amqplib
  • instrumentation-cucumber
  • instrumentation-dataloader
  • instrumentation-fs
  • instrumentation-lru-memoizer
  • instrumentation-mongoose
  • instrumentation-socket.io
  • instrumentation-tedious
  • instrumentation-aws-lambda
  • instrumentation-aws-sdk
  • instrumentation-bunyan
  • instrumentation-cassandra
  • instrumentation-connect
  • instrumentation-dns
  • instrumentation-express (test)
  • instrumentation-fastify (test; there is an ESM limitation, see below)
  • instrumentation-generic-pool
  • instrumentation-graphql
  • instrumentation-hapi (support + test)
  • instrumentation-ioredis (support, test)
  • instrumentation-knex
  • instrumentation-koa (support + test)
  • instrumentation-memcached
  • instrumentation-mongodb
  • instrumentation-mysql
  • instrumentation-mysql2
  • instrumentation-nestjs-core
  • instrumentation-net
  • instrumentation-pg (support, missing test)
  • instrumentation-pino (support, test was accidentally partially removed, so need to restore that, test re-added)
  • instrumentation-redis
  • instrumentation-redis-4
  • instrumentation-restify
  • instrumentation-router
  • instrumentation-winston

dev notes

Run the following to show which (contrib-repo) instrumentations have an ESM-related test.
This shows usage of the required experimental-loader hook to support hooking ESM modules.

rg hook.mjs -g '*.test.*'

Guide on adding ESM support and an ESM test to an instrumentation

TODO(trentm)

@trentm trentm self-assigned this Feb 15, 2024
@trentm
Copy link
Contributor Author

trentm commented Feb 15, 2024

notes on fastify ESM support

Fastify ESM support is slightly limited. The current import style in "opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-fastify/test/fixtures/use-fastify.mjs" works, but this fails:

...
import * as mod from 'fastify';
console.log('mod: ', mod);
const app = mod.fastify();
...

Here mod.default is instrumented, but mod.fastify is not.
This is probably an exceedingly rare way to import and use fastify. Still,
this is why I haven't checked fastify above.

@tobiasmuehl
Copy link

tobiasmuehl commented Feb 17, 2024

According to this post, the following already work, based likely on luck:

  • graphql
  • @grpc/grpc-js

Your checklist only supports binary display - either fully working or not working at all. How about adding emojis to the list to signal the current implementation status? States could be:

  • ❓ Unknown: The ESM support status for the module has not yet been determined.
  • ❌ Not Supported: ESM is known to not be supported at all.
  • ⚠️🔧 Partially Supported: ESM is supported to some extent, but there are known limitations or issues that need addressing.
  • ✅🚫 Missing Tests: ESM is supported, but tests are missing or incomplete.
  • ✅ Fully Supported: ESM is fully supported and has been thoroughly tested.

@tobiasmuehl
Copy link

tobiasmuehl commented Feb 18, 2024

🎉 I found a working solution here! My project is using esm and I switched my runtime from node to jiti and NOW EVERYTHING JUST WORKS!!!!!! I'm using http, express and pg instrumentations. Works on node18 and node20

If we are living in a simulation then this is a complete cheat code 🥷🏼

@trentm
Copy link
Contributor Author

trentm commented Feb 22, 2024

Your checklist only supports binary display ...

Heh. I don't object to more info being included in each bullet point. I'd like to keep the checkboxes. To me "checked" will mean "we've done a first pass on ESM instrumentation for this module". When everything is checked, we can close this issue. There could be separate follow-up issues for particular limitations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature-tracking A feature with sub-issues that need to be addressed
Projects
None yet
Development

No branches or pull requests

3 participants