-
-
Notifications
You must be signed in to change notification settings - Fork 35.1k
diagnostics_channel: add iterator support to tracing channel #61686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -281,6 +281,8 @@ function tracingChannelFrom(nameOrChannels, name) { | |
| } | ||
|
|
||
| class TracingChannel { | ||
| #nextChannel; | ||
|
|
||
| constructor(nameOrChannels) { | ||
| for (let i = 0; i < traceEvents.length; ++i) { | ||
| const eventName = traceEvents[i]; | ||
|
|
@@ -428,6 +430,84 @@ class TracingChannel { | |
| } | ||
| }); | ||
| } | ||
|
|
||
| traceIterator(fn, context = {}, thisArg, ...args) { | ||
| if (!this.hasSubscribers) { | ||
| return ReflectApply(fn, thisArg, args); | ||
| } | ||
|
|
||
| const { start, end, asyncStart, asyncEnd, error } = this; | ||
|
|
||
| const nextChannel = this.#nextChannel ||= tracingChannel({ | ||
| start: channel(start.name.slice(0, -6) + ':next:start'), | ||
| end: channel(end.name.slice(0, -4) + ':next:end'), | ||
| asyncStart: channel(asyncStart.name.slice(0, -11) + ':next:asyncStart'), | ||
| asyncEnd: channel(asyncEnd.name.slice(0, -9) + ':next:asyncEnd'), | ||
| error: channel(error.name.slice(0, -6) + ':next:error'), | ||
| }); | ||
|
|
||
| const wrapIter = (iter) => { | ||
| const { next: iterNext, return: iterReturn, throw: iterThrow } = iter; | ||
|
|
||
| iter.next = (...args) => | ||
| nextChannel.#traceMaybePromise(iterNext, context, iter, ...args); | ||
| iter.return = (...args) => | ||
| nextChannel.#traceMaybePromise(iterReturn, context, iter, ...args); | ||
| iter.throw = (...args) => | ||
| nextChannel.#traceMaybePromise(iterThrow, context, iter, ...args); | ||
|
|
||
| return iter; | ||
| }; | ||
|
|
||
| const result = this.#traceMaybePromise(fn, context, thisArg, ...args); | ||
|
|
||
| return result instanceof Promise ? | ||
| PromisePrototypeThen(result, wrapIter) : | ||
| wrapIter(result); | ||
| } | ||
|
|
||
| #traceMaybePromise(fn, context = {}, thisArg, ...args) { | ||
| if (!this.hasSubscribers) { | ||
| return ReflectApply(fn, thisArg, args); | ||
| } | ||
|
|
||
| const { start, end, asyncStart, asyncEnd, error } = this; | ||
|
|
||
| function reject(err) { | ||
| context.error = err; | ||
| error.publish(context); | ||
| asyncStart.publish(context); | ||
| // TODO: Is there a way to have asyncEnd _after_ the continuation? | ||
| asyncEnd.publish(context); | ||
| return PromiseReject(err); | ||
| } | ||
|
|
||
| function resolve(result) { | ||
| context.result = result; | ||
| asyncStart.publish(context); | ||
| // TODO: Is there a way to have asyncEnd _after_ the continuation? | ||
| asyncEnd.publish(context); | ||
| return result; | ||
| } | ||
|
|
||
| return start.runStores(context, () => { | ||
| try { | ||
| const result = ReflectApply(fn, thisArg, args); | ||
| // TODO: Should tracePromise just always do this? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because thenables. That's why it does the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about functions that can return both though? Sometimes the caller handles that, and changing the return type may be unexpected. In some cases we end up having to manually do a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was originally built to just be the minimum viable implementation to get moving with the pattern. It was never intended to be universal. Now that we've learned more about the weaknesses I think it's better to just make a bunch of specialization classes rather than trying to squeeze a complicated fractal of possibilities into a single implementation. That'd just kill the performance. My original thinking was we'd specialize with more methods, but it's turning out that we really need different channel sets for different situations too. |
||
| if (!(result instanceof Promise)) { | ||
| context.result = result; | ||
| return result; | ||
| } | ||
| return PromisePrototypeThen(result, resolve, reject); | ||
| } catch (err) { | ||
| context.error = err; | ||
| error.publish(context); | ||
| throw err; | ||
| } finally { | ||
| end.publish(context); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| function tracingChannel(nameOrChannels) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| 'use strict'; | ||
|
|
||
| const common = require('../common'); | ||
| const dc = require('diagnostics_channel'); | ||
| const assert = require('assert'); | ||
|
|
||
| const channel = dc.tracingChannel('test'); | ||
| const nextChannel = dc.tracingChannel('test:next'); | ||
|
|
||
| const expectedResult = { foo: 'bar' }; | ||
| const input = { foo: 'bar' }; | ||
| const thisArg = { baz: 'buz' }; | ||
|
|
||
| function check(found) { | ||
| assert.deepStrictEqual(found, input); | ||
| } | ||
|
|
||
| function checkNextAsync(found) { | ||
| check(found); | ||
| assert.strictEqual(found.error, undefined); | ||
| assert.deepStrictEqual(found.result, { value: expectedResult, done: false }); | ||
| } | ||
|
|
||
| // Async function* returns an AsyncGenerator synchronously, so no asyncStart/asyncEnd | ||
| // for the fn call itself | ||
| const handlers = { | ||
| start: common.mustCall(check), | ||
| end: common.mustCall(check), | ||
| asyncStart: common.mustNotCall(), | ||
| asyncEnd: common.mustNotCall(), | ||
| error: common.mustNotCall(), | ||
| }; | ||
|
|
||
| // next() on an AsyncGenerator returns a Promise | ||
| const nextHandlers = { | ||
| start: common.mustCall(check), | ||
| end: common.mustCall(check), | ||
| asyncStart: common.mustCall(checkNextAsync), | ||
| asyncEnd: common.mustCall(checkNextAsync), | ||
| error: common.mustNotCall(), | ||
| }; | ||
|
|
||
| channel.subscribe(handlers); | ||
| nextChannel.subscribe(nextHandlers); | ||
|
|
||
| const iter = channel.traceIterator(common.mustCall(async function*(value) { | ||
| assert.deepStrictEqual(this, thisArg); | ||
| yield value; | ||
| }), input, thisArg, expectedResult); | ||
|
|
||
| // next() returns a Promise since iter is an AsyncGenerator | ||
| iter.next().then(common.mustCall((result) => { | ||
| assert.deepStrictEqual(result, { value: expectedResult, done: false }); | ||
| })); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| 'use strict'; | ||
|
|
||
| const common = require('../common'); | ||
| const dc = require('diagnostics_channel'); | ||
|
|
||
| const channel = dc.tracingChannel('test'); | ||
| const nextChannel = dc.tracingChannel('test:next'); | ||
|
|
||
| const handlers = { | ||
| start: common.mustNotCall(), | ||
| end: common.mustNotCall(), | ||
| asyncStart: common.mustNotCall(), | ||
| asyncEnd: common.mustNotCall(), | ||
| error: common.mustNotCall(), | ||
| }; | ||
|
|
||
| const nextHandlers = { | ||
| start: common.mustNotCall(), | ||
| end: common.mustNotCall(), | ||
| asyncStart: common.mustNotCall(), | ||
| asyncEnd: common.mustNotCall(), | ||
| error: common.mustNotCall(), | ||
| }; | ||
|
|
||
| // Subscribe after traceIterator call - no events should fire for the iterator | ||
| // or for subsequent next() calls since the iterator was not wrapped | ||
| const iter = channel.traceIterator(function*() { | ||
| yield 1; | ||
| }, {}); | ||
|
|
||
| channel.subscribe(handlers); | ||
| nextChannel.subscribe(nextHandlers); | ||
|
|
||
| iter.next(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| 'use strict'; | ||
|
|
||
| const common = require('../common'); | ||
| const dc = require('diagnostics_channel'); | ||
| const assert = require('assert'); | ||
|
|
||
| const channel = dc.tracingChannel('test'); | ||
| const nextChannel = dc.tracingChannel('test:next'); | ||
|
|
||
| const expectedError = new Error('test'); | ||
| const input = { foo: 'bar' }; | ||
| const thisArg = { baz: 'buz' }; | ||
|
|
||
| function check(found) { | ||
| assert.deepStrictEqual(found, input); | ||
| } | ||
|
|
||
| function checkError(found) { | ||
| check(found); | ||
| assert.deepStrictEqual(found.error, expectedError); | ||
| } | ||
|
|
||
| // Two traceIterator calls: one for next() error, one for throw() error | ||
| const handlers = { | ||
| start: common.mustCall(check, 2), | ||
| end: common.mustCall(check, 2), | ||
| asyncStart: common.mustNotCall(), | ||
| asyncEnd: common.mustNotCall(), | ||
| error: common.mustNotCall(), | ||
| }; | ||
|
|
||
| // iter1: next() success + next() throws = start×2, end×2, error×1 | ||
| // iter2: throw() throws = start×1, end×1, error×1 | ||
| const nextHandlers = { | ||
| start: common.mustCall(check, 3), | ||
| end: common.mustCall(check, 3), | ||
| asyncStart: common.mustNotCall(), | ||
| asyncEnd: common.mustNotCall(), | ||
| error: common.mustCall(checkError, 2), | ||
| }; | ||
|
|
||
| channel.subscribe(handlers); | ||
| nextChannel.subscribe(nextHandlers); | ||
|
|
||
| // Test next(): generator throws after the first yield | ||
| const iter1 = channel.traceIterator(common.mustCall(function*() { | ||
| assert.deepStrictEqual(this, thisArg); | ||
| yield 1; | ||
| throw expectedError; | ||
| }), input, thisArg); | ||
|
|
||
| assert.deepStrictEqual(iter1.next(), { value: 1, done: false }); | ||
| assert.throws(() => iter1.next(), expectedError); | ||
|
|
||
| // Test throw(): propagates error through the iterator | ||
| const iter2 = channel.traceIterator(common.mustCall(function*() { | ||
| yield 1; | ||
| }), input, thisArg); | ||
|
|
||
| assert.throws(() => iter2.throw(expectedError), expectedError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of magically deriving these from the main channels. Could we perhaps implement this
traceIteratorfunctionality as an entirely other class which we could simply construct with two separateTracingChannelinstances for the overall execution and the per-yield execution?I've been thinking we might want to split up
TracingChannelinto specializations anyway, something like:SyncTracingChannel,CallbackTracingChannel,PromiseTracingChannel, and a newIteratorTracingChannel?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and I'm also not a fan, but it felt like the simplest approach.
Are you saying to do instead something like this?
I thought about that, but I wasn't sure since it would be the first time that
TracingChannelinstruments anything that is not a function (I mean it still patches the functions on the iterator, but that's more magic than usual). It also makes it impossible to reimplement differently later, although in practice I'm not sure that would happen. And it would also potentially be inconsistent across implementors. One could use:yield, another could use:next, etc, and thenTracingChannelwould lose the consistency it was built for, and subscribers could not be built generically.Worth noting: I'm adding the same functionality to Orchestrion-JS right now and it's unclear whether the
FunctionTypeshould beIteratoror not. If this is split, then I guess it would probably be a separateIteratorType?I prefer the one class because we often have to mix functionalities or use the underlying channels directly for example when functions accept a callback but can also return a promise or a sync value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I did not mean to run the sync trace and then wrap another thing around the iterator separately. I meant to have something like
new IteratorTracingChannel(execChannels, yieldChannels)which would take two sets of channels for the two separate parts of the execution but then would have a singletrace(...)method which would do all of that internally.As the interfaces are currently though, they don't really compose well. That's why I was thinking we make full separate classes for this. In a case like mixed promise/callback functions we could simply have another type which expresses that pattern.
The current
TracingChannelhas a bunch of issues I'd like to address before we consider it stable. The composability is one issue, but there's also the lack of wrap without execution so you can build the wrapper closures once rather than on every execution, probably using some context builder function to take the inputs and do something with them to produce the context object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions:
execChannelsandyieldChannelsbe themselves specialized? So do you have to decide upfront if the function is sync or async and if the returned iterator is sync or async? And if so, how do you deal with cases where they are mixed? Or would there be additional looser specialized types for these cases likeMaybePromiseTracingChannel?FunctionKindvalue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nameOrChannelsinput thatTracingChannelreceives currently. For differentiating between sync or async iterators I think we should have separate sync and async *IteratorTracingChannel types. For mixed cases--that wouldn't really be a thing, would it? It would not be a standard Iterator or AsyncIterator if it was not consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is you can have a sync function returning and async iterator, an async function returning an async iterator, a sync function returning a sync iterator and an async function returning a sync iterator. Technically you could even have callbacks involved in all of this. Even if this was specialized, it needs to be specialized on multiple fronts. That's why I thought the proposal was something like
IteratorTracingChannel(PromiseChannel, SyncChannel)because otherwise if the sub-channels are passed directly there is no way to know the combination unless they are all handled automatically or it ends up being something likeAsyncIteratorButSyncFunctionTracingChannel, or some different combinations of methods (like ifAsyncIteratorTracingChannelwould have atracePromiseandtraceSyncfunction).Unfortunately it tends to be a case in enough places that it's a nuisance even today before any iterator support. People do all sorts of weird stuff in the wild. I guess the argument could be made to just do the sub-channels manually at that point, but outside of APM vendors not many people are familiar enough to do it right.
It's out of the hot path in the sense that it's only created on the first call. So in that sense it's not any more overhead than creating the main channel itself in the first place. Yes it's done lazily so it happens in the hot path once, but that shouldn't be enough to have any sort of real life impact.
Happy to do that, just not sure what the API would be, I guess it will require a bit more thinking/discussing. I think I would actually call figuring that out a blocker though, because I don't want to add something that will instantly be deprecated.
Do you have an example of what this could look like? Right now there is only
FunctionKind: "Sync" | "Async" | "Callback". Would you change that completely or add to it? What would a new format look like?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's created once, but it's checked every single time.
For the iterator consistency, I meant within the iterator itself, not how it is returned. Between yields they're all going to be promises or not. For the weird cases we may want to do some nesting thing like you suggest, but for the common cases we should try to have single specializations which handle the full behaviour with a single thing.
One possible thing we could try is having a composable builder thing where you can separately specify how to instrument the call and how to instrument the return value, if relevant. Something like: