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.
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
src: add trace events for env.cc #23674
src: add trace events for env.cc #23674
Changes from all commits
cf1f5f0
b32b136
065495f
c2df5c8
472f947
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Heads up … there’s a very good chance of the
args
/exec_args
handling being moved to another method as part of better embedding supportThere 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.
Yeah, I was wondering about that. Hmm... perhaps we should make args have their own dedicated trace event category... or even make it part of the process
__metadata
so that it's always there if tracing is enabled. It's useful context either way.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.
@jasnell I think that depends… We’ve had a similar conversation in the node-report PR at #22712 – what arguments do we actually care about here? The ones passed on the original command line, to the Node.js CLI, or the one that this Node.js instance explicitly uses?
The distinction matters for embedder use cases; But maybe, as a more practical example: I’d like to add support for something similar to
execArgv
to Workers. I guess it makes sense to emit these per-Environment?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'm thinking that the one that this Node.js instance explicitly uses... whichever are relevant to each specific
Environment
instance.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.
Does that also apply to
cwd
forchild_process.fork
(as an alternative if it does not)?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 shouldn't, but that's a question for @addaleax and is a separate concern from this PR.
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.
When using
cwd
,chdir()
is executed in the child process, so, yes, I think this test could be written withoutprocess.chdir()
and the need to skip it in Workers.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 chdir is there because of the generated trace event file. I'm inclined to leave the test as it is and refactor all of the trace event tests as a whole if this is the direction we want to go (they all follow this same pattern)