Skip to content
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

diagnostics_channel: early-exit tracing channel trace methods #51915

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

Qard
Copy link
Member

@Qard Qard commented Feb 28, 2024

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

@Qard Qard added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. diagnostics_channel Issues and PRs related to diagnostics channel labels Feb 28, 2024
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 28, 2024
@Qard Qard force-pushed the tracing-channel-early-return branch 3 times, most recently from 8edb24d to 657f046 Compare February 28, 2024 22:13
lib/diagnostics_channel.js Outdated Show resolved Hide resolved
lib/diagnostics_channel.js Outdated Show resolved Hide resolved
lib/diagnostics_channel.js Show resolved Hide resolved
lib/diagnostics_channel.js Outdated Show resolved Hide resolved
@Qard Qard force-pushed the tracing-channel-early-return branch 3 times, most recently from 38c8af3 to 91cb93b Compare February 28, 2024 23:04
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Qard force-pushed the tracing-channel-early-return branch from 91cb93b to 244ee51 Compare February 29, 2024 19:31
@Qard
Copy link
Member Author

Qard commented Feb 29, 2024

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. 😐


// While subscribe occurs _before_ the promise resolves,
// no async events should be published.
channel.tracePromise(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Flarna commented Mar 1, 2024

I interpret this also as a bugfix.
TracingChannel is anyway experimental therefore it's not that important anyway.

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57535/

Copy link

@jsumners-nr jsumners-nr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarification of intent for the API is good for me.

@Qard Qard added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 7, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 7, 2024
@nodejs-github-bot nodejs-github-bot merged commit 4f3cf4e into nodejs:main Mar 7, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4f3cf4e

@Qard Qard deleted the tracing-channel-early-return branch March 7, 2024 17:50
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>
}
}

get hasSubscribers() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

tlhunter added a commit to tlhunter/node that referenced this pull request May 8, 2024
tlhunter added a commit to tlhunter/node that referenced this pull request May 8, 2024
tlhunter added a commit to tlhunter/node that referenced this pull request May 8, 2024
tlhunter added a commit to tlhunter/node that referenced this pull request May 8, 2024
tlhunter added a commit to tlhunter/node that referenced this pull request May 8, 2024
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
tlhunter added a commit to tlhunter/node that referenced this pull request May 9, 2024
tlhunter added a commit to tlhunter/node that referenced this pull request May 9, 2024
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>
nodejs-github-bot pushed a commit that referenced this pull request Jul 24, 2024
follow up work for #51915

PR-URL: #52908
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 28, 2024
follow up work for #51915

PR-URL: #52908
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024
follow up work for #51915

PR-URL: #52908
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
follow up work for #51915

PR-URL: #52908
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
follow up work for #51915

PR-URL: #52908
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
follow up work for #51915

PR-URL: #52908
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. diagnostics_channel Issues and PRs related to diagnostics channel doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants