Allow module to be passed directly into plugins / bypass module hooks

[mattfysh]

Is your feature request related to a problem? Please describe

I use esbuild to create a JS bundle before deploying the bundle into a Docker container. This helps to keep build times and artifact sizes to a minimum. This results in no node_modules directory at the location where my nodejs process is running.

As a result, all instrumentations that use the (for example) new InstrumentationNodeModuleDefinition will not work, as there is no interaction with the module system at runtime.

I suspect that this will also be true for the new ESM support which is yet to land: #2640

Describe the solution you'd like to see

Allow the module to be passed directly into the plugin constructor, e.g.

new PgInstrumentation({
  moduleExports: require('pg')
})

Describe alternatives you've considered

Exclude all node_modules from bundling step, and install them inside the docker container, however this results in significantly higher artefact sizes due to the presence of node_modules

Additional context

[vmarchaud]

I think its worth to be added within the base instrumentation class, moving to the core repo

[Flarna]

It's not that uncommon that an instrumentation hooks an internal file of a module because it has to patch some internal functions (see here for an example).

As a result passing the fully imported module to the instrumentation will only work for some instrumentations (like pg).

[joelmukuthu]

Hi @mattfysh, did you find any other workarounds besides marking all instrumented modules as externals for esbuild?

[mattfysh]

Unfortunately no - the OpenTelemetry framework is currently tightly coupled with the module system, which creates these types of problems for bundled JS.

It would certainly be great to have this new feature soon - as it is becoming a burden having to manage two sets of dependency versions (one where the code is written, and another where deployed with OTel)

[joelmukuthu]

Thanks for responding. In case it helps, I navigated the maintenance burden by putting the external dependencies in a workspace (Yarn or NPM) and then installing them via npm install --prefix ./the/workspace/. At least that way they are also managed by the package manager.

[github-actions]

This issue 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.

[polRk]

+1 If I build my application into a bundle (eg esbuild ) then autoinstrumentation doesn't work.

[drewcorlin1]

+1 I'm also running into this issue.

For some added confusion, I develop locally with ts-node. When running locally via ts-node modules are resolved in a way that does work with auto instrumentation. With that flow, it wasn't until I attempted to deploy into my dev environment that I ran into issues (and went down quite a rabbit hole before understanding the real cause). I would love a solution like suggested in the description, since bundling with esbuild is core to my workflow and marking dependencies as external is a maintenance burden that will likely prevent adoption, despite how much I want to instrument my projects with OTel.

[Flarna]

I guess support for ts-node or esbuild would require hooks in their loaders first. These hooks could then be used by the instrumentation package here.

[drewcorlin1]

I guess support for ts-node or esbuild would require hooks in their loaders first. These hooks could then be used by the instrumentation package here.

Yeah I'm hopeful that is possible with the existing esbuild plugin API, but I'm no expert on that. I use fastify + esbuild, so I'm trying to see if I can get that instrumentation package working as a proof of concept. I'll keep this thread updated if I make any progress there

[drewcorlin1]

I have a very hacky example of this working with esbuild (just for fastify and pino, but it could be expanded to other packages) that I want to share in case anyone else comes across this/in case anyone has advice. Let me know if I should take this to a new issue. This configures the fastify and pino instrumentations using the esbuild plugin API. It's very hacky, and the things I want to improve are

1. Consider a better way to modify the code (avoid writing TS in strings, avoid .replace() on the original source for the reasons commented below). Maybe parse it into an AST? Will think more on this
2. Figure a way to pass configs to the instrumentations, without writing even more TS in a string
3. Test instrumentation packages that return multiple instrumentations from init()

I'll update this thread as I have improvements/figure out the points above

// esbuild.ts
import { build, PluginBuild } from 'esbuild';
import { readFile } from 'fs/promises';

