diagnostics_channel: early-exit tracing channel trace methods #51915
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Qard
added
doc
Qard
force-pushed
the
tracing-channel-early-return
branch
3 times, most recently
from
8edb24d to
657f046
Compare
anonrig
reviewed
Feb 28, 2024
Qard
force-pushed
the
tracing-channel-early-return
branch
3 times, most recently
from
38c8af3 to
91cb93b
Compare
anonrig
approved these changes
Feb 28, 2024
mcollina
reviewed
Feb 29, 2024
There was a problem hiding this comment.
There seem to be spurious changes to other files. Could you fix?
My 2 cents for this is that the data produced by a tracing channel that's not started at startup is useless because incomplete and it could lead to crashes/unexpected behaviors. A developer can always move the tracing setup at the beginning of the program and capture everything. I think this is a bugfix.
Qard
force-pushed
the
tracing-channel-early-return
branch
from
91cb93b to
244ee51
Compare
|
Apologies for the formatting irregularities. I've been trying out Zed and it apparently likes to auto-format buffers not just when opening them, but also when they appear in search results. 😐 |
Flarna
reviewed
Mar 1, 2024
|
|
||
| // While subscribe occurs _before_ the promise resolves, | ||
| // no async events should be published. | ||
| channel.tracePromise(() => { |
There was a problem hiding this comment.
Suggested change
| channel.tracePromise(() => { | |
| channel.tracePromise(common.mustCall(() => { |
You could even add asserts that this/arguments/return values are correctly forwarded.
Similar of the other APIs. But feel free to ignore and keep as is.
Flarna
approved these changes
Mar 1, 2024
|
I interpret this also as a bugfix. |
github-actions
bot
removed
the
request-ci
jsumners-nr
reviewed
Mar 5, 2024
legendecas
approved these changes
Mar 6, 2024
Qard
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 4f3cf4e |
rdw-msft
pushed a commit
to rdw-msft/node
that referenced
this pull request
Mar 26, 2024
PR-URL: nodejs#51915 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito
pushed a commit
that referenced
this pull request
May 2, 2024
PR-URL: #51915 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Merged
tlhunter
reviewed
May 7, 2024
| } | ||
| } | ||
|
|
||
| get hasSubscribers() { |
There was a problem hiding this comment.
It looks like there are no tests for this method. Though it does get code coverage since it's used by other methods. Was that intentional? Seems like an accident since it's a public method. Also it looks like there's no documentation?
This was referenced May 8, 2024
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 8, 2024
follow up work for nodejs#51915
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 8, 2024
follow up work for nodejs#51915
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 8, 2024
follow up work for nodejs#51915
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 8, 2024
follow up work for nodejs#51915
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 8, 2024
follow up work for nodejs#51915
tlhunter
added a commit
to DataDog/dc-polyfill
that referenced
this pull request
May 9, 2024
- some `diagnostics_channel` changes landed in Node.js in nodejs/node#51915 - this PR adds the `TracingChannel#hasSubscribers` helper method available in Node.js v22 and v20.13 - this PR _does not_ add the early exit functionality from that same commit - since this PR only implements half of the functionality in that commit we aren't fully v22 and v20.13 compatible - for that reason I haven't updated the README to state that we track said versions - I did add a note to the README about this partial jump in functionality - this PR also adds CI tests against Node.js v22.x - partially obsoletes #10 which is a WIP that attempted to implement both
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 9, 2024
follow up work for nodejs#51915
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 9, 2024
follow up work for nodejs#51915
tlhunter
added a commit
to tlhunter/node
that referenced
this pull request
May 9, 2024
follow up work for nodejs#51915
jcbhmr
pushed a commit
to jcbhmr/node
that referenced
this pull request
May 15, 2024
PR-URL: nodejs#51915 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This documents and enforces that tracing channel will not publish events unless subscribers are present at the start of the trace. This enables a more performant early exit and ensures graph completeness as prior to this change a subscription added midway through a trace could receive the later events despite missing the earlier ones.
I'm unsure if we should consider this a major change as there is a behaviour change in that a subscriber will no longer receive events for traces which began before the subscription was made. This could be considered a major change, or it could be considered a bug fix--we'll need to decide how we should frame this.
cc @nodejs/diagnostics