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.

Sign up for GitHub

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

Jump to bottom

doc: correct peformance entry types #54263

Merged
merged 1 commit into from
Aug 15, 2024
Merged

doc: correct peformance entry types #54263

merged 1 commit into from
Aug 15, 2024

Conversation

jazelly
Copy link
Contributor

@jazelly jazelly commented Aug 8, 2024 •
edited
Loading

Fixes: #54212
Fixes: #50290

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Aug 8, 2024
jakecastelli
jakecastelli approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@jakecastelli jakecastelli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, one nit: shall we reorder them alphabetically?

e.g. 'dns', 'function', 'gc', 'http', ...

@jazelly
Copy link
Contributor Author

jazelly commented Aug 9, 2024

@jakecastelli Sounds good 👍 reordered the entries

jakecastelli
jakecastelli approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@jakecastelli jakecastelli left a comment

Choose a reason for hiding this comment

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

LGTM

@jakecastelli jakecastelli added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 9, 2024
@Semigradsky
Copy link
Contributor

So 'node' is incorrect performance entry type?

@jazelly

This comment was marked as outdated.

Sign in to view
@jazelly
Copy link
Contributor Author

jazelly commented Aug 9, 2024

@Semigradsky Pretty sure the doc is outdated and node is no longer supported. It was firstly introduced here and got removed in this PR

@Semigradsky
Copy link
Contributor

Ok, I see that we have PerformanceNodeTiming which extends PerformanceEntry, and entryType for it can be 'node' - https://github.com/jasnell/node/blob/262d0d0482ffd2fe7d654422df9df1f350045635/test/sequential/test-perf-hooks.js#L57

Base class can't be more narrow, so entryType can be 'node' for PerformanceEntry

@jazelly
Copy link
Contributor Author

jazelly commented Aug 9, 2024

I overlooked that. I think you are right @Semigradsky after checking the implementation. Thank you for pointing it out. I have updated the PR

I think previously, this PR did not remove the 'node' type but hide it as an unsupported one, so that observer does not track it

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit e020dd8 into nodejs:main Aug 15, 2024
16 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e020dd8

RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
@jazelly @RafaelGSS
doc: correct peformance entry types
814cb80 
Fixes: #54212
Fixes: #50290
PR-URL: #54263
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
@jazelly @RafaelGSS
doc: correct peformance entry types
aa6e770 
Fixes: #54212
Fixes: #50290
PR-URL: #54263
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@aymen94 aymen94 aymen94 approved these changes

@trivikr trivikr trivikr approved these changes

@jakecastelli jakecastelli jakecastelli approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
Milestone
No milestone
9 participants
@jazelly @Semigradsky @nodejs-github-bot @jasnell @lpinca @aymen94 @trivikr @jakecastelli @joyeecheung