Use optional module "@microsoft/typescript-etw" for ETW logging #32612
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amcasey
approved these changes
Jul 30, 2019
weswigham
reviewed
Jul 30, 2019
weswigham
reviewed
Jul 30, 2019
src/compiler/moduleNameResolver.ts
Outdated
| } | ||
| finally { | ||
| if (etwLogger && result && result.resolvedModule) etwLogger.logInfoEvent(`Module "${moduleName}" resolved to "${result.resolvedModule.resolvedFileName}"`); |
There was a problem hiding this comment.
Likewise, maybe in our PerfLogger wrapper, we could have a logInfoEventIf<T extends any[]>(condition: boolean, logText: (...args: T) => string, ...args: T), so we can avoid introducing control flow statements for logging into code not related to the logging?
There was a problem hiding this comment.
I'm not sure I understand this suggestion.
There was a problem hiding this comment.
Rather than
if (etwLogger && result && result.resolvedModule) etwLogger.logInfoEvent(`Module "${moduleName}" resolved to "${result.resolvedModule.resolvedFileName}"`)write
logger.logInfoEventIf(result && result.resolvedModule, (moduleName, result) => `Module "${moduleName}" resolved to "${result.resolvedModule.resolvedFileName}"`, moduleName, result);like how our ts.Debug.assert signature is setup. (This way the control flow for the logger is within the logger and if the message isn't logged, the string isn't calculated)
There was a problem hiding this comment.
It moves the control flow for the conditional-ness of the logging into the logger, so anyone reading the implementation doesn't need to reason over the effects of the control flow that's only there for the logger. The function-instead-of-string bit is just an optimization to avoid doing expensive string building (like typeToString) if the branch isn't hit - it's not strictly needed, and is why ts.Debug.assert takes both a string and a function argument.
|
@weswigham I added a PerfLogger and NullLogger class which addresses your first concern and makes things cleaner. Let me know if it should be organized differently (e.g. is that the correct file and namespace?) I considered the second suggestion (moving conditional logic inside the logger class), but that is very unwieldy given the strong typing that is currently in place. We would need logInfoEventIf(), logStartCommandIf(), logStopCommandIf(), etc... It seems like overkill for this problem. |
|
We'll also need to add |
weswigham
reviewed
Aug 1, 2019
src/compiler/perfLogger.ts
Outdated
| etwModule = undefined; | ||
| } | ||
|
|
||
| export const perfLogger: PerfLogger = etwModule ? etwModule : new NullLogger(); |
There was a problem hiding this comment.
I considered the second suggestion (moving conditional logic inside the logger class), but that is very unwieldy given the strong typing that is currently in place. We would need logInfoEventIf(), logStartCommandIf(), logStopCommandIf(), etc... It seems like overkill for this problem.
Here's a version using a fluent API to avoid remaking conditional methods with similar definitions (you could construct the passthru instance reflectively, but eh):
Suggested change
| export const perfLogger: PerfLogger = etwModule ? etwModule : new NullLogger(); | |
| const nullLogger = new NullLogger(); | |
| class PassThruLogger implements PerfLogger { | |
| constructor(protected base: PerfLogger) {} | |
| logEvent(msg: string): void { | |
| return this.base.logEvent(msg); | |
| } | |
| logErrEvent(msg: string): void { | |
| return this.base.logErrEvent(msg); | |
| } | |
| logPerfEvent(msg: string): void { | |
| return this.base.logPerfEvent(msg); | |
| } | |
| logInfoEvent(msg: string): void { | |
| return this.base.logInfoEvent(msg); | |
| } | |
| logStartCommand(command: string, msg: string): void { | |
| return this.base.logStartCommand(command, msg); | |
| } | |
| logStopCommand(command: string, msg: string): void { | |
| return this.base.logStopCommand(command, msg); | |
| } | |
| logStartUpdateProgram(msg: string): void { | |
| return this.base.startUpdateProgram(msg); | |
| } | |
| logStopUpdateProgram(msg: string): void { | |
| return this.base.logStopUpdateProgram(msg); | |
| } | |
| logStartUpdateGraph(): void { | |
| return this.base.logStartUpdateGraph(); | |
| } | |
| logStopUpdateGraph(): void { | |
| return this.base.logStopUpdateGraph(); | |
| } | |
| logStartResolveModule(name: string): void { | |
| return this.base.logStartResolveModule(name); | |
| } | |
| logStopResolveModule(success: string): void { | |
| return this.base.logStopResolveModule(success); | |
| } | |
| logStartParseSourceFile(filename: string): void { | |
| return this.base.logStartParseSourceFile(filename); | |
| } | |
| logStopParseSourceFile(): void { | |
| return this.base.logStopParseSourceFile(); | |
| } | |
| logStartReadFile(filename: string): void { | |
| return this.base.logStartReadFile(filename); | |
| } | |
| logStopReadFile(): void { | |
| return this.base.logStopReadFile(); | |
| } | |
| logStartBindFile(filename: string): void { | |
| return this.base.logStartBindFile(filename); | |
| } | |
| logStopBindFile(): void { | |
| return this.base.logStopBindFile(); | |
| } | |
| logStartScheduledOperation(operationId: string): void { | |
| return this.base.logStartScheduledOperation(operationId); | |
| } | |
| logStopScheduledOperation(): void { | |
| return this.base.logStopScheduledOperation(); | |
| } | |
| } | |
| class ConditionalPerfLogger extends PassThruLogger { | |
| constructor(base: PerfLogger) { | |
| super(base); | |
| } | |
| if(condition: boolean): PerfLogger { | |
| if (condition) { | |
| return this.base; | |
| } | |
| return nullLogger; | |
| } | |
| } | |
| export const perfLogger: ConditionalPerfLogger = new ConditionalPerfLogger(etwModule ? etwModule : nullLogger); |
usage:
perfLogger.if(moduleResolution).logEvent("message");
There was a problem hiding this comment.
I tried this out but it's still missing the piece where the message is a function instead of a string. Without that the string interpolation could fail with a null reference.
At this point I'd like to move forward with the code that I have which is pretty straightforward and has addressed all of the other comments. What do you think @sheetalkamat ?
There was a problem hiding this comment.
You can redefine if to be
if(condition: boolean, cb: (logger: PerfLogger) => void): void {
if (condition) {
return cb(this.base);
}
}with usage
perfLogger.if(moduleResolution, log => log.logEvent("message"));
if that's the concern.
sheetalkamat
reviewed
Aug 1, 2019
|
@typescript-bot perf test this |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 5e19753. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@RyanCavanaugh Here they are:Comparison Report - master..32612
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sheetalkamat
requested changes
Aug 2, 2019
sheetalkamat
reviewed
Aug 5, 2019
sheetalkamat
approved these changes
Aug 6, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Use the @microsoft/typescript-etw native module for ETW logging.
@microsoft/typescript-etw (and ETW in general) are only supported on Windows, so it is not a good idea to add it to the "dependencies" in package.json. Even on Windows we still do not want this module installed by default. Consumers who are interested (such as Visual Studio) should have to opt in. Therefore ETW logging will only be supported if @microsoft/typescript-etw is installed manually. The code will work if the module is found, otherwise it will do nothing.
Types are added from @types/microsoft__typescript-etw so that we can have type checking even if the actual package is not installed.