Skip to content

lib: add util.getCallSite() API - #54380

Merged
nodejs-github-bot merged 9 commits into
nodejs:mainfrom
RafaelGSS:add-getCallSite
Sep 4, 2024
Merged

lib: add util.getCallSite() API#54380
nodejs-github-bot merged 9 commits into
nodejs:mainfrom
RafaelGSS:add-getCallSite

Conversation

@RafaelGSS

Copy link
Copy Markdown
Member

This pull request introduces a new API method, util.getCallSite(), which provides a way to capture and inspect the call stack within a Node.js application. The method returns an array of CallSite objects, so developers can retrieve information about each function call in the stack trace.

Previously, they could reach the same behaviour by using Error.prepareStackTrace but, this API isn't that friendly IMO.

@RafaelGSSRafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 14, 2024
@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Aug 14, 2024
Comment threadlib/util.js Outdated
@codecov

codecovBot commented Aug 15, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (298dea0) to head (aa11f56).
Report is 250 commits behind head on main.

Files with missing linesPatch %Lines
src/node_util.cc94.59%0 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #54380 +/- ##
==========================================
- Coverage 87.61% 87.60% -0.01% 
==========================================
Files 650 650 Lines 182829 182878 +49 Branches 35382 35390 +8 ==========================================
+ Hits 160179 160210 +31 
Misses 15931 15931 - Partials 6719 6737 +18 
Files with missing linesCoverage Δ
lib/util.js95.91% <100.00%> (+0.12%)⬆️
src/node_util.cc85.77% <94.59%> (+1.61%)⬆️

... and 29 files with indirect coverage changes

@H4adH4ad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we add an option to ignore node:internal?

Also, I think we should pretty print the callsite object, if we console.log it:

[
CallSite {},
CallSite {},
CallSite {},
CallSite {},
CallSite {},
CallSite {},
CallSite {},
CallSite {}
]

If we JSON.stringify:

[{},{},{},{},{},{},{},{}]

And maybe we should add some snapshot tests, I know they could break if we change/add the methods that call them but usually they are great to track regressions, I know we have a couple snapshot tests like that in our codebase.

In general, looks great to me to have a utility function like this.

@RafaelGSS

RafaelGSS commented Aug 17, 2024

Copy link
Copy Markdown
MemberAuthor

I'd not include an option to ignore it in this initial version - maybe a follow up pr once it's landed?

It's not supposed to be printed via console.log because it's a CallSite representation. I will modify the API so it retrieves the stack trace from C++, and this should address this issue with the new object

@H4adH4ad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine landing it with this js version, just include a benchmark and then we can evaluate if the new c++ version will be faster.

Also, having some snapshot tests will be good to see what will change when we land the c++ version.

@RafaelGSSRafaelGSS added the blocked PRs that are blocked by other issues or PRs. label Aug 18, 2024
@RafaelGSS

Copy link
Copy Markdown
MemberAuthor

C++ version will be landed on this PR. I didn't have time yet.

@RafaelGSSRafaelGSS added wip Issues and PRs that are still a work in progress. and removed blocked PRs that are blocked by other issues or PRs. labels Aug 18, 2024
@mugli

Copy link
Copy Markdown

Is the new API going to work for events like unhandledRejection?

@RafaelGSSRafaelGSS removed the wip Issues and PRs that are still a work in progress. label Aug 20, 2024
@RafaelGSSRafaelGSS added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 20, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Did you do a benchmark?

Comment threadsrc/node_util.cc Outdated
Comment threadsrc/node_util.cc Outdated
@RafaelGSS

RafaelGSS commented Aug 20, 2024

Copy link
Copy Markdown
MemberAuthor

lgtm

Did you do a benchmark?

I did, but without having a fair comparison I don't know if it will mean much:

From my local machine

util/get-callsite.js n=1000000: 178,641.08162235853

I suspect the C++ implementation will be slower than the JS one for this particular scenario. We need to iterate over v8::StackTrace to create objects while the JS approach does not compute the values unless the function is called. e.g: .getFileName(). So the comparison will be unfair.

But, remember that we've switched to C++ for a side-effect-less approach. I can optimize this code later.

@RafaelGSSRafaelGSS added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Aug 20, 2024
@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@RafaelGSSRafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 80a989f into nodejs:mainSep 4, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 80a989f

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
PR-URL: #54380
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Sep 16, 2024
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
Notable changes:
lib:
* (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
* doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
* create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
* (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
* (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
* Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384
PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
Notable changes:
lib:
* (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
* doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
* create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
* (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
* (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
* Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384
PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
Notable changes:
lib:
* (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
* doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
* create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
* (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
* (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
* Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384
PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
Notable changes:
lib:
* (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
* doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
* create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
* (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
* (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
* Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384
PR-URL: #54966
Bratan613

This comment was marked as spam.

@BecStem

This comment was marked as off-topic.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.semver-minorPRs that contain new features and should be released in the next minor version.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@RafaelGSS@mugli@nodejs-github-bot@BecStem@mcollina@jasnell@anonrig@cjihrig@ovflowd@H4ad@mertcanaltin@Bratan613@targos