function createApplyPatchesLogic(
  instrumentationPackage: string,
  instrumentationName: string,
  instrumentationConstructorArgs: string | null,
  preInstrumentedVariable: string,
  instrumentedVariable: string,
  imitateEsmModule?: boolean,
) {
  return `
const { ${instrumentationName} } = require('${instrumentationPackage}');
const instrumentations = new ${instrumentationName}(${instrumentationConstructorArgs ?? ''}).init();
// TODO: Get rid of this check, but also ensure it does what we want when there are multiple instrumentations
if (instrumentations.length > 1) throw new Error('Cannot handle multiple ${preInstrumentedVariable} instrumentations');

let ${instrumentedVariable} = ${preInstrumentedVariable};
for (const instrumentation of instrumentations) {
  ${instrumentedVariable} = instrumentation.patch(${
    imitateEsmModule ? `{ ${preInstrumentedVariable}: ${instrumentedVariable} }` : instrumentedVariable
  });
}
`;
}

const instrumentedFastifyName = 'instrumentedFastify';

const instrumentedFastifyExports = `
${createApplyPatchesLogic(
  '@opentelemetry/instrumentation-fastify',
  'FastifyInstrumentation',
  null,
  'fastify',
  instrumentedFastifyName,
  true,
)}
module.exports = ${instrumentedFastifyName};
module.exports.fastify = ${instrumentedFastifyName};
module.exports.default = ${instrumentedFastifyName};`;

function generateFastifyInstrumentation(originalSource: string): string {
  // NB: This depends on all the fastify exports being together in the file, which is currently the case for fastify
  // but is not a safe assumption for all modules (ie pino);
  // TODO: Consider finding each export by itself, and think about it that has any more significant runtime cost
  return originalSource.replace(
    /module\.exports = fastify\n *module.exports.fastify = fastify\n *module.exports.default = fastify/,
    instrumentedFastifyExports,
  );
}

function loadFastify(build: PluginBuild) {
  build.onLoad({ filter: /fastify\/fastify.js$/ }, async () => {
    const resolved = await build.resolve('./fastify', {
      kind: 'require-call',
      resolveDir: './node_modules',
    });

    const contents = await readFile(resolved.path);
    return {
      contents: generateFastifyInstrumentation(contents.toString()),
      resolveDir: './node_modules/fastify',
    };
  });
}

const instrumentedPinoName = 'instrumentedPino';

function generatePinoInstrumentation(originalSource: string): string {
  return (
    originalSource
      // TODO: This depends on the order of export statements, which would change
      .replace(
        /module.exports = pino/,
        `${createApplyPatchesLogic(
          '@opentelemetry/instrumentation-pino',
          'PinoInstrumentation',
          `{
            logHook: (span, record) => {
              // Reformat the injected log fields to use camelCase, eg. trace_id -> traceId
              const context = span.spanContext();
              record.traceId = context.traceId;
              record.spanId = context.spanId;
              record.strTraceFlags = context.traceFlags;

              if (record.trace_id === context.traceId) delete record.trace_id;
              if (record.span_id === context.spanId) delete record.span_id;
              if (Number(record.trace_flags) === context.traceFlags) delete record.trace_flags;
            },
          }`,
          'pino',
          instrumentedPinoName,
        )}
        module.exports = ${instrumentedPinoName};
    `,
      )
      .replace(/module.exports.default = pino/, `module.exports.default = ${instrumentedPinoName}`)
      .replace(/module.exports.pino = pino/, `module.exports.pino = ${instrumentedPinoName}`)
  );
}

function loadPino(build: PluginBuild) {
  build.onLoad({ filter: /pino\/pino.js$/ }, async () => {
    const resolved = await build.resolve('./pino', {
      kind: 'require-call',
      resolveDir: './node_modules',
    });

    const contents = await readFile(resolved.path);
    return {
      contents: generatePinoInstrumentation(contents.toString()),
      resolveDir: './node_modules/pino',
    };
  });
}

build({
  entryPoints: ['src/server.ts'],
  bundle: true,
  outfile: 'dist/server.js',
  target: 'node18',
  platform: 'node',
  sourcemap: true,
  plugins: [
    {
      name: 'open-telemetry',
      setup(build) {
        loadFastify(build);
        loadPino(build);
      },
    },
  ],
}).catch(err => {
  throw err;
});

To run ts-node esbuild.ts in a project that uses fastify and pino.