perf(loader): synchronous import-in-the-middle hooks and 3.2.0 shouldInclude matcher#9026
Merged
Merged
Conversation
Contributor
Overall package sizeSelf size: 6.37 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.2.0 | 104.26 kB | 843.44 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 978fd05 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9026 +/- ##
=======================================
Coverage 93.68% 93.69%
=======================================
Files 889 889
Lines 50841 50842 +1
Branches 11799 11800 +1
=======================================
+ Hits 47632 47636 +4
+ Misses 3209 3206 -3 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-06-26 21:14:21 Comparing candidate commit 978fd05 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2257 metrics, 29 unstable metrics.
|
BridgeAR
force-pushed
the
BridgeAR/2026-06-22-iitm-shouldinclude
branch
3 times, most recently
from
June 22, 2026 21:58
e2b5c0e to
05a0194
Compare
BridgeAR
changed the title
BridgeAR
changed the base branch from
juan-fernandez/iitm-sync-loader-hooks
to
master
June 22, 2026 22:05
BridgeAR
marked this pull request as ready for review
June 23, 2026 19:33
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 782da9406b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
import-in-the-middle scanned the include and exclude options once per resolved module — up to ~290 include entries (RegExp.test or string compare) plus a fileURLToPath on every resolve, nearly all against modules that match nothing. Supply iitm's shouldInclude predicate instead: a single Set answers the bare-specifier check and one combined RegExp covers every instrumented node_modules path and the configured security-control subpaths, with one more RegExp for the exclusions. The Set also carries each built-in's node: specifier (mirroring iitm's include expansion), so `import 'node:crypto'` stays instrumented as well as `import 'crypto'`. Over a mixed resolve corpus this drops the per-resolve matching cost from ~2.5us to ~25ns (about 100x), reproduced across runs. Package names now pass through regexpEscape so a metacharacter in a future package name cannot mis-match.
The rewriter loader spec is the repository's only `.spec.mjs`, and no CI job
ran it: the misc suite glob matched `.spec.js` only, and the exercised-tests
gate collected `.spec.js`/`.test.mjs` but not `.spec.mjs`, so it could not flag
the orphan.
1. Match `*.spec.{js,mjs}` in `test:instrumentations:misc` so the spec runs.
2. Widen the verify-exercised-tests test-file globs to `@(spec|test).@(js|mjs|cjs)`
so every naming convention is tracked and an unrun one fails the gate.
3. Load the loader through require(esm) in the spec so its async and sync
transforms land on nyc's CommonJS instrumentation path; the top-level import
bypassed coverage entirely.
BridgeAR
force-pushed
the
BridgeAR/2026-06-22-iitm-shouldinclude
branch
from
June 25, 2026 19:42
f2f52ac to
1788919
Compare
…sm) support The loader spec require()s the .mjs loader so nyc instruments it through the CommonJS require extension and its transforms count as covered. On Node 18 that require has no native require(esm) to fall through to, so nyc's .mjs extension hands the source to the CommonJS compiler, which throws a SyntaxError on the loader's ESM import rather than ERR_REQUIRE_ESM. The before hook only caught the latter and crashed the whole suite. Gate on process.features.require_module instead: require(esm) where the runtime supports it (covered), import() where it does not.
bengl
approved these changes
Jun 26, 2026
dd-octo-sts Bot
pushed a commit
that referenced
this pull request
Jun 29, 2026
…te (#9026) import-in-the-middle scanned the include and exclude arrays once per resolved module — up to ~290 include entries (RegExp.test or string compare) plus a fileURLToPath on every resolve, nearly all against modules that match nothing. Supplying iitm 3.2.0's shouldInclude predicate replaces that scan with a single Set lookup for bare specifiers and one combined RegExp covering every instrumented node_modules path and the configured security-control subpaths, plus one RegExp for the exclusions. Over a mixed resolve corpus this drops the per-resolve matching cost from ~2.5µs to ~25ns (about 100x). The Set also carries each built-in's node: specifier, mirroring iitm's include expansion, so `import 'node:crypto'` stays instrumented alongside `import 'crypto'`. Package names pass through regexpEscape so a metacharacter in a future package name cannot mis-match. The .mjs rewriter loader spec was the repository's only .spec.mjs and no CI job ran it: the misc suite glob matched .spec.js only, and the exercised-tests gate collected .spec.js/.test.mjs but not .spec.mjs, so it could not flag the orphan. 1. Match *.spec.{js,mjs} in test:instrumentations:misc so the spec runs. 2. Widen verify-exercised-tests globs to @(spec|test).@(js|mjs|cjs) so every naming convention is tracked and an unrun one fails the gate. 3. Load the loader through require(esm) where the runtime supports it so its transforms land on nyc's CommonJS instrumentation path; gate on process.features.require_module so Node 18 falls back to import() instead of crashing the suite on the CommonJS compiler's SyntaxError.
Closed
rochdev
pushed a commit
that referenced
this pull request
Jul 2, 2026
…te (#9026) import-in-the-middle scanned the include and exclude arrays once per resolved module — up to ~290 include entries (RegExp.test or string compare) plus a fileURLToPath on every resolve, nearly all against modules that match nothing. Supplying iitm 3.2.0's shouldInclude predicate replaces that scan with a single Set lookup for bare specifiers and one combined RegExp covering every instrumented node_modules path and the configured security-control subpaths, plus one RegExp for the exclusions. Over a mixed resolve corpus this drops the per-resolve matching cost from ~2.5µs to ~25ns (about 100x). The Set also carries each built-in's node: specifier, mirroring iitm's include expansion, so `import 'node:crypto'` stays instrumented alongside `import 'crypto'`. Package names pass through regexpEscape so a metacharacter in a future package name cannot mis-match. The .mjs rewriter loader spec was the repository's only .spec.mjs and no CI job ran it: the misc suite glob matched .spec.js only, and the exercised-tests gate collected .spec.js/.test.mjs but not .spec.mjs, so it could not flag the orphan. 1. Match *.spec.{js,mjs} in test:instrumentations:misc so the spec runs. 2. Widen verify-exercised-tests globs to @(spec|test).@(js|mjs|cjs) so every naming convention is tracked and an unrun one fails the gate. 3. Load the loader through require(esm) where the runtime supports it so its transforms land on nyc's CommonJS instrumentation path; gate on process.features.require_module so Node 18 falls back to import() instead of crashing the suite on the CommonJS compiler's SyntaxError.
rochdev
pushed a commit
that referenced
this pull request
Jul 2, 2026
…te (#9026) import-in-the-middle scanned the include and exclude arrays once per resolved module — up to ~290 include entries (RegExp.test or string compare) plus a fileURLToPath on every resolve, nearly all against modules that match nothing. Supplying iitm 3.2.0's shouldInclude predicate replaces that scan with a single Set lookup for bare specifiers and one combined RegExp covering every instrumented node_modules path and the configured security-control subpaths, plus one RegExp for the exclusions. Over a mixed resolve corpus this drops the per-resolve matching cost from ~2.5µs to ~25ns (about 100x). The Set also carries each built-in's node: specifier, mirroring iitm's include expansion, so `import 'node:crypto'` stays instrumented alongside `import 'crypto'`. Package names pass through regexpEscape so a metacharacter in a future package name cannot mis-match. The .mjs rewriter loader spec was the repository's only .spec.mjs and no CI job ran it: the misc suite glob matched .spec.js only, and the exercised-tests gate collected .spec.js/.test.mjs but not .spec.mjs, so it could not flag the orphan. 1. Match *.spec.{js,mjs} in test:instrumentations:misc so the spec runs. 2. Widen verify-exercised-tests globs to @(spec|test).@(js|mjs|cjs) so every naming convention is tracked and an unrun one fails the gate. 3. Load the loader through require(esm) where the runtime supports it so its transforms land on nyc's CommonJS instrumentation path; gate on process.features.require_module so Node 18 falls back to import() instead of crashing the suite on the CommonJS compiler's SyntaxError.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Standalone PR combining the synchronous import-in-the-middle loader (originally #8942, commits by @juan-fernandez) with the import-in-the-middle 3.2.0 upgrade and a consumer-owned
shouldIncludematcher.module.registerHooks()on Node.js versions that support it, else the asyncmodule.registerloader (@juan-fernandez).shouldIncludepredicate.fileURLToPath) with ashouldIncludepredicate: oneSetfor bare specifiers (built-ins carry both their bare andnode:form), one combinedRegExpfor instrumentednode_modulespaths and security-control subpaths, oneRegExpfor the exclusions.register.jswarns and falls back to the asynchronous loader instead of throwing when synchronous hooks are expected but cannot register.Why
The matcher drops per-resolve cost from ~2.5µs to ~25ns (~100x) on a mixed corpus — the startup hot path on large ESM graphs. Package names pass through
regexpEscapeso a future metacharacter cannot mis-match.Open question
register.jskeeps a small Node-version pre-check rather than importing iitm'ssupportsSyncHooksdirectly: it is CJS, and importing the ESM helper would load iitm's machinery (acorn + the code transformer) on the main thread for unsupported runtimes, regressing their startup. iitm'ssupportsSyncHooksstays the runtime authority insideloader-hook.mjs; the pre-check only avoids that load. The clean fix is upstream — iitm exposingsupportsSyncHooksfrom a dependency-free module.Test plan
mocha packages/dd-trace/test/register.spec.jsmocha packages/datadog-instrumentations/test/helpers/rewriter/loader.spec.mjs