@opentelemetry/instrumentation-aws-lambda is not compatible with @opentelemetry/instrumentation@^0.34.0

[mhassan1]

• This only affects the JavaScript OpenTelemetry library
• This may affect other libraries, but I would like to get opinions here first

In open-telemetry/opentelemetry-js#3161, we started passing null to require-in-the-middle (RITM), which allows us to intercept all require calls with a single RITM instance; however, when null is passed, RITM no longer intercepts require calls to absolute paths. Since @opentelemetry/instrumentation-aws-lambda needs an absolute path to be intercepted, we cannot upgrade to @opentelemetry/instrumentation@^0.34.0. As far as I can tell, @opentelemetry/instrumentation-aws-lambda is the only instrumentation plugin that uses an absolute path.

We will need to resolve this one of these ways:

1. In require-in-the-middle, add support for absolute paths when null is passed
2. In @opentelemetry/instrumentation, handle this scenario by creating a new instance of require-in-the-middle, passing the absolute path
3. In @opentelemetry/instrumentation-aws-lambda, instrument code using another method, e.g. use require-in-the-middle directly, or patch the code directly

[dyladan]

We knew this when we updated the instrumentation. The lambda instrumentation will probably have to switch to using require-in-the-middle directly

[Flarna]

fyi @willarmiros

[Flarna]

Should we rate this as bug? If instrumentations require different versions of @opentelemetry/instrumentation every user of @opentelemetry/auto-instrumentations-node gets two instances of it.

[icco]

This is blocking our upgrade to 1.8.0 and 0.34. Do we have a sense of the amount of work this would take?

[Flarna]

I changed the label from discussion to bug.

We might consider to remove @opentelemetry/instrumentation-aws-lambda from @opentelemetry/auto-instrumentations-node to unblock users to upgrade.

[mhassan1]

Just for my understanding, why does this prevent users from upgrading? @opentelemetry/instrumentation-aws-lambda@0.34.0 has a peer dependency on @opentelemetry/api@^1.3.0 and a dependency on @opentelemetry/instrumentation@^0.32.0. The only downside I see is that there will need to be two versions of @opentelemetry/instrumentation at once; is that a blocker?

[Aneurysm9]

@Flarna can you help me understand the work required to fix this durably? I'm happy to help out and do the work, but I'm not familiar with RITM or how it's used here.

[mhassan1]

@Aneurysm9 I am working on a pull request now.

[dyladan]

I'm not sure I understand what the bug is? The user may get 2 versions of require in the middle but both would work fine. There would just be 2 wraps on require which would have a minor performance impact still much better than the 1 wrap per instrumentation we had before.

[icco]

@mhassan1 mostly out of caution. In the past, whenever we've seen multiple versions of opentelemetry packages we've seen issues where all observability disappears.

[mhassan1]

@dyladan Here are two possible resolutions to this issue:

open-telemetry/opentelemetry-js#3457
In @opentelemetry/instrumentation, handle this scenario by creating a new instance of require-in-the-middle, passing the absolute path

#1314
In @opentelemetry/instrumentation-aws-lambda, instrument code using another method, e.g. use require-in-the-middle directly, or patch the code directly

I think the first option is cleaner, since it avoids polluting instrumentation plugins with RITM logic and InstrumentationBase logic (e.g. to support instrumentation.disable and instrumentation.enable).

[dyladan]

I agree the first option is cleaner

[Flarna]

I'm not sure I understand what the bug is? The user may get 2 versions of require in the middle but both would work fine. There would just be 2 wraps on require which would have a minor performance impact still much better than the 1 wrap per instrumentation we had before.

Mainly based on the comment from @icco that it blocks updates.

I agree that having two instances of @opentelemetry/instrumentation should not harm. But we have seen in the past that typescript sometimes disagrees in this regard once classes are in the exported API.

I think the first option is cleaner

I vote also for option 1.