instrumentation: respect multiple instrumentation module definitions with different supported versions

[seemk]

What version of OpenTelemetry are you using?

0.18.2

What version of Node are you using?

v14.16.0

This came up when isntrumenting pino (open-telemetry/opentelemetry-js-contrib#432), the idea was to support multiple versions of pino, so I set up multiple definitions corresponding to each pino version:

class PinoInstrumentation extends InstrumentationBase {

protected init() {
  return [
    new InstrumentationNodeModuleDefinition<Pino6>(
      'pino',
      ['6.x'],
      pinoModule => {
        /* patch, cut for brevity */
        return pinoModule;
      },
     ()=> { /* unpatch */ },
    ),
    new InstrumentationNodeModuleDefinition<Pino5>(
      'pino',
      ['5.x'],
      pinoModule => {
        /* patch, cut for brevity */
        return pinoModule;
      },
      ()=> { /* unpatch */ },
    ),
  ];
}

}

This however does not work due to faulty logic in InstrumentationBase._isSupported (https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts#L62) because onRequire hook will iterate over ALL of the module definitions: suppose I am hooking into pino 5, InstrumentationBase registers 2 hooks - 1 for each definition. When pino is required, the hook for pino 6 module definition fires first, isSupported iterates over all module definitions and does the check against pino 5 supported versions, meaning that the patch function for pino 6 definition will be called even though I have required pino 5.

The workaround is to directly use semver in the patch callbacks: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/432/files#diff-660728f57a3c2b22336bc39c10f17f768ce60fc9d3e706f7a648e61dc0ebf35bR63-R77

The fix for this is to omit iterating over all modules at onRequire hook, but use the same module passed to onRequire instead.

[dyladan]

This came up when isntrumenting pino (open-telemetry/opentelemetry-js-contrib#432), the idea was to support multiple versions of pino, so I set up multiple definitions corresponding to each pino version:

Thanks for bringing this up because that was sort of the whole reason for having multiple definitions in the first place :) (also to support things like http and https in the same instr)

[obecny]

on it

[dyladan]

on it

He already opened the fix #2120

[obecny]

on it

He already opened the fix #2120

I see