crash when using aws-lambda instrumentation

[vmarchaud]

I've been running into an exception where the JS auto-instrumentation attempts to load package.json from the base path of a lambda handler. Is there a way to customize the path from /var/task/src/handlers/my-function-name/package.json to a higher level /var/task/package.json or /opt/my-extension/package.json?

Copying the package.json into the expected locations causes the instrumentation to work as expected, it would just be painful for the project's build process.

Setup:

• AWS managed Lambda Layer for ADOT JavaScript SDK and ADOT Collector
• OpenTelemetry JS v0.18.0
• OpenTelemetry JS Contrib v0.20b0,
• ADOT Collector for Lambda v0.9.1
• Node 12 runtimes
• Serverless framework builds and deploys

Exception:

2021-05-07T16:40:57.227Z undefined ERROR Uncaught Exception {"errorType":"Runtime.ImportModuleError",
"errorMessage":"Error: Cannot find module '/var/task/src/handlers/my-function-name/package.json'\n
Require stack:\n- /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/platform/node/instrumentation.js\n-
 /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/platform/node/index.js\n-
 /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/platform/index.js\n-
 /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/autoLoader.js\n-
 /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/index.js\n-
 /opt/nodejs/node_modules/@opentelemetry/instrumentation-aws-lambda/build/src/aws-lambda.js\n-
 /opt/nodejs/node_modules/@opentelemetry/instrumentation-aws-lambda/build/src/index.js\n-
 /opt/wrapper.js\n-
 
 internal/preload","stack":["Runtime.ImportModuleError: Error: Cannot find module '/var/task/src/handlers/my-function-name/package.json'",
 "Require stack:","- /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/platform/node/instrumentation.js","-
  /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/platform/node/index.js","-
   /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/platform/index.js","-
    /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/autoLoader.js","-
     /opt/nodejs/node_modules/@opentelemetry/instrumentation/build/src/index.js","-
      /opt/nodejs/node_modules/@opentelemetry/instrumentation-aws-lambda/build/src/aws-lambda.js","-
       /opt/nodejs/node_modules/@opentelemetry/instrumentation-aws-lambda/build/src/index.js","-
        /opt/wrapper.js","- internal/preload"," at _loadUserApp (/var/runtime/UserFunction.js:100:13)","
         at Object.module.exports.load (/var/runtime/UserFunction.js:140:17)"," at Object.<anonymous> (/var/runtime/index.js:43:30)",
         " at Module._compile (internal/modules/cjs/loader.js:999:30)","
         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)"," at Module.load (internal/modules/cjs/loader.js:863:32)","
          at Function.Module._load (internal/modules/cjs/loader.js:708:14)","
           at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)"," at internal/main/run_main_module.js:17:47"]}

Originally posted by @adambartholomew in #2192

[vmarchaud]

I believe this is happening because lambda have a require path behavior than node itself, i'm converting this into an issue since its a bug in the base instrumentation class

/cc @anuraaga @willarmiros

[willarmiros]

Thanks for bringing this up, here's the problematic require in the base instrumentation:

opentelemetry-js/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

Line 76 in 1758fa6

const version = require(path.join(baseDir, 'package.json')).version;

One suggestion is we could use pkginfo which is made for this use case. All it does is recursively search up the file tree for the first package.json it finds, so we could also roll our own solution for that if we wanted to avoid the dependency.

[vmarchaud]

I would prefer to have this logic in the base instrumentation to avoid the dependency indeed (specially since we only need this code for the node platform)

[dyladan]

I believe this is happening because lambda have a require path behavior than node itself, i'm converting this into an issue since its a bug in the base instrumentation class

/cc @anuraaga @willarmiros

I don't believe that amazon has a different require behavior. They do change some of the require path configuration, but I believe the resolution behavior is still the documented node behavior.

[dyladan]

When is this bug triggered? I assume not for all lambda invocations, so does it only happen when the user application doesn't have a package.json? Which module is being intercepted that uses the user's application as the base path? Is it the load of the user application itself?

[willarmiros]

I assume not for all lambda invocations, so does it only happen when the user application doesn't have a package.json?

