Add telemetry for download, extract, and analyze. #2597
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add telemetry to download, extract, and analyze, phases of the Python Language Server |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Stop duplicate initializations of Python Language Server progress reporter. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,18 @@ | |
| import { Progress, ProgressLocation, window } from 'vscode'; | ||
| import { Disposable, LanguageClient } from 'vscode-languageclient'; | ||
| import { createDeferred, Deferred } from '../../utils/async'; | ||
| import { StopWatch } from '../../utils/stopWatch'; | ||
| import { sendTelemetryEvent } from '../telemetry'; | ||
| import { PYTHON_LANGUAGE_SERVER_ANALYSISTIME } from '../telemetry/constants'; | ||
| import { LanguageServerTelemetry } from '../telemetry/types'; | ||
|
|
||
| export class ProgressReporting { | ||
| private statusBarMessage: Disposable | undefined; | ||
| private progress: Progress<{ message?: string; increment?: number }> | undefined; | ||
| private progressDeferred: Deferred<void> | undefined; | ||
| private progressTimer: StopWatch | undefined; | ||
| private progressTimeout: NodeJS.Timer | undefined; | ||
| private ANALYSIS_TIMEOUT_MS: number = 60000; | ||
|
There was a problem hiding this comment. This needs to be a constant, not a member. There was a problem hiding this comment. Agreed. I will make the change. Thanks for calling this out! |
||
|
|
||
| constructor(private readonly languageClient: LanguageClient) { | ||
| this.languageClient.onNotification('python/setStatusBarMessage', (m: string) => { | ||
|
|
@@ -19,7 +26,14 @@ export class ProgressReporting { | |
| }); | ||
|
|
||
| this.languageClient.onNotification('python/beginProgress', async _ => { | ||
| if (this.progressDeferred) { // if we restarted, no worries as reporting will still funnel to the same place. | ||
|
There was a problem hiding this comment. This is the fix for #2297 and related... There was a problem hiding this comment. For #2297 (while most of the problem is not here, restart is rare) you need to track 'stateChange' on the language client. When it gets to 'stopped' LS has terminated. There was a problem hiding this comment. Cool! Thanks for the tip. I'll move this code to another PR that we can review separately and cover all the appropriate cases. There was a problem hiding this comment. @MikhailArkhipov please see #2606 |
||
| return; | ||
| } | ||
|
|
||
| this.progressDeferred = createDeferred<void>(); | ||
| this.progressTimer = new StopWatch(); | ||
| this.progressTimeout = setTimeout(this.handleTimeout, this.ANALYSIS_TIMEOUT_MS); | ||
|
|
||
|
There was a problem hiding this comment. Change to There was a problem hiding this comment. Cool - that callback scope still isn't always clear to me, thanks for catching this. |
||
| window.withProgress({ | ||
| location: ProgressLocation.Window, | ||
| title: '' | ||
|
|
@@ -40,7 +54,32 @@ export class ProgressReporting { | |
| if (this.progressDeferred) { | ||
| this.progressDeferred.resolve(); | ||
| this.progressDeferred = undefined; | ||
| this.completeAnalysisTracking(true); | ||
| this.progress = undefined; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private completeAnalysisTracking(isSuccess: boolean): void { | ||
| if (this.progressTimer) { | ||
| const lsAnalysisTelemetry: LanguageServerTelemetry = { | ||
| success: isSuccess | ||
| }; | ||
| sendTelemetryEvent( | ||
| PYTHON_LANGUAGE_SERVER_ANALYSISTIME, | ||
| this.progressTimer.elapsedTime, | ||
| lsAnalysisTelemetry | ||
| ); | ||
| this.progressTimer = undefined; | ||
| } | ||
|
|
||
| if (this.progressTimeout) { | ||
| this.progressTimeout = undefined; | ||
| } | ||
| } | ||
|
|
||
| // tslint:disable-next-line:no-any | ||
| private handleTimeout(_args: any[]): void { | ||
| this.completeAnalysisTracking(false); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,15 @@ export function sendTelemetryEvent(eventName: string, durationMs?: number, prope | |
| } | ||
|
|
||
| // tslint:disable-next-line:no-any function-name | ||
| export function captureTelemetry( | ||
| eventName: string, | ||
| properties?: TelemetryProperties, | ||
| captureDuration: boolean = true, | ||
| // tslint:disable-next-line:no-any | ||
| beforeSuccessEmit?: (props?: any) => void, | ||
|
There was a problem hiding this comment. We don't need these two new parameters. Just remove the last two parameters. |
||
| // tslint:disable-next-line:no-any | ||
| beforeFailEmit?: (props?: any) => void | ||
| ) { | ||
| // tslint:disable-next-line:no-function-expression no-any | ||
| return function (target: Object, propertyKey: string, descriptor: TypedPropertyDescriptor<any>) { | ||
| const originalMethod = descriptor.value; | ||
|
|
@@ -48,11 +56,17 @@ export function captureTelemetry(eventName: string, properties?: TelemetryProper | |
| // tslint:disable-next-line:prefer-type-cast | ||
| (result as Promise<void>) | ||
| .then(data => { | ||
| if (beforeSuccessEmit) { | ||
|
There was a problem hiding this comment. Please remove this if condition and this new code. |
||
| beforeSuccessEmit(properties); | ||
| } | ||
| sendTelemetryEvent(eventName, stopWatch.elapsedTime, properties); | ||
| return data; | ||
| }) | ||
| // tslint:disable-next-line:promise-function-async | ||
| .catch(ex => { | ||
| if (beforeFailEmit) { | ||
|
There was a problem hiding this comment. Please remove this if condition and this code. Change as follows: properties = properties ? properties : {};
properties.success = false;There was a problem hiding this comment. I cannot quite understand this exactly. What happens when For instance, what happens when this comes from the decorator on That use of properties = properties ? properties : {};
if ((<LanguageServerTelemetry>properties).success) {
(<LanguageServerTelemetry>properties).success = false;
}Reference: Type Guards and Differentiating Types There was a problem hiding this comment. Indeed, I will have to do the type-check/type-cast it would seem:
There was a problem hiding this comment. Just change as follows: properties = (properties ? properties : {}) as any;
if (properties.success !== undefined) {
properties.success = false;
}Check for undefined There was a problem hiding this comment. Yeah, I don't think we want to do this. Assuming that I want to go back to doing something more explicit in the downloader/languageServer themselves instead. |
||
| beforeFailEmit(properties); | ||
| } | ||
| sendTelemetryEvent(eventName, stopWatch.elapsedTime, properties); | ||
| return Promise.reject(ex); | ||
| }); | ||
|
|
||
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.
Please remove