From cff19ab89d06e37e683b6f52e1e34b973d9a0510 Mon Sep 17 00:00:00 2001 From: Pablo Erhard Date: Thu, 1 Oct 2026 14:59:12 -0400 Subject: [PATCH 1/2] perf: check subscribers before allocating the per-call context Signed-off-by: Pablo Erhard --- lib/transforms.js | 66 ++++-- tests/arguments_mutation_kinds_cjs/mod.js | 30 +++ tests/arguments_mutation_kinds_cjs/test.js | 80 +++++++ tests/common/transport_spy.js | 34 +++ tests/fast_path_cjs/mod.js | 82 +++++++ tests/fast_path_cjs/test.js | 262 +++++++++++++++++++++ tests/tests.test.mjs | 107 +++++++++ 7 files changed, 635 insertions(+), 26 deletions(-) create mode 100644 tests/arguments_mutation_kinds_cjs/mod.js create mode 100644 tests/arguments_mutation_kinds_cjs/test.js create mode 100644 tests/common/transport_spy.js create mode 100644 tests/fast_path_cjs/mod.js create mode 100644 tests/fast_path_cjs/test.js diff --git a/lib/transforms.js b/lib/transforms.js index 967fd84..16cab2f 100644 --- a/lib/transforms.js +++ b/lib/transforms.js @@ -266,7 +266,7 @@ function traceFunction (state, node, program) { async: node.async, expression: false, generator: node.generator, - }, program) + }, program, node.type === 'ArrowFunctionExpression') node.params = wrapParams(params) @@ -368,16 +368,24 @@ function traceInstanceMethod (state, node, program) { * Builds the replacement block statement for a function body. * * Selects the appropriate wrapper template (`wrapSync`, `wrapPromise`, etc.) and - * prepends the shared `__apm$ctx` / `__apm$traced` preamble before returning the + * prepends the shared `__apm$traced` / `__apm$ctx` preamble before returning the * resulting block statement body. * + * The no-subscriber check sits between the two halves of the preamble: after + * `__apm$traced` (which only captures the call arguments it is given) and + * before `__apm$arguments` / `__apm$ctx`, so an unsubscribed call does not + * build any of the per-call transport. + * * @param {object} state * @param {import('estree').Node} node - The original function (or identifier for instance methods). * @param {import('estree').Program} program + * @param {boolean} [outerIsArrow] - Whether the wrapper itself is an arrow + * function, in which case it has no own `arguments`. * @returns {import('estree').BlockStatement['body']} */ -function wrap (state, node, program) { - const { operator, moduleVersion, selfBinding } = state +function wrap (state, node, program, outerIsArrow = false) { + const { operator, moduleVersion, channelName, selfBinding } = state + const channelVariable = formatChannelVariable(channelName) const { returnKind } = state.functionQuery const iterPatch = returnKind ? generateIterPatch(state, returnKind, program) : '' @@ -396,18 +404,21 @@ function wrap (state, node, program) { const block = wrapper.body[0].body // Extract only block statement of function body. // Declared in the outer function scope so that the `super()` call sites in // the moved body can write to it and the `finally` block can read it back. + // It sits above the subscriber check because the fast path runs `super()` + // too, and assigning a `let` before its declaration runs throws. const declareSelf = selfBinding ? `let ${selfBinding};` : '' - const common = parse(declareSelf + (node.type === 'ArrowFunctionExpression' + const innerIsArrow = node.type === 'ArrowFunctionExpression' + const callWrapped = innerIsArrow + ? '__apm$wrapped(...__apm$callArgs)' + : '__apm$wrapped.apply(this, __apm$callArgs)' + const fastArgs = outerIsArrow ? `[${args}]` : 'arguments' + const transport = innerIsArrow ? ` const __apm$arguments = [${args}]; const __apm$ctx = { arguments: __apm$arguments, moduleVersion: ${JSON.stringify(moduleVersion)} }; - const __apm$traced = () => { - const __apm$wrapped = () => {}; - return __apm$wrapped(...__apm$arguments); - }; ` : ` const __apm$arguments = [${args}].slice(0, arguments.length); @@ -416,11 +427,18 @@ function wrap (state, node, program) { self: this, moduleVersion: ${JSON.stringify(moduleVersion)} }; - const __apm$traced = () => { - const __apm$wrapped = () => {}; - return __apm$wrapped.apply(this, __apm$arguments); - }; - `)).body + ` + const common = parse(` + function wrapper () { + ${declareSelf} + const __apm$traced = (__apm$callArgs) => { + const __apm$wrapped = () => {}; + return ${callWrapped}; + }; + if (!tr_ch_apm_hasSubscribers(${channelVariable})) return __apm$traced(${fastArgs}); + ${transport} + } + `).body[0].body.body block.body.unshift(...common) @@ -619,9 +637,9 @@ function wrapCallback (state, node, iterPatch = '') { function wrapper () { const __apm$cb = Array.prototype.at.call(__apm$arguments, ${callbackIndex}); - if (!${channelVariable}.start.hasSubscribers) return __apm$traced(); + if (!${channelVariable}.start.hasSubscribers) return __apm$traced(__apm$arguments); - function __apm$wrappedCb(err, res) { + const __apm$wrappedCb = function (err, res) { if (err) { __apm$ctx.error = err; ${channelVariable}.error.publish(__apm$ctx); @@ -639,16 +657,16 @@ function wrapCallback (state, node, iterPatch = '') { ${channelVariable}.asyncEnd.publish(__apm$ctx); } }); - } + }; if (typeof __apm$cb !== 'function') { - return __apm$traced(); + return __apm$traced(__apm$arguments); } Array.prototype.splice.call(__apm$arguments, ${callbackIndex}, 1, __apm$wrappedCb); return ${channelVariable}.start.runStores(__apm$ctx, () => { try { - return __apm$traced(); + return __apm$traced(__apm$arguments); } catch (err) { __apm$ctx.error = err; ${channelVariable}.error.publish(__apm$ctx); @@ -685,11 +703,9 @@ function wrapPromise (state, node, iterPatch = '') { // mutated. return parse(` function wrapper () { - if (!tr_ch_apm_hasSubscribers(${channelVariable})) return __apm$traced(); - return ${channelVariable}.start.runStores(__apm$ctx, () => { try { - let promise = __apm$traced(); + let promise = __apm$traced(__apm$arguments); if (typeof promise?.then !== 'function') { __apm$ctx.result = promise; ${iterPatch} @@ -770,11 +786,9 @@ function wrapSync (state, node, iterPatch = '') { // re-throws, so the trailing `return` is unreachable. return parse(` function wrapper () { - if (!tr_ch_apm_hasSubscribers(${channelVariable})) return __apm$traced(); - return ${channelVariable}.start.runStores(__apm$ctx, () => { try { - __apm$ctx.result = __apm$traced(); + __apm$ctx.result = __apm$traced(__apm$arguments); ${iterPatch} } catch (err) { __apm$ctx.error = err; @@ -836,8 +850,8 @@ function generateIterPatch (state, returnKind, program) { const __apm$orig = __apm$iter[method]; if (typeof __apm$orig !== 'function') return; __apm$iter[method] = function () { + if (!tr_ch_apm_hasSubscribers(${iterChannelVariable})) return __apm$orig.apply(this, arguments); const __apm$iterArgs = Array.prototype.slice.call(arguments); - if (!tr_ch_apm_hasSubscribers(${iterChannelVariable})) return __apm$orig.apply(this, __apm$iterArgs); __apm$ctx.method = method; __apm$ctx.arguments = __apm$iterArgs; return ${iterChannelVariable}.${traceMethod}(__apm$orig, __apm$ctx, this, ...__apm$iterArgs); diff --git a/tests/arguments_mutation_kinds_cjs/mod.js b/tests/arguments_mutation_kinds_cjs/mod.js new file mode 100644 index 0000000..d103bcb --- /dev/null +++ b/tests/arguments_mutation_kinds_cjs/mod.js @@ -0,0 +1,30 @@ +'use strict' + +function syncFn (a, ...rest) { + return { self: this, a, rest, len: arguments.length } +} + +async function asyncFn (a, ...rest) { + return { self: this, a, rest, len: arguments.length } +} + +function callbackFn (a, cb) { + cb(null, { self: this, a, len: arguments.length }) +} + +class Base { + constructor (a, ...rest) { + this.a = a + this.rest = rest + } +} + +class Derived extends Base { + constructor (a, ...rest) { + super(a, ...rest) + this.derivedA = a + this.derivedRest = rest + } +} + +module.exports = { syncFn, asyncFn, callbackFn, Base, Derived } diff --git a/tests/arguments_mutation_kinds_cjs/test.js b/tests/arguments_mutation_kinds_cjs/test.js new file mode 100644 index 0000000..f3f8e44 --- /dev/null +++ b/tests/arguments_mutation_kinds_cjs/test.js @@ -0,0 +1,80 @@ +'use strict' + +const { syncFn, asyncFn, callbackFn, Base, Derived } = require('./instrumented.js') +const assert = require('node:assert') +const { tracingChannel } = require('node:diagnostics_channel') + +// A `start` subscriber replaces the first argument and appends one in place; +// the original must see both, because the wrapper hands it the same array +// that was published as `message.arguments`. +function replaceAndAppend (name) { + tracingChannel(`orchestrion:undici:${name}`).subscribe({ + start (message) { + message.arguments[0] = 'replaced' + message.arguments.push('appended') + }, + }) +} + +for (const name of ['syncFn', 'asyncFn', 'Base_ctor', 'Derived_ctor']) replaceAndAppend(name) + +;(async () => { + const self = { self: true } + + assert.deepStrictEqual(syncFn.call(self, 'original', 'extra'), { + self, a: 'replaced', rest: ['extra', 'appended'], len: 3, + }) + + assert.deepStrictEqual(await asyncFn.call(self, 'original'), { + self, a: 'replaced', rest: ['appended'], len: 2, + }) + + // Constructors forward `__apm$arguments` by spreading it into the moved + // body, so parameters see the mutation. (`arguments` inside the body is the + // constructor's own and is not affected, as before.) + const base = new Base('original') + assert.strictEqual(base.a, 'replaced') + assert.deepStrictEqual(base.rest, ['appended']) + + // `super()` goes through the separately instrumented `Base`, whose own + // subscriber appends once more. + const derived = new Derived('original', 'extra') + assert.strictEqual(derived.derivedA, 'replaced') + assert.deepStrictEqual(derived.derivedRest, ['extra', 'appended']) + assert.strictEqual(derived.a, 'replaced') + assert.deepStrictEqual(derived.rest, ['extra', 'appended', 'appended']) + + // Callback: by the time `start` fires the wrapper has already spliced its + // own callback into `message.arguments`. A subscriber can wrap that in turn, + // and both layers must run. + { + const events = [] + tracingChannel('orchestrion:undici:callbackFn').subscribe({ + start (message) { + events.push('start') + message.arguments[0] = 'replaced' + const wrappedCb = message.arguments[1] + assert.strictEqual(typeof wrappedCb, 'function') + message.arguments[1] = function (err, res) { + events.push('subscriber-cb') + return wrappedCb.call(this, err, res) + } + }, + asyncStart () { events.push('asyncStart') }, + asyncEnd () { events.push('asyncEnd') }, + end () { events.push('end') }, + }) + + const result = await new Promise((resolve, reject) => { + callbackFn.call(self, 'original', (err, res) => { + events.push('user-cb') + err ? reject(err) : resolve(res) + }) + }) + assert.deepStrictEqual(result, { self, a: 'replaced', len: 2 }) + assert.deepStrictEqual(events, ['start', 'subscriber-cb', 'asyncStart', 'user-cb', 'asyncEnd', 'end']) + } +})().catch((err) => { + console.error(err) + process.exit(1) +}) diff --git a/tests/common/transport_spy.js b/tests/common/transport_spy.js new file mode 100644 index 0000000..62bd39a --- /dev/null +++ b/tests/common/transport_spy.js @@ -0,0 +1,34 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +'use strict' + +// The wrappers build their per-call transport with `[...].slice(...)` +// (`__apm$arguments`), `Array.prototype.at.call(...)` (callback lookup) and +// `Array.prototype.slice.call(arguments)` (iterator `next` arguments). Counting +// those calls while a single wrapped call runs synchronously tells us whether +// the wrapper took the no-subscriber fast path or built the transport. +const { slice, at } = Array.prototype + +function measure (fn) { + const counts = { slice: 0, at: 0 } + Array.prototype.slice = function (...args) { // eslint-disable-line no-extend-native + counts.slice++ + return slice.apply(this, args) + } + Array.prototype.at = function (...args) { // eslint-disable-line no-extend-native + counts.at++ + return at.apply(this, args) + } + let value + try { + value = fn() + } finally { + Array.prototype.slice = slice // eslint-disable-line no-extend-native + Array.prototype.at = at // eslint-disable-line no-extend-native + } + return { value, slice: counts.slice, at: counts.at } +} + +module.exports = { measure } diff --git a/tests/fast_path_cjs/mod.js b/tests/fast_path_cjs/mod.js new file mode 100644 index 0000000..6a2006a --- /dev/null +++ b/tests/fast_path_cjs/mod.js @@ -0,0 +1,82 @@ +'use strict' + +// Every original records what it was actually called with, so the test can +// compare the unsubscribed fast path against the subscribed path. +function record (self, args, named) { + return { self, len: args.length, args: Array.from(args), ...named } +} + +function syncFn (a, b = 'b-default', ...rest) { + return record(this, arguments, { a, b, rest }) +} + +async function asyncFn (a, b = 'b-default', ...rest) { + return record(this, arguments, { a, b, rest }) +} + +function callbackFn (a, cb) { + const cbArg = arguments[arguments.length - 1] + cbArg(null, record(this, arguments, { a, cb: cbArg })) +} + +function autoFn (a, cb) { + const last = arguments[arguments.length - 1] + if (typeof last === 'function') { + last(null, record(this, arguments, { a, cb: last })) + } else { + return Promise.resolve(record(this, arguments, { a })) + } +} + +function * iterFn (a) { + const sent = yield record(this, arguments, { a }) + yield sent +} + +async function * asyncIterFn (a) { + const sent = yield record(this, arguments, { a }) + yield sent +} + +const arrowFn = (a, b = 'b-default', ...rest) => ({ a, b, rest }) + +class Service { + method (a, b = 'b-default', ...rest) { + return record(this, arguments, { a, b, rest }) + } +} + +class Base { + constructor (a, b = 'b-default', ...rest) { + this.base = record(undefined, arguments, { a, b, rest }) + this.newTarget = new.target + } +} + +class Derived extends Base { + constructor (a, b = 'd-default', ...rest) { + super(a, b, ...rest) + this.derived = record(undefined, arguments, { a, b, rest }) + } +} + +// Not declared on the class body, so it is patched onto the instance from a +// synthesised constructor at runtime. +class Holder {} +Holder.prototype.run = function (a, b = 'b-default', ...rest) { + return record(this, arguments, { a, b, rest }) +} + +module.exports = { + syncFn, + asyncFn, + callbackFn, + autoFn, + iterFn, + asyncIterFn, + arrowFn, + Service, + Base, + Derived, + Holder, +} diff --git a/tests/fast_path_cjs/test.js b/tests/fast_path_cjs/test.js new file mode 100644 index 0000000..841b96f --- /dev/null +++ b/tests/fast_path_cjs/test.js @@ -0,0 +1,262 @@ +'use strict' + +const m = require('./instrumented.js') +const assert = require('node:assert') +const { tracingChannel } = require('node:diagnostics_channel') +const { measure } = require('../common/transport_spy.js') + +// Fewer, exactly as many, and more arguments than the original declares. +const SHAPES = [['fewer', [1]], ['exact', [1, 2]], ['more', [1, 2, 3, 4]]] +const EVENTS = ['start', 'end', 'asyncStart', 'asyncEnd', 'error'] + +function watch (name, events = EVENTS) { + const seen = [] + const handlers = {} + for (const event of events) handlers[event] = () => seen.push(event) + const channel = tracingChannel(`orchestrion:undici:${name}`) + return { + seen, + subscribe () { channel.subscribe(handlers) }, + unsubscribe () { channel.unsubscribe(handlers) }, + } +} + +function assertParams (rec, callArgs, bDefault = 'b-default') { + assert.strictEqual(rec.a, callArgs[0]) + assert.strictEqual(rec.b, callArgs.length > 1 ? callArgs[1] : bDefault) + assert.deepStrictEqual(rec.rest, callArgs.slice(2)) +} + +function assertRecord (rec, self, callArgs, bDefault) { + assert.strictEqual(rec.self, self) + assert.strictEqual(rec.len, callArgs.length) + assert.deepStrictEqual(rec.args, callArgs) + assertParams(rec, callArgs, bDefault) +} + +function callWithCallback (fn, self, callArgs, ctx) { + return new Promise((resolve, reject) => { + ctx.cb = (err, rec) => err ? reject(err) : resolve(rec) + fn.call(self, ...callArgs, ctx.cb) + }) +} + +function assertCallbackRecord (rec, self, callArgs, ctx, subscribed) { + assert.strictEqual(rec.self, self) + assert.strictEqual(rec.len, callArgs.length + 1) + assert.deepStrictEqual(rec.args.slice(0, -1), callArgs) + assert.strictEqual(rec.a, callArgs[0]) + if (subscribed) { + assert.notStrictEqual(rec.cb, ctx.cb, 'subscribed calls receive the wrapped callback') + } else { + assert.strictEqual(rec.cb, ctx.cb, 'unsubscribed calls receive the caller\'s callback untouched') + } +} + +const self = { self: true } +const service = new m.Service() +const holder = new m.Holder() + +const cases = [ + { + channel: 'syncFn', + events: ['start', 'end'], + observable: true, + call: (callArgs) => m.syncFn.call(self, ...callArgs), + check: (rec, callArgs) => assertRecord(rec, self, callArgs), + }, + { + channel: 'asyncFn', + events: ['start', 'end', 'asyncStart', 'asyncEnd'], + observable: true, + call: (callArgs) => m.asyncFn.call(self, ...callArgs), + check: (rec, callArgs) => assertRecord(rec, self, callArgs), + }, + { + channel: 'callbackFn', + events: ['start', 'asyncStart', 'asyncEnd', 'end'], + observable: true, + call: (callArgs, ctx) => callWithCallback(m.callbackFn, self, callArgs, ctx), + check: (rec, callArgs, { ctx, subscribed }) => assertCallbackRecord(rec, self, callArgs, ctx, subscribed), + }, + { + channel: 'autoFn', + label: 'autoFn (callback)', + events: ['start', 'asyncStart', 'asyncEnd', 'end'], + observable: true, + call: (callArgs, ctx) => callWithCallback(m.autoFn, self, callArgs, ctx), + check: (rec, callArgs, { ctx, subscribed }) => assertCallbackRecord(rec, self, callArgs, ctx, subscribed), + }, + { + channel: 'autoFn', + label: 'autoFn (promise)', + events: ['start', 'end', 'asyncStart', 'asyncEnd'], + observable: true, + call: (callArgs) => m.autoFn.call(self, ...callArgs), + check: (rec, callArgs) => { + assert.strictEqual(rec.self, self) + assert.strictEqual(rec.len, callArgs.length) + assert.deepStrictEqual(rec.args, callArgs) + }, + }, + { + channel: 'iterFn', + events: ['start', 'end'], + observable: true, + call: (callArgs) => m.iterFn.call(self, ...callArgs), + check: (iter, callArgs, { label }) => { + // Only the main channel is subscribed here, never `:next`, so `next` + // must forward its argument without building `__apm$iterArgs`. + const first = measure(() => iter.next()) + assert.strictEqual(first.slice, 0, `${label}: next() copied its arguments`) + assert.strictEqual(first.value.value.self, self) + assert.deepStrictEqual(first.value.value.args, callArgs) + const second = measure(() => iter.next('sent')) + assert.strictEqual(second.slice, 0, `${label}: next('sent') copied its arguments`) + assert.deepStrictEqual(second.value, { value: 'sent', done: false }) + }, + }, + { + channel: 'asyncIterFn', + events: ['start', 'end'], + observable: true, + call: (callArgs) => m.asyncIterFn.call(self, ...callArgs), + check: async (iter, callArgs, { label }) => { + const first = measure(() => iter.next()) + assert.strictEqual(first.slice, 0, `${label}: next() copied its arguments`) + const { value: rec } = await first.value + assert.strictEqual(rec.self, self) + assert.deepStrictEqual(rec.args, callArgs) + const second = measure(() => iter.next('sent')) + assert.strictEqual(second.slice, 0, `${label}: next('sent') copied its arguments`) + assert.deepStrictEqual(await second.value, { value: 'sent', done: false }) + }, + }, + { + channel: 'arrowFn', + events: ['start', 'end'], + observable: false, + call: (callArgs) => m.arrowFn(...callArgs), + check: (rec, callArgs) => assertParams(rec, callArgs), + }, + { + channel: 'Service_method', + events: ['start', 'end'], + observable: true, + call: (callArgs) => service.method(...callArgs), + check: (rec, callArgs) => assertRecord(rec, service, callArgs), + }, + { + channel: 'Holder_run', + events: ['start', 'end'], + observable: true, + call: (callArgs) => holder.run(...callArgs), + check: (rec, callArgs) => assertRecord(rec, holder, callArgs), + }, + { + channel: 'Base_ctor', + events: ['start', 'end'], + observable: false, + call: (callArgs) => new m.Base(...callArgs), + check: (instance, callArgs) => { + assert.ok(instance instanceof m.Base) + assert.strictEqual(instance.newTarget, m.Base) + assertRecord(instance.base, undefined, callArgs) + }, + }, + { + // Also guards the derived-constructor fast path against a TDZ error if a + // binding assigned at the `super()` call site is ever declared below the + // subscriber check. + channel: 'Derived_ctor', + events: ['start', 'end'], + observable: false, + call: (callArgs) => new m.Derived(...callArgs), + check: (instance, callArgs) => { + assert.ok(instance instanceof m.Derived) + assert.ok(instance instanceof m.Base) + assert.strictEqual(instance.newTarget, m.Derived) + assertRecord(instance.derived, undefined, callArgs, 'd-default') + const superArgs = [callArgs[0], callArgs.length > 1 ? callArgs[1] : 'd-default', ...callArgs.slice(2)] + assertRecord(instance.base, undefined, superArgs) + }, + }, +] + +async function runCase (c) { + const label = c.label ?? c.channel + const w = watch(c.channel) + // Unsubscribed, then subscribed, then unsubscribed again. + const phases = [['before', false], ['subscribed', true], ['after', false]] + for (const [phase, subscribed] of phases) { + if (phase === 'subscribed') w.subscribe() + if (phase === 'after') w.unsubscribe() + for (const [shape, callArgs] of SHAPES) { + const info = { label: `${label} ${phase} ${shape}`, subscribed, ctx: {} } + w.seen.length = 0 + const { value, slice, at } = measure(() => c.call(callArgs, info.ctx)) + if (!subscribed) { + assert.strictEqual(slice, 0, `${info.label}: built the arguments array`) + assert.strictEqual(at, 0, `${info.label}: looked up the callback`) + } else if (c.observable) { + assert.ok(slice + at > 0, `${info.label}: expected the subscribed path to build its transport`) + } + await c.check(await value, callArgs, info) + assert.deepStrictEqual(w.seen, subscribed ? c.events : [], info.label) + } + } +} + +;(async () => { + assert.strictEqual(m.syncFn.length, 1) + assert.strictEqual(m.callbackFn.length, 2) + assert.strictEqual(holder.run.length, 1) + + for (const c of cases) await runCase(c) + + // Iterator `next` once `:next` is subscribed: traced, and its argument is + // still forwarded. + { + const main = watch('iterFn') + const next = watch('iterFn:next', ['start', 'end']) + main.subscribe() + next.subscribe() + const iter = m.iterFn.call(self, 1) + iter.next() + assert.deepStrictEqual(iter.next('sent'), { value: 'sent', done: false }) + assert.deepStrictEqual(next.seen, ['start', 'end', 'start', 'end']) + next.unsubscribe() + assert.deepStrictEqual(iter.next('more'), { value: undefined, done: true }) + assert.deepStrictEqual(next.seen, ['start', 'end', 'start', 'end']) + main.unsubscribe() + } + + // Callback/Auto with only `asyncEnd` subscribed. Pins existing behaviour: + // the callback paths still only trace when `start` has subscribers, while + // the Auto promise fallback traces when any event does. + { + const w = watch('callbackFn', ['asyncEnd']) + w.subscribe() + const ctx = {} + const rec = await callWithCallback(m.callbackFn, self, [1], ctx) + assert.strictEqual(rec.cb, ctx.cb) + assert.deepStrictEqual(w.seen, []) + w.unsubscribe() + } + { + const w = watch('autoFn', ['asyncEnd']) + w.subscribe() + const ctx = {} + const rec = await callWithCallback(m.autoFn, self, [1], ctx) + assert.strictEqual(rec.cb, ctx.cb) + assert.deepStrictEqual(w.seen, []) + + const promised = await m.autoFn.call(self, 1) + assert.deepStrictEqual(promised.args, [1]) + assert.deepStrictEqual(w.seen, ['asyncEnd']) + w.unsubscribe() + } +})().catch((err) => { + console.error(err) + process.exit(1) +}) diff --git a/tests/tests.test.mjs b/tests/tests.test.mjs index 97ef14b..b57944c 100644 --- a/tests/tests.test.mjs +++ b/tests/tests.test.mjs @@ -7,6 +7,8 @@ import { spawnSync } from 'node:child_process' import { join, dirname } from 'node:path' import { fileURLToPath } from 'node:url' import { SourceMapConsumer } from 'source-map' +import esquery from 'esquery' +import { parse } from 'meriyah' const __dirname = dirname(fileURLToPath(import.meta.url)) @@ -76,6 +78,19 @@ describe('arguments_mutation', () => { }) }) +describe('arguments_mutation_kinds_cjs', () => { + test('in-place argument mutation reaches the original for Sync, Async, Callback and constructors', () => { + const M = { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH } + runTest('arguments_mutation_kinds_cjs', [ + { channelName: 'syncFn', module: M, functionQuery: { functionName: 'syncFn', kind: 'Sync' } }, + { channelName: 'asyncFn', module: M, functionQuery: { functionName: 'asyncFn', kind: 'Async' } }, + { channelName: 'callbackFn', module: M, functionQuery: { functionName: 'callbackFn', kind: 'Callback', callbackIndex: 1 } }, + { channelName: 'Base_ctor', module: M, functionQuery: { className: 'Base' } }, + { channelName: 'Derived_ctor', module: M, functionQuery: { className: 'Derived' } }, + ]) + }) +}) + describe('class_expression_cjs', () => { test('instruments async class method on class expression', () => { runTest('class_expression_cjs', [ @@ -946,6 +961,98 @@ describe('async_iterator_cjs', () => { }) }) +describe('fast_path_cjs', () => { + const M = { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH } + + test('unsubscribed calls skip the per-call transport for every wrapper kind', () => { + runTest('fast_path_cjs', [ + { channelName: 'syncFn', module: M, functionQuery: { functionName: 'syncFn', kind: 'Sync' } }, + { channelName: 'asyncFn', module: M, functionQuery: { functionName: 'asyncFn', kind: 'Async' } }, + { channelName: 'callbackFn', module: M, functionQuery: { functionName: 'callbackFn', kind: 'Callback' } }, + { channelName: 'autoFn', module: M, functionQuery: { functionName: 'autoFn', kind: 'Auto' } }, + { channelName: 'iterFn', module: M, functionQuery: { functionName: 'iterFn', kind: 'Sync', returnKind: 'Iterator' } }, + { channelName: 'asyncIterFn', module: M, functionQuery: { functionName: 'asyncIterFn', kind: 'Sync', returnKind: 'AsyncIterator' } }, + { channelName: 'arrowFn', module: M, functionQuery: { expressionName: 'arrowFn', kind: 'Sync' } }, + { channelName: 'Service_method', module: M, functionQuery: { className: 'Service', methodName: 'method', kind: 'Sync' } }, + { channelName: 'Holder_run', module: M, functionQuery: { className: 'Holder', methodName: 'run', kind: 'Sync' } }, + { channelName: 'Base_ctor', module: M, functionQuery: { className: 'Base' } }, + { channelName: 'Derived_ctor', module: M, functionQuery: { className: 'Derived' } }, + ]) + }) +}) + +describe('fast path ordering', () => { + const M = { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH } + + const transform = (code, functionQuery) => + create([{ channelName: 'ch', module: M, functionQuery }]) + .getTransformer(TEST_MODULE_NAME, TEST_MODULE_VERSION, TEST_MODULE_PATH) + .transform(code, 'cjs').code + + const declares = (name) => (stmt) => + stmt.type === 'VariableDeclaration' && stmt.declarations.some(d => d.id.name === name) + const isTracedCall = (node) => + node?.type === 'CallExpression' && node.callee.type === 'Identifier' && node.callee.name === '__apm$traced' + const isGuard = (stmt) => + stmt.type === 'IfStatement' && + stmt.test.operator === '!' && + stmt.test.argument.callee?.name === 'tr_ch_apm_hasSubscribers' + // `wrapSuper` hoists its `__apm$super` capture above everything else. + const isSuperCapture = (stmt) => + declares('__apm$super')(stmt) || + (stmt.type === 'ExpressionStatement' && stmt.expression.left?.object?.name === '__apm$super') + // A derived constructor declares the `let` that its `super()` call sites + // assign, and the fast path runs those call sites too. + const isSelfDeclaration = declares('tr_ch_apm$ch$self') + + const cases = [ + ['Sync', 'function f (a, b) { return a }', { functionName: 'f', kind: 'Sync' }, 2], + ['Async', 'async function f (a) { return a }', { functionName: 'f', kind: 'Async' }, 2], + ['Callback', 'function f (a, cb) { cb(null, a) }', { functionName: 'f', kind: 'Callback' }], + ['Auto', 'function f (a, cb) { cb(null, a) }', { functionName: 'f', kind: 'Auto' }], + ['Iterator', 'function * f (a) { yield a }', { functionName: 'f', kind: 'Sync', returnKind: 'Iterator' }, 2], + ['AsyncIterator', 'async function * f (a) { yield a }', { functionName: 'f', kind: 'Async', returnKind: 'AsyncIterator' }, 2], + ['arrow expression', 'const f = (a) => a', { expressionName: 'f', kind: 'Sync' }, 2], + ['class method', 'class C { m (a) { return a } }', { className: 'C', methodName: 'm', kind: 'Sync' }, 2], + ['class method using super', 'class B { m () {} }\nclass C extends B { m (a) { return super.m(a) } }', { className: 'C', methodName: 'm', kind: 'Async' }, 2], + ['derived constructor', 'class B {}\nclass C extends B { constructor (a) { super(); this.a = a } }', { className: 'C' }, 2], + ['runtime-patched instance method', 'class C {}', { className: 'C', methodName: 'm', kind: 'Async' }, 2], + ] + + for (const [label, code, functionQuery, tracedCalls] of cases) { + test(`checks for subscribers before building the transport: ${label}`, () => { + const ast = parse(transform(code, functionQuery), { next: true }) + const wrappers = esquery.query(ast, ':function > BlockStatement') + .filter(block => block.body.some(declares('__apm$ctx'))) + assert.equal(wrappers.length, 1) + + const body = wrappers[0].body + const guard = body.findIndex(isGuard) + assert.ok(guard > 0, 'missing subscriber guard') + assert.ok(guard < body.findIndex(declares('__apm$arguments')), 'guard must precede __apm$arguments') + assert.ok(guard < body.findIndex(declares('__apm$ctx')), 'guard must precede __apm$ctx') + assert.ok(isTracedCall(body[guard].consequent.argument), 'guard must return __apm$traced(...)') + for (const stmt of body.slice(0, guard)) { + assert.ok(declares('__apm$traced')(stmt) || isSuperCapture(stmt) || isSelfDeclaration(stmt), 'only __apm$traced may precede the guard') + } + + if (tracedCalls !== undefined) { + const calls = esquery.query(wrappers[0], 'CallExpression[callee.name="__apm$traced"]') + assert.equal(calls.length, tracedCalls) + } + + if (functionQuery.returnKind) { + // `wrapPromise` inlines the patch once per settlement path. + const patched = esquery.query(wrappers[0], 'AssignmentExpression[left.object.name="__apm$iter"] > FunctionExpression') + assert.ok(patched.length > 0) + for (const fn of patched) { + assert.ok(isGuard(fn.body.body[0]), 'iterator method must check for subscribers first') + } + } + }) + } +}) + describe('idempotency', () => { const M = { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH } From 54a16294301dbeda57d805bed1c4767771660556 Mon Sep 17 00:00:00 2001 From: Pablo Erhard Date: Mon, 5 Oct 2026 15:22:27 -0400 Subject: [PATCH 2/2] docs: reword the wrap() comment and note the transport spy's scope Reword the wrap() doc comment as suggested in review, and explain why the Array.prototype spy used by the fast path fixtures stays contained. Signed-off-by: Pablo Erhard --- lib/transforms.js | 2 +- tests/common/transport_spy.js | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/transforms.js b/lib/transforms.js index 16cab2f..1fe0330 100644 --- a/lib/transforms.js +++ b/lib/transforms.js @@ -372,7 +372,7 @@ function traceInstanceMethod (state, node, program) { * resulting block statement body. * * The no-subscriber check sits between the two halves of the preamble: after - * `__apm$traced` (which only captures the call arguments it is given) and + * `__apm$traced` (which takes the call arguments as its parameter) and * before `__apm$arguments` / `__apm$ctx`, so an unsubscribed call does not * build any of the per-call transport. * diff --git a/tests/common/transport_spy.js b/tests/common/transport_spy.js index 62bd39a..0311d53 100644 --- a/tests/common/transport_spy.js +++ b/tests/common/transport_spy.js @@ -9,6 +9,13 @@ // `Array.prototype.slice.call(arguments)` (iterator `next` arguments). Counting // those calls while a single wrapped call runs synchronously tells us whether // the wrapper took the no-subscriber fast path or built the transport. +// +// Replacing the prototype methods is process-wide, so keep the window small: +// they are only swapped for one synchronous call and always restored, and each +// fixture runs in its own `node` process. Callers should only assert zero +// counts on the fast path and a non-zero count on the subscribed path, never +// exact numbers. The "fast path ordering" test in tests.test.mjs checks the +// same property on the generated code without patching anything. const { slice, at } = Array.prototype function measure (fn) {