Add instrumentation support to the typescript code #12991
Conversation
…rosoft/vscode-cpptools into dev/garretts/instrumentation
| import { CppBuildTask, CppBuildTaskDefinition, cppBuildTaskProvider } from '../LanguageServer/cppBuildTaskProvider'; | ||
| import { configPrefix } from '../LanguageServer/extension'; | ||
| import { CppSettings, OtherSettings } from '../LanguageServer/settings'; | ||
| import { getOutputChannel, getOutputChannelLogger, Logger, showOutputChannel } from '../logger'; |
There was a problem hiding this comment.
It's unclear to me why this is changing. It doesn't seem related to the purpose of this PR. Can you revert this file?
| @@ -7,7 +7,7 @@ | |||
| import * as vscode from 'vscode'; | |||
| import { localize } from 'vscode-nls'; | |||
| import * as util from '../common'; | |||
| import * as logger from '../logger'; | |||
| import { getOutputChannelLogger } from '../logger'; | |||
There was a problem hiding this comment.
It's unclear to me why this is changing. It doesn't seem related to the purpose of this PR. Can you revert this file?
| @@ -8,7 +8,7 @@ import * as vscode from 'vscode'; | |||
| import { Position, TextDocumentIdentifier } from 'vscode-languageclient'; | |||
| import * as nls from 'vscode-nls'; | |||
| import * as util from '../common'; | |||
| import * as logger from '../logger'; | |||
| import { getOutputChannel } from '../logger'; | |||
There was a problem hiding this comment.
It's unclear to me why this is changing. It doesn't seem related to the purpose of this PR. Can you revert this file?
|
|
||
| public append(level: number, message: string): void; | ||
| public append(message: string): void; | ||
| public append(levelOrMessage: string | number, message?: string): void { |
There was a problem hiding this comment.
I understand why you did this, but I'm not a fan of this pattern (the optional parameter is the first arg). Since we don't want to put the loggingLevel second, I'd prefer we not change the logging API until we're ready to change it everywhere in the code.
Suggestion: Create a new API taking the level and the message and deprecate the old one. Then we can migrate the rest of the extension to it in a separate PR.
| message = message || levelOrMessage as string; | ||
| const hasLevel = typeof levelOrMessage === 'number'; | ||
|
|
||
| if (!hasLevel || getLoggingLevel() >= levelOrMessage) { |
There was a problem hiding this comment.
This being a semi-high-frequency function, perhaps the current logging level should be stored on the Logger as a field instead of having to call a function every time.
| public append(level: number, message: string): void; | ||
| public append(message: string): void; | ||
| public append(levelOrMessage: string | number, message?: string): void { | ||
| message = message || levelOrMessage as string; |
There was a problem hiding this comment.
Is it possible for levelOrMessage to be a number when message is not passed in? Better to test the inputs with the isString isNumber helpers than to assert with as. (though my preference is to remove the overloads and create a new API that can forward here for logging to the output channel when the checks pass)
| import * as plist from 'plist'; | ||
| import * as nls from 'vscode-nls'; | ||
| import { LinuxDistribution } from './linuxDistribution'; | ||
| import { getOutputChannelLogger, showOutputChannel } from './logger'; |
There was a problem hiding this comment.
It's unclear to me why this is changing. It doesn't seem related to the purpose of this PR. Can you revert this file?
| } | ||
|
|
||
| // If the instrumentation object was loaded then we can set the services from the global object | ||
| if ((global as any).instrumentation) { |
There was a problem hiding this comment.
Use isInstrumentationEnabled here or connect it to the last block with an else?
| } | ||
|
|
||
| export function getLoggingLevel() { | ||
| return getNumericLoggingLevel(vscode.workspace.getConfiguration("C_Cpp", null).get<string>("loggingLevel")); |
There was a problem hiding this comment.
Since we can't pull CppSettings into this module, maybe we need to split out the validation code because we shouldn't be reading raw settings in the extension anymore. Apparently this file was skipped due to it not using CppSettings.
This isn't necessarily your problem. I'm just thinking out loud.
This adds the instrumentation support to enable performance testing and monitoring.
It:
PERFECTO_LAUNCH_SETTINGSenvironment variable is not set, everything here is a no-op