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.
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 think this is ok in a pinch, but it actually feels a bit weird to me that this module has observable side effects upon being loaded. @mrcrane, I’m not too familiar with your ETW work, but could this log statement be wrapped in an exported function instead of evaluated immediately? Or at least be predicated upon the existence of
etwModule
?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.
Logging is predicated on the existence of
etwModule
through the use ofnullLogger
on line 40.However there seems to be a problem where we are still evaluating the parameter to the logging function even though
nullLogger
will ignore it. I'm not sure of the best way to solve that problem in a concise way for all of theperfLogger.log
statements that are littered throughout the code base. Is that the problem you are trying to solve?This particular log statement does not really need to be called immediately from within this module. Instead of wrapping it in an exported function we could just simply move the log statement somewhere else if that would be more appropriate. But it would still need a fix like the one in this PR. Alternatively we could come up with a way to guard the parameter from being evaluated, which we would want to apply to all log statements throughout the code base.
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 real problem is that
process
can’t be referenced without checking it, because some parts of TypeScript need to be able to run in the browser. But the fact that this runs something (even though in most contexts it’s a noop) on load is a little unusual. I would expect a startup message to go, perhaps,executeCommandLine
in tsc.ts.As-is, you could get some strange effects by using TypeScript’s Node API. Imagine you’re working in a Node project in which you have
@microsoft/typescript-etw
installed, and you do something likeIn this program, the log happens immediately after the VM parses this source file, even though you haven’t started doing anything with TypeScript yet. Your program may never do something with TypeScript, and yet the logger will say “Starting TypeScript... with command line” and then spit out the command line arguments that were given to your program, not even to TypeScript. So yeah, it’s easy to work around the
process
problem here, but it drew my attention to the fact that this might not be the right place for that log statement in the place.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.
Ok I see what you're getting at now and I agree. I wasn't initially aware of those other ways to load TypeScript.
Actually our primary use case for this perf logging is around tsserver so we would like to capture tsserver args, but having them available for tsc could be useful as well. In any case this can be resolved by removing this
perfLogger.log
statement from this file and essentially pasting it into those other entrypoints where we want to capture the args. It does not require any local variables so it should be fully moveable. That would certainly be a more sane solution.