opentelemetry-js-contrib icon indicating copy to clipboard operation
opentelemetry-js-contrib copied to clipboard

coordinate first pass at ESM support for all instrumentations

Open trentm opened this issue 1 year ago • 4 comments

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)
  • [x] instrumentation-http (core repo, support + test)
  • [ ] 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
  • [x] instrumentation-express (test)
  • [ ] instrumentation-fastify (test; there is an ESM limitation, see below)
  • [ ] instrumentation-generic-pool
  • [ ] instrumentation-graphql
  • [x] instrumentation-hapi (support + test)
  • [x] instrumentation-ioredis (support, test)
  • [ ] instrumentation-knex
  • [x] instrumentation-koa (support + test)
  • [ ] instrumentation-memcached
  • [ ] instrumentation-mongodb
  • [ ] instrumentation-mysql
  • [ ] instrumentation-mysql2
  • [ ] instrumentation-nestjs-core
  • [ ] instrumentation-net
  • [ ] instrumentation-pg (support, missing test)
  • [x] 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)

  • https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1735 added runTestFixture

trentm avatar Feb 15 '24 01:02 trentm

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.

trentm avatar Feb 15 '24 01:02 trentm

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 avatar Feb 17 '24 07:02 tobiasmuehl

🎉 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 🥷🏼

tobiasmuehl avatar Feb 18 '24 06:02 tobiasmuehl

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.

trentm avatar Feb 22 '24 22:02 trentm