perf(ext): convert ext/telemetry and ext/cron JS sources to lazy-loaded scripts#33801
Conversation
…ed scripts Convert `ext/telemetry/telemetry.ts`, `ext/telemetry/util.ts`, and `ext/cron/01_cron.ts` from ESM to IIFE-wrapped lazy-loaded scripts (`lazy_loaded_js`), eliminating ESM module resolution overhead. These are the first `.ts` sources converted to lazy-loaded scripts -- the `deno_core` transpiler handles both `lazy_loaded_js` and `esm` paths identically. Telemetry's mutable exports (`TRACING_ENABLED`, `METRICS_ENABLED`, `PROPAGATORS`, `getOtelSpan`) use getters in the return object so consumers see the updated values after `bootstrap()` runs. Cron's `import.meta.log` (unavailable in script context) is replaced with `core.print` to stderr. Updates consumers to use `core.loadExtScript()`: - `ext/http/00_serve.ts` - `ext/node/polyfills/_http_server.js` - `ext/node/polyfills/_http_client.js` - `runtime/js/90_deno_ns.js` - `runtime/js/99_main.js`
097de3b to
c7f605a
Compare
fibibot
left a comment
There was a problem hiding this comment.
Substance walks cleanly for the mechanical IIFE-wrap parts (consistent with #33778/#33779/#33780/#33784/#33800), and converting .ts sources to lazy_loaded_js is a useful first — the deno_core transpiler handling both paths identically per the body checks out. The unblocking of ext/cron's conversion in the same PR is reasonable scope.
Substantive concern: the getter-in-return-object pattern doesn't preserve live bindings for destructure consumers
The PR body claims:
Mutable exports (
TRACING_ENABLED,METRICS_ENABLED,PROPAGATORS,getOtelSpan) use getters in the return object so consumers see the updated values afterbootstrap()runs.
That's true for namespace-access consumers (telemetryModule.TRACING_ENABLED) — the getter fires on every property read. But all four destructure consumers in this PR's diff destructure these mutable values, capturing them at destructure time:
ext/http/00_serve.ts:const { ..., PROPAGATORS, ..., TRACING_ENABLED } = core.loadExtScript(...)then usesif (TRACING_ENABLED)andfor (const propagator of ... PROPAGATORS)inside request handlers.ext/cron/01_cron.ts: same destructure, sameif (TRACING_ENABLED)+PROPAGATORSiteration inside the cron-handler dispatch.ext/node/polyfills/_http_server.js: same destructure + same conditional usage in request handler.ext/node/polyfills/_http_client.js: same destructure +if (TRACING_ENABLED && !this[kOtelSpan])in request setup.
For each of these, the getter fires once at destructure time, the local const TRACING_ENABLED is bound to that snapshot value, and subsequent bootstrap() calls that flip the inner let TRACING_ENABLED = true inside the telemetry IIFE never propagate to those local consts.
Load-order trace
Why this matters in practice:
- Runtime startup →
99_main.jsloads (ESM). Its imports includeext:runtime/90_deno_ns.js. 90_deno_ns.jsbody runs (ESM imports complete first). The new lineconst cron = core.loadExtScript("ext:deno_cron/01_cron.ts")evaluates the cron IIFE.- Cron IIFE body runs
const { TRACING_ENABLED, PROPAGATORS, ... } = core.loadExtScript("ext:deno_telemetry/telemetry.ts")— telemetry IIFE evaluates here for the first time, returns its namespace object with getters. Cron destructures, getters fire, capturesTRACING_ENABLED = falseandPROPAGATORS = [](the initial values). - 99_main.js body eventually executes
bootstrapMainRuntime→bootstrapOtel(otelConfig)→ flips the innerTRACING_ENABLEDtotrueinside the telemetry IIFE's closure scope. - A cron handler fires later →
if (TRACING_ENABLED)reads the cron module's local const, which is stillfalsefrom step 3. No span recorded.
Same regression shape as the OTEL kind: 3 vs kind: 2 issue I flagged in #33784's destructure-of-internals.__telemetry pattern.
Why current CI may not catch it
Unit / spec / node_compat tests typically don't exercise the bootstrap-then-handler ordering with OTEL config — most tests run with OTEL disabled by default, where TRACING_ENABLED is false either way. The regression surfaces in production when users run with OTEL_DENO=true or equivalent and observe missing http/cron spans.
Direction
Two options that preserve live-binding semantics for destructure consumers:
- Don't destructure the mutables. Have consumers do
import * as telemetry(in lazy_loaded form:const telemetry = core.loadExtScript("ext:deno_telemetry/telemetry.ts")) and readtelemetry.TRACING_ENABLED/telemetry.PROPAGATORSinline. Verbose but matches ESM live-binding semantics through the getter. - Wrap mutables in functions (
isTracingEnabled(),getPropagators()) — destructure-friendly because functions don't change identity, but each call site has to use the function-call form.
Option 1 is the smaller delta from this PR's current shape.
CI
54 pass, 12 pending, 1 skip at this head. No failures yet, but as noted above the typical CI signal won't catch the OTEL regression. Holding COMMENT regardless of CI outcome — substance concern stands.
Destructuring mutable exports from loadExtScript copies values at destructure time. After bootstrap() updates TRACING_ENABLED etc., consumers still see the original false values. Fix: expose mutable state via an otelState object that consumers receive by reference, so property access at call-time always reflects the latest values.
The conversion to lazy-loaded IIFE scripts broke cron error logging because import.meta.log is unavailable in IIFEs. Added internals.log() as an equivalent API and updated ext/cron to use it.
fibibot
left a comment
There was a problem hiding this comment.
LGTM — author addressed my prior live-binding concern exactly as I'd diagnosed it. Walking the two new commits since c7f605a:
d40a8ec — otelState object preserves live-binding semantics
The commit message is precise:
Destructuring mutable exports from loadExtScript copies values at destructure time. After bootstrap() updates TRACING_ENABLED etc., consumers still see the original false values.
The fix is the mutable-object-reference pattern I'd suggested:
const otelState = {
TRACING_ENABLED: false,
METRICS_ENABLED: false,
PROPAGATORS: [] as TextMapPropagator[],
getOtelSpan: undefined as ((span: object) => OtelSpan | null | undefined) | undefined,
};
function wrappedBootstrap(config) {
_origBootstrap(config);
otelState.TRACING_ENABLED = TRACING_ENABLED;
otelState.METRICS_ENABLED = METRICS_ENABLED;
otelState.PROPAGATORS = PROPAGATORS;
otelState.getOtelSpan = getOtelSpan;
}And bootstrap: wrappedBootstrap in the return object so 99_main.js's bootstrapOtel(config) triggers the post-bootstrap state copy.
All four consumers correctly updated:
ext/http/00_serve.ts:if (otelState.TRACING_ENABLED),for (const propagator of new SafeArrayIterator(otelState.PROPAGATORS)),otelState.getOtelSpan?.(span)ext/cron/01_cron.ts: same pattern.ext/node/polyfills/_http_server.js: same.ext/node/polyfills/_http_client.js: same.
The optional-chaining on otelState.getOtelSpan?.(span) correctly handles the not-yet-bootstrapped case (getOtelSpan is undefined until wrappedBootstrap copies it).
4a3b67f — internals.log for lazy-loaded scripts
The IIFE-wrap loses access to import.meta.log (which is real-ESM-only). The previous workaround in cron used core.print("[error] ...") — losing the structured-log routing. The new commit:
- Adds
internals.log = function internalLog(levelStr, ...args)in99_main.jsthat mirrorsimport.meta.log's shape but uses a fixed"ext:runtime"module URL. - Restores cron to use
internals.log("error", ...)instead ofcore.print(...).
Minor caveat: all logs from lazy-loaded scripts now appear as originating from "ext:runtime" rather than the actual source file. Worth knowing for log-grep workflows but not a blocker — the level + message + args still carry the actionable detail. Worth a TODO to thread the actual ext URL in for accuracy at some point.
CI
Fully green at this head: 135/136 success, 1 skip, 0 fail. All test unit / test unit_node / test node_compat / test specs shards pass across all platforms × debug+release.
Smaller observation (non-blocking)
The _origBootstrap / wrappedBootstrap pattern doubles the function-pointer indirection for one call (bootstrap runs once at startup), so perf is irrelevant. The otelState object adds one property-access dereference per call site — also irrelevant given the per-call cost is dominated by the actual span/metric work. Pure pattern note.
… scripts (#33817) - Convert `ext/process/40_process.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Convert `ext/http/00_serve.ts`, `ext/http/01_http.js`, `ext/http/02_websocket.ts` from ESM to IIFE-wrapped lazy-loaded scripts - Update all consumers (`runtime/js/90_deno_ns.js`, `runtime/js/99_main.js`, `ext/node/polyfills/child_process.ts`, `ext/node/polyfills/internal/child_process.ts`) to use `core.loadExtScript()` instead of ESM imports - Replace `import.meta.log` calls with `internals.log` in `00_serve.ts` Follows the same pattern established in #33778, #33780, #33784, #33800, and #33801.
…ripts (#33818) ## Summary - Convert `ext/kv/01_db.ts` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Convert `ext/webgpu/00_init.js` from ESM to IIFE-wrapped lazy-loaded script (`lazy_loaded_js`) - Update all consumers (`runtime/js/90_deno_ns.js`, `runtime/js/98_global_scope_shared.js`, `runtime/js/98_global_scope_window.js`, `runtime/js/98_global_scope_worker.js`) to use `core.loadExtScript()` instead of ESM imports - Replace `import.meta.log` call with `internals.log` in `01_db.ts` Follows the same pattern established in #33778, #33780, #33784, #33800, #33801, and #33817.
Follow-up to #33800. Converts
ext/telemetry/telemetry.tsandext/telemetry/util.tsfrom ESM to IIFE-wrapped lazy-loaded scripts(
lazy_loaded_js), eliminating ESM module resolution overhead.These are the first
.tssources converted to lazy-loaded scripts --the
deno_coretranspiler handles bothlazy_loaded_jsandesmpaths identically.
Telemetry's mutable exports (
TRACING_ENABLED,METRICS_ENABLED,PROPAGATORS,getOtelSpan) are exposed via a sharedotelStateobject that consumers receive by reference, so property access at
call-time always reflects the latest values set by
bootstrap().(Destructuring from
loadExtScriptwould copy values at load time,before
bootstrap()runs.)Updates 7 consumers to use
core.loadExtScript():ext/http/00_serve.tsext/cron/01_cron.tsext/node/polyfills/_http_server.jsext/node/polyfills/_http_client.jsruntime/js/90_deno_ns.jsruntime/js/99_main.js