feat(instrumentation): add support for esm module

[vmarchaud]

trafficstars

Re-opening following https://github.com/open-telemetry/opentelemetry-js/pull/2640 + https://github.com/open-telemetry/opentelemetry-js/pull/2763

I'm marking this as draft since we have a semi problem with this: its really not easy to test this. Granted there wasn't a lot of test in place before that for require in the middle but i think it make sense to do it anyway. I've looked around and if we want to run in esm mode and verify that we can correctly hook esm module, we need to mark the package itself as being ESM: https://github.com/mochajs/mocha-examples/issues/47#issuecomment-952339528 I'm pretty sure doing this will cause downstream problem for users so i don't think this is the way to go, do someone have an idea ?

[codecov[bot]]

Codecov Report

Merging #2846 (992c18c) into main (d1621d2) will increase coverage by 0.41%. The diff coverage is n/a.

:exclamation: Current head 992c18c differs from pull request most recent head f6384aa. Consider uploading reports for the commit f6384aa to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2846      +/-   ##
==========================================
+ Coverage   93.26%   93.67%   +0.41%     
==========================================
  Files         247      242       -5     
  Lines        7346     7181     -165     
  Branches     1512     1477      -35     
==========================================
- Hits         6851     6727     -124     
+ Misses        495      454      -41     

Impacted Files	Coverage Δ
...n/src/platform/node/RequireInTheMiddleSingleton.ts
...src/platform/node/instrumentationNodeModuleFile.ts
...nstrumentation/src/platform/node/ModuleNameTrie.ts
...atform/node/instrumentationNodeModuleDefinition.ts
...strumentation/src/platform/node/instrumentation.ts

[Flarna]

Why is it needed to mark @opentelemetry/instrumentation as ESM? Is it needed only for our tests or also for production? I think datadog APM uses import-in-the-middle but I don't think it's marked as module.

[vmarchaud]

Is it needed only for our tests or also for production?

Apparently for tests only but i didn't test it yet

[vmarchaud]

Added a simple test that patches a esm module and verify that we get the patched version, however making it work with our typescript tooling was pretty much a nightmare. Would like input of everyone to discuss if we ship it like this or we want to do more ?

PS: Wil fix tests for older node version / changelog after settling above comment

cc @open-telemetry/javascript-approvers

[vmarchaud]

I've fixed the tests (sorry for the bash hack, i didn't see another way to not run those tests for node < 12) and added a comment in the readme to explain that user need to use choose the iitm loader so we can hook into modules

[vmarchaud]

Have you tried running any contrib package against this?

Well i believe most of our instrumentation target cjs module so i didn't try, do you have one in mind that have a esm bundle ?

I love a good bash hack(sarcasm), but if you don't, an less-bash-hacky alternative could be something like:

I prefer your suggestion, i will update my PR

[dyladan]

Dismissed my approval because we need to make sure a basic npm test works for windows users. This means either making test:esm work in windows, only running test:cjs for a basic test script, or both.

[rauno56]

Well i believe most of our instrumentation target cjs module so i didn't try, do you have one in mind that have a esm bundle ?

I assumed the new paths will also kick in if my application is esm, but not the library. Isn't that the case?

[github-actions[bot]]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[dyladan]

This PR is not stale

[vmarchaud]

FYI @JamieDanielson offered to move this PR forward in slack's dm (as i find it quite hard to commit time on otel those days, it seems that it might go forward faster this way)

[dyladan]

Thanks Jamie

[JamieDanielson]

FYI @JamieDanielson offered to move this PR forward in slack's dm (as i find it quite hard to commit time on otel those days, it seems that it might go forward faster this way)

Sorry for the delay, some different tasks have come up recently but I plan to dig into this more meaningfully to address the open questions/comments during the first week of August

[github-actions[bot]]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[chmking]

Any update here? With Nuxt 3 moving to native ESM, this would be amazing.

[JamieDanielson]

Apologies for further delay on helping with this! Some other things had come up but this has moved back into the priority list now. Hope to have an update in the coming days/weeks, as this is definitely an important addition.

[vmarchaud]

All that activity over #1946 motivated me to take some time from my holiday to get back to this. Good news actually some of the issues i've had has been resolved by other PRs:

• https://github.com/open-telemetry/opentelemetry-js/pull/2846#discussion_r859026725: no longer needed with newer IITM version
• https://github.com/open-telemetry/opentelemetry-js/pull/3161 extracted the logic to a singleton so i've moved the logic there too
• https://github.com/open-telemetry/opentelemetry-js/pull/2846#discussion_r864258373 we dropped support for node 12 so the hack to run esm only when available isnt needed anymore

