Add threads.preload config to preload modules (e.g. APM agents) on worker threads#1569
Conversation
…rker threads Adds a threads.preload config option (string or array) that injects the resolved module(s) via --require into each worker thread's execArgv, so an APM agent like dd-trace/init loads before any Harper or app module and can instrument subsequent module loads. Bare specifiers resolve against installed components' node_modules (with the components root and Harper install as fallbacks); absolute paths are also accepted. Not applied under Bun, which does not use execArgv here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Memoize resolution at the spawn call site (resolve once per process, not per worker spawn/restart), which also stops repeated warn logs for an unresolvable specifier. - Guard the components-root readdir with try/catch so a missing/unreadable root can't crash worker spawning. - Reject relative-path specifiers (non-deterministic across component anchors); contract is an absolute path or a package installed in a component. - Add a trace log of resolved preloads; add tests for relative rejection and the unreadable-root path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration option, threads.preload, which allows preloading specified modules (such as APM agents) on worker threads via --require before other modules load. It includes schema updates, validation logic, resolution logic in a new file resolvePreload.ts, and corresponding unit tests. The review feedback suggests improving the resolution of absolute paths by leveraging Node's built-in createRequire mechanism instead of a simple existsSync check, which would robustly handle file extensions and directory indexes.
|
Reviewed; no blockers found. |
…via Node resolver Addresses PR review: - resolvePreloadModules now takes (configured, componentsRoot) explicitly; the caller in manageThreads reads config. Makes the unit test stub-free (plain assert, no sinon) per AGENTS.md test conventions. - Absolute specifiers now go through createRequire().resolve() like bare ones, so extensionless paths and directory/package 'main' resolve correctly (was a bare existsSync check that only matched exact files). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Synced with Datadog on this — one thing to add: support preloading via Otherwise it looks good on their side and we're clear to test once released. Thanks @kriszyp! |
Make threads.preload use --import (ESM/loader-hook registration) so an APM agent's register entry (e.g. dd-trace/register.js) instruments worker threads, which is where Harper runs. Add threads.preloadRequire for the --require (CJS) path used by agents that document it (dd-trace/init, Dynatrace OneAgent). Per Datadog: recent dd-trace/init (--require) only instruments the main thread; the register entry loaded via --import is what covers worker threads. --import paths are passed as file URLs (ESM preload is URL-based; a bare absolute path can break on Windows). resolvePreloadModules gains a configKey label so warnings name the right key; resolution logic is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Patch cherry-pick: mergedCherry-picked onto |
Add threads.preload config to preload modules (e.g. APM agents) on worker threads
Summary
Adds two
threads.*config options that preload module(s) into each worker thread at spawn, before the worker's entry script and any other Harper or app module — the earliest point in a worker's lifetime, early enough for an APM agent (e.g.dd-trace, Dynatrace OneAgent) to instrument all subsequent module loads:threads.preload— preloads via Node's--import(ESM / loader-hook registration).threads.preloadRequire— preloads via Node's--require(CommonJS).Purpose
Operators want to run APM/instrumentation agents that must load before everything else to monkey-patch modules as they're required. Requiring such a module from within
threadServer.jsis too late — the worker entry file has alreadyrequired ~20 Harper modules at its top. Injecting the preload into the worker'sexecArgvruns it before the entry script's body executes at all.Why both
--importand--require: Harper does its real work on worker threads, so instrumenting those threads is the whole point. Per Datadog, recentdd-trace/init(the--requireentry) only instruments the main thread; theirregisterentry loaded via--import dd-trace/register.jsis what installs the loader hooks that cover worker threads. Sothreads.preloaduses--importas the primary path.threads.preloadRequireremains for CJS agents that document the--requirepath and don't need ESM loader hooks (e.g. Dynatrace OneAgent) — where--import-ing a CJS agent would be an unvalidated deviation from their docs.How it works
server/threads/resolvePreload.tsreads a config value (string or array) and resolves each specifier to an absolute path. Bare specifiers (e.g.dd-trace/register.js) resolve against installed components'node_modules— each component directory under the components root is tried first, then the components root and the Harper install as fallbacks — so an agent bundled as a dependency of a deployed component is found. Absolute paths are accepted directly. The resolver is mode-agnostic; aconfigKeylabel only tailors warning messages.server/threads/manageThreads.jsstartWorker()pushes--import <file-url>for eachthreads.preloadmodule and--require <path>for eachthreads.preloadRequiremodule intoexecArgv.--importis URL-based, so resolved paths are passed aspathToFileURL(...).href(a bare absolute path can break--importon Windows). Resolution is memoized once per process (config and installed components are fixed for an APM agent's purpose — it must be present at boot to instrument boot). This covers both HTTP workers (threadServer.js) and job workers (jobProcess.js), since both spawn throughstartWorker.THREADS_PRELOAD/THREADS_PRELOADREQUIREterms, Joi validation (string | array of strings | null),defaultConfig.yaml, and the JSON config schema.Where to look
resolveSpecifier/getResolutionAnchorsinresolvePreload.ts— the resolution order and the fallbacks. Relative-path specifiers (./x) are deliberately rejected: they'd resolve against whichever component anchor matched first (non-deterministic). The contract is an absolute path or a package installed in a component.--importfile URL inmanageThreads.js— the ESM preload path is resolved to an absolute filesystem path, then passed to--importas afile://URL for cross-platform correctness.execArgvpath (it has its own--preload). Called out in a comment; no Bun support in this PR.Testing
unitTests/server/threads/resolvePreload.test.js— 10 tests covering bare/subpath/array/absolute resolution, unresolvable-warn, relative rejection,configKeylabel independence, and that a missing/unreadable components root doesn't throw. Fullserver/threads+config+validationunit suites pass. Build and lint clean.Review
Cross-model reviewed (Gemini leg + Harper-domain adjudication). Findings folded in before this opened: memoize resolution (avoid per-spawn
readdirSync+ repeated warn logs),try/catchthe components-root readdir so a missing/unreadable root can't crash worker spawning, reject relative specifiers, and a trace log of resolved preloads. The--import/preloadRequiresplit was added in response to Datadog's guidance that theregister/--importentry is required to instrument worker threads.Generated with assistance from Claude (Opus 4.8); reviewed and committed by Kris.
Docs
Companion documentation PR: HarperFast/documentation#561