-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect and warn if duplicate lambda libraries are detected #585
base: main
Are you sure you want to change the base?
Detect and warn if duplicate lambda libraries are detected #585
Conversation
…bda library, and log a warning if so.
cc6b8da
to
97937ec
Compare
detectDuplicateInstallations() | ||
.then((duplicateFound) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put this under the datadog()
wrapper, but I'm not sure if this is the best place to make this call. If there's somewhere that's better, I'm open to suggestions!
// Check for duplicate installations of the Lambda library | ||
detectDuplicateInstallations() | ||
.then((duplicateFound) => { | ||
if (duplicateFound) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this code might be relevant, so wanted to share for reviewers:
datadog-lambda-js/src/trace/tracer-wrapper.ts
Lines 28 to 44 in f05c600
export class TracerWrapper { | |
private tracer: any; | |
constructor() { | |
try { | |
// Try and use the same version of the tracing library the user has installed. | |
// This handles edge cases where two versions of dd-trace are installed, one in the layer | |
// and one in the user's code. | |
const path = require.resolve("dd-trace", { paths: ["/var/task/node_modules", ...module.paths] }); | |
this.tracer = require(path); | |
return; | |
} catch (err) { | |
if (err instanceof Object || err instanceof Error) { | |
logDebug("Couldn't require dd-trace from main", err); | |
} | |
} | |
} |
Note that this code only loads in one version of dd-trace
, not to be confused with the datadog-lambda-js
; this PR detects duplicate installations of datadog-lambda-js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if there's a way for us to use similar logic like this one 🤔
What does this PR do?
Asynchronously detects if the Lambda library is installed both in node_modules and as a Lambda library. If so, it logs a warning.
Motivation
It's common for users to install the Lambda layers as instructed by the documentation, but then then see import errors in their code, causing them to run
npm install datadog-lambda-js
. Now they will have the Lambda library installed in two places, which will (1) increase package size and cold start duration, and (2) break custom metrics.We've had many customers open support tickets recently about broken metrics caused by this issue, so this PR aims to avoid this problem in the future.
Testing Guidelines
Manual tests: logs a warning as expected when installed in both places, and doesn't log any warning if only installed in the layer or node_modules.
Added unit tests.
Additional Notes
This works by using
fs
and checking if the paths exist. I'm assuming the paths will always be:'/opt/nodejs/node_modules/datadog-lambda-js'
path.join(process.cwd(), 'node_modules/datadog-lambda-js')
We could use
process.env.NODE_PATH
and search fordatadog-lambda-js/package.json
in the paths. However, my solution is simpler and works perfectly fine. It is also faster than relying on node path because we don't have to parse the node path.Types of Changes
Check all that apply