Based on @adambartholomew's original issue, it sounds like there was a package.json at the root of the function bundle (at /var/task), but this error still occurred because it was searching in the baseDir /var/task/src/handlers/my-function-name.

One thing I'm unclear on is what exactly require-in-the-middle is intended to be used for. My understanding is it's a way to safely require modules that are targeted for instrumentation so that we can execute instrumentation logic when the module is being imported. This would make me think that the baseDir (on the line I linked above) would always be pointing to the customer's app rather than the instrumented module as expected. Could someone help clarify that?

[dyladan]

require-in-the-middle intercepts the require of instrumented modules. For instance the mysql instrumentation registers a RITM hook for the mysql module. When the customer app calls require('mysql'), our hook is fired before the require returns the module to the user app. In this hook we patch anything we need. The basedir is typically the directory of the instrumented module (in this case mysql) so we can check to ensure it is a version we are capable of instrumenting.

[willarmiros]

Ok that makes sense. I think the root issue here is that the Lambda instrumentation follows a different pattern for declaring its target module instrumentation. Typically, instrumented modules are well-defined module names (e.g. mysql) that are the same as the name of their NPM package and have a package.json available at the root by definition. The Lambda instrumentation is targeting an arbitrary user-defined module instead, wherever their Lambda request handler happens to be. This weirdness was briefly discussed in review.

The problem now is that package.json does not necessarily exist alongside the handler (as was the case here), or at all, in a Lambda function. So I propose a few options:

1. Stick with the recursive search in fix(instrumentation-node): recursively discover package.json  #2206, since it will be the same as existing behavior for all "normal" modules where the package.json is right where you'd expect it and no recursion is needed.
2. Given that the Lambda instrumentation accepts all versions, we could just skip the package.json require & check altogether if all versions are accepted, as a special case.
3. Similar to option 2. Change the InstrumentationNodeModuleDefinition to have supported versions as optional, since it's clearly not applicable for this instrumentation, and skip the version checks if not provided.

[dyladan]

I'm pretty sure (3) is already the case.

[willarmiros]

I think it's required?

opentelemetry-js/packages/opentelemetry-instrumentation/src/platform/node/types.ts

Line 45 in 1758fa6

supportedVersions: string[];

[dyladan]

Ah I think probably we made it required so authors would have to manually put * if they really meant to support all versions (it is very hard impossible to write an instrumentation that really works across all versions of some package).

[vmarchaud]

Could we not lookup the version if we see * in the supported versions then ?

[dyladan]

That's what I was thinking. If the supportedVersions.contains('*') we shouldn't have any reason to even attempt to find the package.json

[gregoryfranklin]

Is there consensus on the solution?

The most recent suggestion was to only read the package.json if supportedVersions does not contain *. This will resolve the lambda issue because the lambda instrumentation does have *.

[adambartholomew]

I took a stab at this back in May, and ended up stashing it as it didn't match the exact solution proposed above. I was honestly just hoping someone more experienced would pick it up.

It was extremely challenging to do a simple supportedVersions.contains('*') as the code is forked for two separate flows- Modules, and files. Additionally, the module.moduleVersion is also patched prior to these supportedVersion calls. Lastly, the moduleVersion is propagated to the module.patch, and file.patch functions.

I do not have enough understanding on the importance of these versions throughout this package, modules, and exports to be comfortable enough to change the behaviour of these methods.

This is the closest I got without any major refactoring:
https://github.com/open-telemetry/opentelemetry-js/compare/main...adambartholomew:aws-lambda-crash?expand=1
I could convert this to a PR, and move it through if there is sufficient interest.

This has been tested exclusively with unit tests, and should help raise the overall coverage on these classes. I have not loaded this into any real-world aws-lambda instrumentation projects for testing.

[nozik]

@vmarchaud The PR was merged (#2450) - should we close the ticket?

[knvpk]

Is this released, because im getting this error with all latest versions

[HarryEMartland]

I think we are waiting for a new release of @opentelemetry/instrumentation-aws-lambda. The confusing thing is the latest version of @opentelemetry/instrumentation-aws-lambda is 0.26.0 which is the version of @opentelemetry/instrumentation which contains the fix.