I assumed the new paths will also kick in if my application is esm, but not the library. Isn't that the case?

From my understanding, the new path for esm only kicks in when the ESM module loader is used so for ESM modules, you can find more information there: https://nodejs.org/dist/latest-v18.x/docs/api/packages.html#determining-module-system

[weyert]

Thank you for taking time of your holiday working on this. Let me know, how I can help. I think that agitated is a popular ESM package that could be a test target?

[vmarchaud]

Thank you for taking time of your holiday working on this. Let me know, how I can help. I think that agitated is a popular ESM package that could be a test target?

My simple unit test shows that you can mutate exports with the PR so yeah i think once merged it would be nice to have a esm-only instrumentation created

[weyert]

Thank you for taking time of your holiday working on this. Let me know, how I can help. I think that agitated is a popular ESM package that could be a test target?

My simple unit test shows that you can mutate exports with the PR so yeah i think once merged it would be nice to have a esm-only instrumentation created

I think the got package is an interesting test case as its a package that is widely used 11.8.5 which still CommonJS while the >= 12.0.0 became a ESM pure module. I think both versions are still heavily used

[jeremysf]

I gave this a PR try on node v18.12.1. I know it's totally work in progress, but just in case it's helpful, I thought I'd post my results here.

Short version: I'm not having much luck :/

