fix: handling of multipleInstrumentationNodeModuleDefinition with the same name

[RafalSumislawski]

When there are multiple InstrumentationNodeModuleDefinitions the urrent implementation chooses one of theInstrumentationNodeModuleDefinitions in bunle-time and then in runtime selects it by name. This breaks when there are multiple InstrumentationNodeModuleDefinitions with the same name, as the wrong one may be selected.

This is the case with:
mongodb - https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts#L96
winston - https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-winston/src/instrumentation.ts#L48
socket.io - https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/instrumentation-socket.io/src/socket.io.ts#L218
generic-pool - https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-generic-pool/src/instrumentation.ts#L43

This PR approaches the problem analogically to how instrumentation.files are handled. By iterating over the instrumentation in runtime and applying the ones with matching version.

[drewcorlin1]

Don't we still need to check instrumentation.name for cases like the aws sdk which has InstrumentationNodeModuleDefinition's with many names?

• https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts#L101
• https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts#L129

[RafalSumislawski]

Indeed we do. I converted this PR to a draft exactly because I realized aws-sdk stopped working after these changes.

[drewcorlin1]

Shouldn't we throw the underlying error to make any issues with this visible to users without requiring them to set the OTel diag log level?

[RafalSumislawski]

My opinion is that bugs in instrumentation (especially automatic) shouldn't break the service. I would like to surface the errors message of course.

I'm not sure what's idiomatic across otel, but I think this is a similar case to how auto-instrumentations-node handles startup errors here: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/metapackages/auto-instrumentations-node/src/register.ts#L37

[drewcorlin1]

Good point + agree the consistency with auto instrumentations is good

[drewcorlin1]

@RafalSumislawski i'll merge if that works for you?

[RafalSumislawski]

Sure 👍

[drewcorlin1]

thanks again for the fix!