Fix patching behavior for require-in-the-middle #4298
Conversation
Overall package sizeSelf size: 6.48 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
Here's an alternative approach that hard codes module names to bypass RITM: #4305 |
|
Turns out the reason is that they add additional resolving capabilities to |
BenchmarksBenchmark execution time: 2024-05-21 21:18:45 Comparing candidate commit 08257a8 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 263 metrics, 3 unstable metrics. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4298 +/- ##
==========================================
- Coverage 69.19% 68.31% -0.88%
==========================================
Files 1 13 +12
Lines 198 972 +774
Branches 33 33
==========================================
+ Hits 137 664 +527
- Misses 61 308 +247 ☔ View full report in Codecov by Sentry. |
packages/dd-trace/test/ritm.spec.js
Outdated
| 'a failing `require(...)` can still throw as expected' | ||
| ) | ||
|
|
||
| httpHook.unhook() |
There was a problem hiding this comment.
This should go in an afterEach otherwise it won't be executed if the test fails.
There was a problem hiding this comment.
I added set up and tear down functions to the tests to avoid this issue
|
LGTM, please test this in Lambda as well as Azure to confirm things still work. (unit tests look good for coverage, but still worth manually checking) |
| @@ -50,8 +50,20 @@ function Hook (modules, options, onrequire) { | |||
|
|
|||
| if (patchedRequire) return | |||
|
|
|||
| const _origRequire = Module.prototype.require | |||
There was a problem hiding this comment.
this might be unrelated to this PR, but if we are using this _origRequire, then can we remove the origRequire above and use this one in other places too?
There was a problem hiding this comment.
Hey @joeyzhao2018! I address that in this comment: #4298 (comment)
|
/merge |
|
🚂 MergeQueue This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
dd-devflow
bot
added
mergequeue-status: done
and removed
mergequeue-status: waiting
labels
|
🚂 MergeQueue This pull request was merged directly. |
What does this PR do?
Applies a fix made to the
require-in-the-middlepackage so monkey patched modules can be required correctly.Motivation
https://datadoghq.atlassian.net/browse/SVLS-4805
This is needed to use
require-in-the-middlewithazure-functions-nodejs-worker, which patchesrequireto supportrequire('@azure/functions-core').@azure/functionsadds these modules that otherwise would not be handled by require. So fromdd-trace's perspective these modules don't exist and it errors out when trying to require them.require-in-the-middle: feat: support deferring to the underlying 'require' implementation if Module._resolveFilename fails elastic/require-in-the-middle#59Plugin Checklist
Additional Notes
Confirmed unit tests fails before fix to
ritm.jsis added.Mimic
require-in-the-middleby setting_origRequireinside theHookfunction. Our version of ritm setsorigRequireoutside ofHookand prevented the unit test from passing. I tried refactoring all of the references toorigRequirebut it resulted in unit tests in other packages failing. I took this approach to prevent regressions.https://github.com/elastic/require-in-the-middle/blob/826dbde2898ef4a3a4ddb0fe6d44287694238b5b/index.js#L124