$ node --experimental-loader=import-in-the-middle/hook.mjs dist/user.mjs
(node:17) ExperimentalWarning: Custom ESM Loaders is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
@opentelemetry/api: Registered a global for diag v1.3.0.
Exporter "otlp" requested through environment variable is unavailable.
@opentelemetry/api: Registered a global for trace v1.3.0.
@opentelemetry/api: Registered a global for context v1.3.0.
@opentelemetry/api: Registered a global for propagation v1.3.0.
@opentelemetry/instrumentation-express Module express has been loaded before @opentelemetry/instrumentation-express so it might not work, please initialize it before requiring express
@opentelemetry/instrumentation-grpc Module @grpc/grpc-js has been loaded before @opentelemetry/instrumentation-grpc so it might not work, please initialize it before requiring @grpc/grpc-js
@opentelemetry/instrumentation-mysql2 Module mysql2 has been loaded before @opentelemetry/instrumentation-mysql2 so it might not work, please initialize it before requiring mysql2
@opentelemetry/instrumentation-aws-sdk Module aws-sdk has been loaded before @opentelemetry/instrumentation-aws-sdk so it might not work, please initialize it before requiring aws-sdk
@opentelemetry/instrumentation-redis Module redis has been loaded before @opentelemetry/instrumentation-redis so it might not work, please initialize it before requiring redis
@opentelemetry/instrumentation-memcached Module memcached has been loaded before @opentelemetry/instrumentation-memcached so it might not work, please initialize it before requiring memcached
Patching [email protected]
Patching Connection.prototype.query
MySQL2Instrumentation: patched mysql query/execute
MySQL2Instrumentation: patched mysql query/execute
items to be sent [
  Span {
    attributes: {
      'db.system': 'mysql',
...

I'm using esbuild to bundle my TypeScript app's source code, but am excluding all node_modules from bundling (so I'm just bundling my app's transpiled TypeScript source code).

Despite the debug error logs above, I am not importing anything before loading the instrumentations as can be seen from the source of my bundled app:

import {createRequire} from 'module';const require=createRequire(import.meta.url);
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __export = (target, all) => {
  for (var name in all)
    __defProp(target, name, { get: all[name], enumerable: true });
};
var __decorateClass = (decorators, target, key, kind) => {
  var result = kind > 1 ? void 0 : kind ? __getOwnPropDesc(target, key) : target;
  for (var i = decorators.length - 1, decorator; i >= 0; i--)
    if (decorator = decorators[i])
      result = (kind ? decorator(target, key, result) : decorator(result)) || result;
  if (kind && result)
    __defProp(target, key, result);
  return result;
};

// backend/common/tracing/initialize.ts
import * as openTelemetryAPI from "../node_modules/@opentelemetry/api/build/src/index.js";
import { registerInstrumentations } from "../node_modules/@opentelemetry/instrumentation/build/src/index.js";
import { NodeTracerProvider } from "../node_modules/@opentelemetry/sdk-trace-node/build/src/index.js";
import { OTLPTraceExporter } from "../node_modules/@opentelemetry/exporter-trace-otlp-proto/build/src/index.js";
import { Resource } from "../node_modules/@opentelemetry/resources/build/src/index.js";
import { SemanticResourceAttributes } from "../node_modules/@opentelemetry/semantic-conventions/build/src/index.js";
import { SimpleSpanProcessor } from "../node_modules/@opentelemetry/sdk-trace-base/build/src/index.js";
import { GrpcInstrumentation } from "../node_modules/@opentelemetry/instrumentation-grpc/build/src/index.js";
import { HttpInstrumentation } from "../node_modules/@opentelemetry/instrumentation-http/build/src/index.js";
import { ExpressInstrumentation } from "../node_modules/@opentelemetry/instrumentation-express/build/src/index.js";
import { MySQL2Instrumentation } from "../node_modules/@opentelemetry/instrumentation-mysql2/build/src/index.js";
import { AmqplibInstrumentation } from "../node_modules/@opentelemetry/instrumentation-amqplib/build/src/index.js";
import { AwsInstrumentation } from "../node_modules/@opentelemetry/instrumentation-aws-sdk/build/src/index.js";
import { RedisInstrumentation } from "../node_modules/@opentelemetry/instrumentation-redis/build/src/index.js";
import { MemcachedInstrumentation } from "../node_modules/@opentelemetry/instrumentation-memcached/build/src/index.js";

// backend/common/tracing/config.ts
var serviceName = process.env["SERVICE_NAME"] ?? "unknown";
var tracingEnabled = process.env["TRACING_ENABLED"] === "true";
var tracingDebugLoggingEnabled = process.env["TRACING_DEBUG_LOGGING"] === "true";

// backend/common/tracing/initialize.ts
if (tracingEnabled) {
  if (tracingDebugLoggingEnabled) {
    openTelemetryAPI.diag.setLogger(new openTelemetryAPI.DiagConsoleLogger(), openTelemetryAPI.DiagLogLevel.DEBUG);
  }
  const provider = new NodeTracerProvider({
    resource: new Resource({
      [SemanticResourceAttributes.SERVICE_NAME]: serviceName
    })
  });
  provider.addSpanProcessor(new SimpleSpanProcessor(new OTLPTraceExporter()));
  provider.register();
  registerInstrumentations({
    instrumentations: [
      new HttpInstrumentation({
        enabled: true,
        ignoreIncomingRequestHook(req) {
          return !!req.url?.match(/chkhealth|chkready/);
        }
      }),
      new ExpressInstrumentation({ enabled: true }),
      new GrpcInstrumentation({ enabled: true }),
      new MySQL2Instrumentation({ enabled: true }),
      new AmqplibInstrumentation({ enabled: true }),
      new AwsInstrumentation({ enabled: true }),
      new RedisInstrumentation({ enabled: true }),
      new MemcachedInstrumentation({ enabled: true })
    ]
  });
}
...

I am seeing auto-instrumented traces in the debug logs of my app and in Jaeger for mysql and http, but I'm not seeing debug logs or traces for any of the other instrumentations.

I recently switched the app from CommonJS to ESM due to several of my dependencies dropping support for CommonJS. I previously saw traces in Jaeger, and then (unsurprisingly) stopped seeing traces after switching my app to ESM.

Anyway, thank you so much for working on this feature, and hopefully my experience is just user error and will work out of the box once this PR lands.

[vmarchaud]

Anyway, thank you so much for working on this feature, and hopefully my experience is just user error and will work out of the box once this PR lands.

I believe this show that my PR doesn't work when we try to patch CJS module from ESM (since most of instrumentation we are patching are still CJS). My test only does ESM loading ESM modules. I'm gonna try to reproduce when i've time but in the mean time:

• could you test to bundle all of your node_modules to ESM see if it's better ?
• make a small repro of your build so i can test against ?

Thanks for your help

[JamieDanielson]

I branched and created an example app to try some things out using HTTP and Express instrumentation. I don't know if it was all necessary, but in a few places I updated the package.json to help ensure I was using the local instrumentation package.

There are instructions in a README for running this app as CommonJS and as ESModules - it defaults to CommonJS. When I run as CommonJS, I see 5 total spans: 1 http, 3 express, 1 manual. When I run as ESM, I only see the manual span.

I notice these in the console output for CommonJS and not ESM:

@opentelemetry/instrumentation-http Applying patch for [email protected]
Applying patch for [email protected]

[JamieDanielson]

As discussed outside of this PR and in SIG meetings, here is the new PR for this work: https://github.com/open-telemetry/opentelemetry-js/pull/3698