fix(aws-lambda-instrumentation): errors while instrumenting cause instrumented lambda to fail

[mzl-md]

Which problem is this PR solving?

Errors thrown by shimmer cause the instrumented lambda to fail.

Fixes #1287

Related to open-telemetry/opentelemetry-js#2260

Short description of the changes

Since the instrumentation should never cause the instrumented lambdas to fail, errors thrown by shimmer should be caught and logged.

I can reproduce the error but I don't really have an idea on how to fix this. @anuraaga could you help?

Output of the test with try-catch enabled:

npx mocha test/integrations/lambda-handler.moduleError.test.ts
lambda handler
handler with module.exports syntax
1) should not fail
no original to unwrap to -- has handler already been unwrapped?
0 passing (2s)
1 failing

1. lambda handler
   handler with module.exports syntax
   should not fail:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/ms/Repos/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-aws-lambda/test/integrations/lambda-handler.moduleError.test.ts)
   at listOnTimeout (node:internal/timers:581:17)
   at processTimers (node:internal/timers:519:7)

Without try-catch:

npx mocha test/integrations/lambda-handler.instrumentationError.test.ts
lambda handler
handler with module.exports syntax
1) should not fail
no original to unwrap to -- has handler already been unwrapped?
0 passing (9ms)
1 failing

1. lambda handler
   handler with module.exports syntax
   should not fail:
   TypeError: Cannot redefine property: handler

[jj22ee]

Instead of letting an error get thrown and potentially relying on catching it, one idea is preemptively determining if TypeError: Cannot redefine property: handler will occur, and in that case we just return [] and don't attempt to use InstrumentationNodeModuleDefinition or InstrumentationNodeModuleFile.

I haven't looked into yet, but is it possible to check if the "lambda handler is compiled with esbuild and using an immutable ESM export export const handler = function() {} transpiled to CJS."?

[mzl-md]

I think so, but trying to guess if the wrap-function would throw an error seems not a good approach to me. If the implementation of shimmer changes, we would need to update this as well. Besides, there could be other reasons for wrapping to fail and in my opinion this should never lead to an error in the instrumented code.

[anuraaga]

I haven't used lambda in a long time but I guess this is related to this issue that was staleboted

open-telemetry/opentelemetry-js#2260

(cc @trask we decided in Java to not close due to lack of maintainer attention a long time ago and wonder if this should be a org-wide consistent policy one way or the other)

I am confused by the change since previously we were trying to suppress an exception exactly to prevent crashes and now don't seem to. Shouldn't the current form be safer?

[anuraaga]

Note that if there are crashing issues, I would personally go to opentelemetry-js to make sure _wrap and family never throw. This seems generally safer.

[mzl-md]

Note that if there are crashing issues, I would personally go to opentelemetry-js to make sure _wrap and family never throw.

I thought about that but was not sure who else uses _wrap and might depend on it throwing errors. Maybe we can introduce a _saveWrap(). I will open a pull requests there.