-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add telemetry for download, extract, and analyze. #2597
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
Changes from 9 commits
7deb17e
5334b59
3f04848
44c9396
9ea1c39
fa2376f
2f6cd5f
4fd9348
0dd4175
ec73eb4
c211f0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 |
---|---|---|
|
@@ -7,8 +7,13 @@ import * as path from 'path'; | |
import * as requestProgress from 'request-progress'; | ||
import { ProgressLocation, window } from 'vscode'; | ||
import { createDeferred } from '../../utils/async'; | ||
import { StopWatch } from '../../utils/stopWatch'; | ||
import { IFileSystem } from '../common/platform/types'; | ||
import { IExtensionContext, IOutputChannel } from '../common/types'; | ||
import { sendTelemetryEvent } from '../telemetry'; | ||
import { | ||
PYTHON_LANGUAGE_SERVER_DOWNLOADED | ||
} from '../telemetry/constants'; | ||
import { PlatformData, PlatformName } from './platformData'; | ||
import { IDownloadFileService } from './types'; | ||
|
||
|
@@ -28,6 +33,7 @@ export const DownloadLinks = { | |
}; | ||
|
||
export class LanguageServerDownloader { | ||
|
||
constructor( | ||
private readonly output: IOutputChannel, | ||
private readonly fs: IFileSystem, | ||
|
@@ -43,19 +49,35 @@ export class LanguageServerDownloader { | |
|
||
public async downloadLanguageServer(context: IExtensionContext): Promise<void> { | ||
const downloadUri = this.getDownloadUri(); | ||
|
||
const timer: StopWatch = new StopWatch(); | ||
let success: boolean = true; | ||
let localTempFilePath = ''; | ||
|
||
try { | ||
localTempFilePath = await this.downloadFile(downloadUri, 'Downloading Microsoft Python Language Server... '); | ||
// success telemetry for download | ||
success = true; | ||
} catch (err) { | ||
this.output.appendLine('failed.'); | ||
this.output.appendLine(err); | ||
success = false; | ||
throw new Error(err); | ||
} finally { | ||
sendTelemetryEvent(PYTHON_LANGUAGE_SERVER_DOWNLOADED, timer.elapsedTime, { success: success === true }); | ||
} | ||
|
||
timer.reset(); | ||
try { | ||
await this.unpackArchive(context.extensionPath, localTempFilePath); | ||
success = true; | ||
} catch (err) { | ||
this.output.appendLine('failed.'); | ||
this.output.appendLine(err); | ||
success = true; | ||
throw new Error(err); | ||
} finally { | ||
if (localTempFilePath.length > 0) { | ||
await this.fs.deleteFile(localTempFilePath); | ||
} | ||
sendTelemetryEvent('DEREK2', timer.elapsedTime, { success: success === true }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies - this is a WIP at the moment... |
||
await this.fs.deleteFile(localTempFilePath); | ||
} | ||
} | ||
|
||
|
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be a constant, not a member. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the fix for #2297 and related... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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); | ||
} | ||
} |
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.
Why did we go with this approach instead of the decorator?
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.
As you can see there's some duplication of code..
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.
Because the decorator emits telemetry, and adds timing info. It does not allow for state changes in property values without increasing complexity of the decorator function. I couldn't come up with an appropriate reason for the added complexity.