From 4f3cf4e89ace2a6b74eaabd7bac4a7dae84632b1 Mon Sep 17 00:00:00 2001 From: Stephen Belanger Date: Wed, 6 Mar 2024 19:03:32 -0800 Subject: [PATCH] diagnostics_channel: early-exit tracing channel trace methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/51915 Reviewed-By: Yagiz Nizipli Reviewed-By: Gerhard Stöbich Reviewed-By: Chengzhong Wu --- doc/api/diagnostics_channel.md | 20 ++++++ lib/diagnostics_channel.js | 66 ++++++++++++------- ...tics-channel-tracing-channel-args-types.js | 2 +- ...agnostics-channel-tracing-channel-async.js | 60 ----------------- ...nel-tracing-channel-callback-early-exit.js | 19 ++++++ ...channel-tracing-channel-callback-error.js} | 20 ++---- ...ostics-channel-tracing-channel-callback.js | 43 ++++++++++++ ...nnel-tracing-channel-promise-early-exit.js | 21 ++++++ ...s-channel-tracing-channel-promise-error.js | 38 +++++++++++ ...nostics-channel-tracing-channel-promise.js | 41 ++++++++++++ ...channel-tracing-channel-sync-early-exit.js | 20 ++++++ ...hannel-tracing-channel-sync-run-stores.js} | 0 12 files changed, 250 insertions(+), 100 deletions(-) delete mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-async.js create mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-callback-early-exit.js rename test/parallel/{test-diagnostics-channel-tracing-channel-async-error.js => test-diagnostics-channel-tracing-channel-callback-error.js} (64%) create mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-callback.js create mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-promise-early-exit.js create mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-promise-error.js create mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-promise.js create mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-sync-early-exit.js rename test/parallel/{test-diagnostics-channel-tracing-channel-run-stores.js => test-diagnostics-channel-tracing-channel-sync-run-stores.js} (100%) diff --git a/doc/api/diagnostics_channel.md b/doc/api/diagnostics_channel.md index 7b68b3b8d1c..e27c55eedf3 100644 --- a/doc/api/diagnostics_channel.md +++ b/doc/api/diagnostics_channel.md @@ -789,6 +789,11 @@ if the given function throws an error. This will run the given function using [`channel.runStores(context, ...)`][] on the `start` channel which ensures all events should have any bound stores set to match this trace context. +To ensure only correct trace graphs are formed, events will only be published +if subscribers are present prior to starting the trace. Subscriptions which are +added after the trace begins will not receive future events from that trace, +only future traces will be seen. + ```mjs import diagnostics_channel from 'node:diagnostics_channel'; @@ -838,6 +843,11 @@ returned promise rejects. This will run the given function using [`channel.runStores(context, ...)`][] on the `start` channel which ensures all events should have any bound stores set to match this trace context. +To ensure only correct trace graphs are formed, events will only be published +if subscribers are present prior to starting the trace. Subscriptions which are +added after the trace begins will not receive future events from that trace, +only future traces will be seen. + ```mjs import diagnostics_channel from 'node:diagnostics_channel'; @@ -891,6 +901,11 @@ the callback is set. This will run the given function using [`channel.runStores(context, ...)`][] on the `start` channel which ensures all events should have any bound stores set to match this trace context. +To ensure only correct trace graphs are formed, events will only be published +if subscribers are present prior to starting the trace. Subscriptions which are +added after the trace begins will not receive future events from that trace, +only future traces will be seen. + ```mjs import diagnostics_channel from 'node:diagnostics_channel'; @@ -979,6 +994,11 @@ of the callback while the `error` will either be a thrown error visible in the `end` event or the first callback argument in either of the `asyncStart` or `asyncEnd` events. +To ensure only correct trace graphs are formed, events should only be published +if subscribers are present prior to starting the trace. Subscriptions which are +added after the trace begins should not receive future events from that trace, +only future traces will be seen. + Tracing channels should follow a naming pattern of: * `tracing:module.class.method:start` or `tracing:module.function:start` diff --git a/lib/diagnostics_channel.js b/lib/diagnostics_channel.js index 97a86805ce7..68f604e719d 100644 --- a/lib/diagnostics_channel.js +++ b/lib/diagnostics_channel.js @@ -6,6 +6,7 @@ const { ArrayPrototypePush, ArrayPrototypeSplice, SafeFinalizationRegistry, + ObjectDefineProperty, ObjectGetPrototypeOf, ObjectSetPrototypeOf, Promise, @@ -250,35 +251,40 @@ function assertChannel(value, name) { } } +function tracingChannelFrom(nameOrChannels, name) { + if (typeof nameOrChannels === 'string') { + return channel(`tracing:${nameOrChannels}:${name}`); + } + + if (typeof nameOrChannels === 'object' && nameOrChannels !== null) { + const channel = nameOrChannels[name]; + assertChannel(channel, `nameOrChannels.${name}`); + return channel; + } + + throw new ERR_INVALID_ARG_TYPE('nameOrChannels', + ['string', 'object', 'TracingChannel'], + nameOrChannels); +} + class TracingChannel { constructor(nameOrChannels) { - if (typeof nameOrChannels === 'string') { - this.start = channel(`tracing:${nameOrChannels}:start`); - this.end = channel(`tracing:${nameOrChannels}:end`); - this.asyncStart = channel(`tracing:${nameOrChannels}:asyncStart`); - this.asyncEnd = channel(`tracing:${nameOrChannels}:asyncEnd`); - this.error = channel(`tracing:${nameOrChannels}:error`); - } else if (typeof nameOrChannels === 'object') { - const { start, end, asyncStart, asyncEnd, error } = nameOrChannels; - - assertChannel(start, 'nameOrChannels.start'); - assertChannel(end, 'nameOrChannels.end'); - assertChannel(asyncStart, 'nameOrChannels.asyncStart'); - assertChannel(asyncEnd, 'nameOrChannels.asyncEnd'); - assertChannel(error, 'nameOrChannels.error'); - - this.start = start; - this.end = end; - this.asyncStart = asyncStart; - this.asyncEnd = asyncEnd; - this.error = error; - } else { - throw new ERR_INVALID_ARG_TYPE('nameOrChannels', - ['string', 'object', 'Channel'], - nameOrChannels); + for (const eventName of traceEvents) { + ObjectDefineProperty(this, eventName, { + __proto__: null, + value: tracingChannelFrom(nameOrChannels, eventName), + }); } } + get hasSubscribers() { + return this.start.hasSubscribers || + this.end.hasSubscribers || + this.asyncStart.hasSubscribers || + this.asyncEnd.hasSubscribers || + this.error.hasSubscribers; + } + subscribe(handlers) { for (const name of traceEvents) { if (!handlers[name]) continue; @@ -302,6 +308,10 @@ class TracingChannel { } traceSync(fn, context = {}, thisArg, ...args) { + if (!this.hasSubscribers) { + return ReflectApply(fn, thisArg, args); + } + const { start, end, error } = this; return start.runStores(context, () => { @@ -320,6 +330,10 @@ class TracingChannel { } tracePromise(fn, context = {}, thisArg, ...args) { + if (!this.hasSubscribers) { + return ReflectApply(fn, thisArg, args); + } + const { start, end, asyncStart, asyncEnd, error } = this; function reject(err) { @@ -358,6 +372,10 @@ class TracingChannel { } traceCallback(fn, position = -1, context = {}, thisArg, ...args) { + if (!this.hasSubscribers) { + return ReflectApply(fn, thisArg, args); + } + const { start, end, asyncStart, asyncEnd, error } = this; function wrappedCallback(err, res) { diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-args-types.js b/test/parallel/test-diagnostics-channel-tracing-channel-args-types.js index 5ae55badc4a..a96b303aa15 100644 --- a/test/parallel/test-diagnostics-channel-tracing-channel-args-types.js +++ b/test/parallel/test-diagnostics-channel-tracing-channel-args-types.js @@ -24,7 +24,7 @@ assert.throws(() => (channel = dc.tracingChannel(0)), { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError', message: - /The "nameOrChannels" argument must be of type string or an instance of Channel or Object/, + /The "nameOrChannels" argument must be of type string or an instance of TracingChannel or Object/, }); // tracingChannel creating without instance of Channel must throw error diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-async.js b/test/parallel/test-diagnostics-channel-tracing-channel-async.js deleted file mode 100644 index 03bdc85fb14..00000000000 --- a/test/parallel/test-diagnostics-channel-tracing-channel-async.js +++ /dev/null @@ -1,60 +0,0 @@ -'use strict'; - -const common = require('../common'); -const dc = require('diagnostics_channel'); -const assert = require('assert'); - -const channel = dc.tracingChannel('test'); - -const expectedResult = { foo: 'bar' }; -const input = { foo: 'bar' }; -const thisArg = { baz: 'buz' }; - -function check(found) { - assert.deepStrictEqual(found, input); -} - -const handlers = { - start: common.mustCall(check, 2), - end: common.mustCall(check, 2), - asyncStart: common.mustCall((found) => { - check(found); - assert.strictEqual(found.error, undefined); - assert.deepStrictEqual(found.result, expectedResult); - }, 2), - asyncEnd: common.mustCall((found) => { - check(found); - assert.strictEqual(found.error, undefined); - assert.deepStrictEqual(found.result, expectedResult); - }, 2), - error: common.mustNotCall() -}; - -channel.subscribe(handlers); - -channel.traceCallback(function(cb, err, res) { - assert.deepStrictEqual(this, thisArg); - setImmediate(cb, err, res); -}, 0, input, thisArg, common.mustCall((err, res) => { - assert.strictEqual(err, null); - assert.deepStrictEqual(res, expectedResult); -}), null, expectedResult); - -channel.tracePromise(function(value) { - assert.deepStrictEqual(this, thisArg); - return Promise.resolve(value); -}, input, thisArg, expectedResult).then( - common.mustCall((value) => { - assert.deepStrictEqual(value, expectedResult); - }), - common.mustNotCall() -); - -let failed = false; -try { - channel.traceCallback(common.mustNotCall(), 0, input, thisArg, 1, 2, 3); -} catch (err) { - assert.ok(/"callback" argument must be of type function/.test(err.message)); - failed = true; -} -assert.strictEqual(failed, true); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-callback-early-exit.js b/test/parallel/test-diagnostics-channel-tracing-channel-callback-early-exit.js new file mode 100644 index 00000000000..6ba5fd17bb4 --- /dev/null +++ b/test/parallel/test-diagnostics-channel-tracing-channel-callback-early-exit.js @@ -0,0 +1,19 @@ +'use strict'; + +const common = require('../common'); +const dc = require('diagnostics_channel'); + +const channel = dc.tracingChannel('test'); + +const handlers = { + start: common.mustNotCall(), + end: common.mustNotCall(), + asyncStart: common.mustNotCall(), + asyncEnd: common.mustNotCall(), + error: common.mustNotCall() +}; + +// While subscribe occurs _before_ the callback executes, +// no async events should be published. +channel.traceCallback(setImmediate, 0, {}, null, common.mustCall()); +channel.subscribe(handlers); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-async-error.js b/test/parallel/test-diagnostics-channel-tracing-channel-callback-error.js similarity index 64% rename from test/parallel/test-diagnostics-channel-tracing-channel-async-error.js rename to test/parallel/test-diagnostics-channel-tracing-channel-callback-error.js index 7335e15de9d..672500e7683 100644 --- a/test/parallel/test-diagnostics-channel-tracing-channel-async-error.js +++ b/test/parallel/test-diagnostics-channel-tracing-channel-callback-error.js @@ -15,14 +15,14 @@ function check(found) { } const handlers = { - start: common.mustCall(check, 2), - end: common.mustCall(check, 2), - asyncStart: common.mustCall(check, 2), - asyncEnd: common.mustCall(check, 2), + start: common.mustCall(check), + end: common.mustCall(check), + asyncStart: common.mustCall(check), + asyncEnd: common.mustCall(check), error: common.mustCall((found) => { check(found); assert.deepStrictEqual(found.error, expectedError); - }, 2) + }) }; channel.subscribe(handlers); @@ -34,13 +34,3 @@ channel.traceCallback(function(cb, err) { assert.strictEqual(err, expectedError); assert.strictEqual(res, undefined); }), expectedError); - -channel.tracePromise(function(value) { - assert.deepStrictEqual(this, thisArg); - return Promise.reject(value); -}, input, thisArg, expectedError).then( - common.mustNotCall(), - common.mustCall((value) => { - assert.deepStrictEqual(value, expectedError); - }) -); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-callback.js b/test/parallel/test-diagnostics-channel-tracing-channel-callback.js new file mode 100644 index 00000000000..d306f0f51b7 --- /dev/null +++ b/test/parallel/test-diagnostics-channel-tracing-channel-callback.js @@ -0,0 +1,43 @@ +'use strict'; + +const common = require('../common'); +const dc = require('diagnostics_channel'); +const assert = require('assert'); + +const channel = dc.tracingChannel('test'); + +const expectedResult = { foo: 'bar' }; +const input = { foo: 'bar' }; +const thisArg = { baz: 'buz' }; + +function check(found) { + assert.deepStrictEqual(found, input); +} + +function checkAsync(found) { + check(found); + assert.strictEqual(found.error, undefined); + assert.deepStrictEqual(found.result, expectedResult); +} + +const handlers = { + start: common.mustCall(check), + end: common.mustCall(check), + asyncStart: common.mustCall(checkAsync), + asyncEnd: common.mustCall(checkAsync), + error: common.mustNotCall() +}; + +channel.subscribe(handlers); + +channel.traceCallback(function(cb, err, res) { + assert.deepStrictEqual(this, thisArg); + setImmediate(cb, err, res); +}, 0, input, thisArg, common.mustCall((err, res) => { + assert.strictEqual(err, null); + assert.deepStrictEqual(res, expectedResult); +}), null, expectedResult); + +assert.throws(() => { + channel.traceCallback(common.mustNotCall(), 0, input, thisArg, 1, 2, 3); +}, /"callback" argument must be of type function/); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-promise-early-exit.js b/test/parallel/test-diagnostics-channel-tracing-channel-promise-early-exit.js new file mode 100644 index 00000000000..fce7f40b753 --- /dev/null +++ b/test/parallel/test-diagnostics-channel-tracing-channel-promise-early-exit.js @@ -0,0 +1,21 @@ +'use strict'; + +const common = require('../common'); +const dc = require('diagnostics_channel'); + +const channel = dc.tracingChannel('test'); + +const handlers = { + start: common.mustNotCall(), + end: common.mustNotCall(), + asyncStart: common.mustNotCall(), + asyncEnd: common.mustNotCall(), + error: common.mustNotCall() +}; + +// While subscribe occurs _before_ the promise resolves, +// no async events should be published. +channel.tracePromise(() => { + return new Promise(setImmediate); +}, {}); +channel.subscribe(handlers); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-promise-error.js b/test/parallel/test-diagnostics-channel-tracing-channel-promise-error.js new file mode 100644 index 00000000000..f1f52d72f80 --- /dev/null +++ b/test/parallel/test-diagnostics-channel-tracing-channel-promise-error.js @@ -0,0 +1,38 @@ +'use strict'; + +const common = require('../common'); +const dc = require('diagnostics_channel'); +const assert = require('assert'); + +const channel = dc.tracingChannel('test'); + +const expectedError = new Error('test'); +const input = { foo: 'bar' }; +const thisArg = { baz: 'buz' }; + +function check(found) { + assert.deepStrictEqual(found, input); +} + +const handlers = { + start: common.mustCall(check), + end: common.mustCall(check), + asyncStart: common.mustCall(check), + asyncEnd: common.mustCall(check), + error: common.mustCall((found) => { + check(found); + assert.deepStrictEqual(found.error, expectedError); + }) +}; + +channel.subscribe(handlers); + +channel.tracePromise(function(value) { + assert.deepStrictEqual(this, thisArg); + return Promise.reject(value); +}, input, thisArg, expectedError).then( + common.mustNotCall(), + common.mustCall((value) => { + assert.deepStrictEqual(value, expectedError); + }) +); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-promise.js b/test/parallel/test-diagnostics-channel-tracing-channel-promise.js new file mode 100644 index 00000000000..20892ca40f5 --- /dev/null +++ b/test/parallel/test-diagnostics-channel-tracing-channel-promise.js @@ -0,0 +1,41 @@ +'use strict'; + +const common = require('../common'); +const dc = require('diagnostics_channel'); +const assert = require('assert'); + +const channel = dc.tracingChannel('test'); + +const expectedResult = { foo: 'bar' }; +const input = { foo: 'bar' }; +const thisArg = { baz: 'buz' }; + +function check(found) { + assert.deepStrictEqual(found, input); +} + +function checkAsync(found) { + check(found); + assert.strictEqual(found.error, undefined); + assert.deepStrictEqual(found.result, expectedResult); +} + +const handlers = { + start: common.mustCall(check), + end: common.mustCall(check), + asyncStart: common.mustCall(checkAsync), + asyncEnd: common.mustCall(checkAsync), + error: common.mustNotCall() +}; + +channel.subscribe(handlers); + +channel.tracePromise(function(value) { + assert.deepStrictEqual(this, thisArg); + return Promise.resolve(value); +}, input, thisArg, expectedResult).then( + common.mustCall((value) => { + assert.deepStrictEqual(value, expectedResult); + }), + common.mustNotCall() +); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-sync-early-exit.js b/test/parallel/test-diagnostics-channel-tracing-channel-sync-early-exit.js new file mode 100644 index 00000000000..7568e6626d9 --- /dev/null +++ b/test/parallel/test-diagnostics-channel-tracing-channel-sync-early-exit.js @@ -0,0 +1,20 @@ +'use strict'; + +const common = require('../common'); +const dc = require('diagnostics_channel'); + +const channel = dc.tracingChannel('test'); + +const handlers = { + start: common.mustNotCall(), + end: common.mustNotCall(), + asyncStart: common.mustNotCall(), + asyncEnd: common.mustNotCall(), + error: common.mustNotCall() +}; + +// While subscribe occurs _before_ the sync call ends, +// no end event should be published. +channel.traceSync(() => { + channel.subscribe(handlers); +}, {}); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-run-stores.js b/test/parallel/test-diagnostics-channel-tracing-channel-sync-run-stores.js similarity index 100% rename from test/parallel/test-diagnostics-channel-tracing-channel-run-stores.js rename to test/parallel/test-diagnostics-channel-tracing-channel-sync-run-stores.js