From 7deb17e3d69124485bfce9eb82a4e5584cc54cbb Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 16:54:45 -0700 Subject: [PATCH 01/11] Remove all .venv*/ folders in .gitignore --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index c6cd087c91ce..211427a32f66 100644 --- a/.gitignore +++ b/.gitignore @@ -12,7 +12,7 @@ npm-debug.log !yarn.lock coverage/ .vscode-test/** -.venv +.venv*/ pythonFiles/experimental/ptvsd/** debug_coverage*/** languageServer/** From 5334b598c66e1b221ee7a7b24209a12723ccbc14 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 19:37:23 -0700 Subject: [PATCH 02/11] Add telemetry for download, extract, and analyze. Fixes #2461 (alternate fix to PR #2593) - Capture telemetry surrounds methods, minimizing change - Telemetry type can be altered with less code later. - Add success/fail props modifyer func to `captureTelemetry` --- src/client/activation/downloader.ts | 23 +++++++++++++++++++++++ src/client/activation/languageServer.ts | 15 ++++++++++++++- src/client/telemetry/constants.ts | 1 + src/client/telemetry/index.ts | 16 +++++++++++++++- src/client/telemetry/types.ts | 17 ++++++++++++++++- 5 files changed, 69 insertions(+), 3 deletions(-) diff --git a/src/client/activation/downloader.ts b/src/client/activation/downloader.ts index c12634722d8e..3e266003c6e9 100644 --- a/src/client/activation/downloader.ts +++ b/src/client/activation/downloader.ts @@ -9,6 +9,12 @@ import { ProgressLocation, window } from 'vscode'; import { createDeferred } from '../../utils/async'; import { IFileSystem } from '../common/platform/types'; import { IExtensionContext, IOutputChannel } from '../common/types'; +import { captureTelemetry } from '../telemetry'; +import { + PYTHON_LANGUAGE_SERVER_DOWNLOADED, + PYTHON_LANGUAGE_SERVER_EXTRACTED +} from '../telemetry/constants'; +import { LanguageServerInstallOpTelemetry } from '../telemetry/types'; import { PlatformData, PlatformName } from './platformData'; import { IDownloadFileService } from './types'; @@ -28,6 +34,11 @@ export const DownloadLinks = { }; export class LanguageServerDownloader { + //tslint:disable-next-line:no-unused-variable + private lsDownloadTelemetry: LanguageServerInstallOpTelemetry = {}; + //tslint:disable-next-line:no-unused-variable + private lsExtractTelemetry: LanguageServerInstallOpTelemetry = {}; + constructor( private readonly output: IOutputChannel, private readonly fs: IFileSystem, @@ -59,6 +70,12 @@ export class LanguageServerDownloader { } } + @captureTelemetry( + PYTHON_LANGUAGE_SERVER_DOWNLOADED, + {}, + true, + (props?: LanguageServerInstallOpTelemetry) => props ? props.success = true : undefined + ) private async downloadFile(uri: string, title: string): Promise { this.output.append(`Downloading ${uri}... `); const tempFile = await this.fs.createTemporaryFile(downloadFileExtension); @@ -101,6 +118,12 @@ export class LanguageServerDownloader { return tempFile.filePath; } + @captureTelemetry( + PYTHON_LANGUAGE_SERVER_EXTRACTED, + this.languageServerStartupTelemetry, + true, + (props?: LanguageServerInstallOpTelemetry) => props ? props.success = true : undefined + ) private async unpackArchive(extensionPath: string, tempFilePath: string): Promise { this.output.append('Unpacking archive... '); diff --git a/src/client/activation/languageServer.ts b/src/client/activation/languageServer.ts index 51c5d62d6065..c5dcd440a9ae 100644 --- a/src/client/activation/languageServer.ts +++ b/src/client/activation/languageServer.ts @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +'use strict'; + import { inject, injectable } from 'inversify'; import * as path from 'path'; import { @@ -28,12 +30,15 @@ import { import { IEnvironmentVariablesProvider } from '../common/variables/types'; import { IServiceContainer } from '../ioc/types'; import { LanguageServerSymbolProvider } from '../providers/symbolProvider'; +import { captureTelemetry } from '../telemetry'; import { PYTHON_LANGUAGE_SERVER_DOWNLOADED, PYTHON_LANGUAGE_SERVER_ENABLED, - PYTHON_LANGUAGE_SERVER_ERROR + PYTHON_LANGUAGE_SERVER_ERROR, + PYTHON_LANGUAGE_SERVER_STARTUP } from '../telemetry/constants'; import { getTelemetryReporter } from '../telemetry/telemetry'; +import { LanguageServerInstallOpTelemetry } from '../telemetry/types'; import { IUnitTestManagementService } from '../unittests/types'; import { LanguageServerDownloader } from './downloader'; import { InterpreterData, InterpreterDataService } from './interpreterDataService'; @@ -76,6 +81,8 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { private surveyBanner: IPythonExtensionBanner; // tslint:disable-next-line:no-unused-variable private progressReporting: ProgressReporting | undefined; + //tslint:disable-next-line:no-unused-variable + private languageServerStartupTelemetry: LanguageServerInstallOpTelemetry = {}; constructor(@inject(IServiceContainer) private readonly services: IServiceContainer) { this.context = this.services.get(IExtensionContext); @@ -141,6 +148,12 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { (this.configuration.getSettings() as PythonSettings).removeListener('change', this.onSettingsChanged.bind(this)); } + @captureTelemetry( + PYTHON_LANGUAGE_SERVER_STARTUP, + this.languageServerStartupTelemetry, + true, + (props?: LanguageServerInstallOpTelemetry) => props ? props.success = true : undefined + ) private async startLanguageServer(clientOptions: LanguageClientOptions): Promise { // Determine if we are running MSIL/Universal via dotnet or self-contained app. diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index 78bbb138d89d..d35e815dc204 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -33,6 +33,7 @@ export const UNITTEST_RUN = 'UNITTEST.RUN'; export const UNITTEST_DISCOVER = 'UNITTEST.DISCOVER'; export const UNITTEST_VIEW_OUTPUT = 'UNITTEST.VIEW_OUTPUT'; export const PYTHON_LANGUAGE_SERVER_ENABLED = 'PYTHON_LANGUAGE_SERVER.ENABLED'; +export const PYTHON_LANGUAGE_SERVER_EXTRACTED = 'PYTHON_LANGUAGE_SERVER.EXTRACTED'; export const PYTHON_LANGUAGE_SERVER_DOWNLOADED = 'PYTHON_LANGUAGE_SERVER.DOWNLOADED'; export const PYTHON_LANGUAGE_SERVER_ERROR = 'PYTHON_LANGUAGE_SERVER.ERROR'; export const PYTHON_LANGUAGE_SERVER_STARTUP = 'PYTHON_LANGUAGE_SERVER.STARTUP'; diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index dcce014c9c8c..12318a0ecf58 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -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) { +export function captureTelemetry( + eventName: string, + properties?: TelemetryProperties, + captureDuration: boolean = true, + // tslint:disable-next-line:no-any + beforeSuccessEmit?: (props?: any) => void, + // 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) { 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) .then(data => { + if (beforeSuccessEmit) { + beforeSuccessEmit(properties); + } sendTelemetryEvent(eventName, stopWatch.elapsedTime, properties); return data; }) // tslint:disable-next-line:promise-function-async .catch(ex => { + if (beforeFailEmit) { + beforeFailEmit(properties); + } sendTelemetryEvent(eventName, stopWatch.elapsedTime, properties); return Promise.reject(ex); }); diff --git a/src/client/telemetry/types.ts b/src/client/telemetry/types.ts index 7865a5ea4c94..239ce2c74640 100644 --- a/src/client/telemetry/types.ts +++ b/src/client/telemetry/types.ts @@ -15,6 +15,10 @@ export type FormatTelemetry = { formatSelection: boolean; }; +export type LanguageServerInstallOpTelemetry = { + success?: boolean; +}; + export type LinterTrigger = 'auto' | 'save'; export type LintingTelemetry = { @@ -81,4 +85,15 @@ export type TerminalTelemetry = { pythonVersion?: string; interpreterType?: InterpreterType; }; -export type TelemetryProperties = FormatTelemetry | LintingTelemetry | EditorLoadTelemetry | PythonInterpreterTelemetry | CodeExecutionTelemetry | TestRunTelemetry | TestDiscoverytTelemetry | FeedbackTelemetry | TerminalTelemetry | DebuggerTelemetryV2 | SettingsTelemetry; +export type TelemetryProperties = FormatTelemetry + | LanguageServerInstallOpTelemetry + | LintingTelemetry + | EditorLoadTelemetry + | PythonInterpreterTelemetry + | CodeExecutionTelemetry + | TestRunTelemetry + | TestDiscoverytTelemetry + | FeedbackTelemetry + | TerminalTelemetry + | DebuggerTelemetryV2 + | SettingsTelemetry; From 3f04848e22638a6e87e60d82c7e6498805f0c27e Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 22:11:47 -0700 Subject: [PATCH 03/11] Simplify telemetry properties. - LanguageServerTelemetry is simpler and succinct enough for this use --- src/client/activation/downloader.ts | 10 +++++----- src/client/activation/languageServer.ts | 6 +++--- src/client/telemetry/types.ts | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/client/activation/downloader.ts b/src/client/activation/downloader.ts index 3e266003c6e9..565efc0de90a 100644 --- a/src/client/activation/downloader.ts +++ b/src/client/activation/downloader.ts @@ -14,7 +14,7 @@ import { PYTHON_LANGUAGE_SERVER_DOWNLOADED, PYTHON_LANGUAGE_SERVER_EXTRACTED } from '../telemetry/constants'; -import { LanguageServerInstallOpTelemetry } from '../telemetry/types'; +import { LanguageServerTelemetry } from '../telemetry/types'; import { PlatformData, PlatformName } from './platformData'; import { IDownloadFileService } from './types'; @@ -35,9 +35,9 @@ export const DownloadLinks = { export class LanguageServerDownloader { //tslint:disable-next-line:no-unused-variable - private lsDownloadTelemetry: LanguageServerInstallOpTelemetry = {}; + private lsDownloadTelemetry: LanguageServerTelemetry = {}; //tslint:disable-next-line:no-unused-variable - private lsExtractTelemetry: LanguageServerInstallOpTelemetry = {}; + private lsExtractTelemetry: LanguageServerTelemetry = {}; constructor( private readonly output: IOutputChannel, @@ -74,7 +74,7 @@ export class LanguageServerDownloader { PYTHON_LANGUAGE_SERVER_DOWNLOADED, {}, true, - (props?: LanguageServerInstallOpTelemetry) => props ? props.success = true : undefined + (props?: LanguageServerTelemetry) => props ? props.success = true : undefined ) private async downloadFile(uri: string, title: string): Promise { this.output.append(`Downloading ${uri}... `); @@ -122,7 +122,7 @@ export class LanguageServerDownloader { PYTHON_LANGUAGE_SERVER_EXTRACTED, this.languageServerStartupTelemetry, true, - (props?: LanguageServerInstallOpTelemetry) => props ? props.success = true : undefined + (props?: LanguageServerTelemetry) => props ? props.success = true : undefined ) private async unpackArchive(extensionPath: string, tempFilePath: string): Promise { this.output.append('Unpacking archive... '); diff --git a/src/client/activation/languageServer.ts b/src/client/activation/languageServer.ts index c5dcd440a9ae..ddf8c74e5322 100644 --- a/src/client/activation/languageServer.ts +++ b/src/client/activation/languageServer.ts @@ -38,7 +38,7 @@ import { PYTHON_LANGUAGE_SERVER_STARTUP } from '../telemetry/constants'; import { getTelemetryReporter } from '../telemetry/telemetry'; -import { LanguageServerInstallOpTelemetry } from '../telemetry/types'; +import { LanguageServerTelemetry } from '../telemetry/types'; import { IUnitTestManagementService } from '../unittests/types'; import { LanguageServerDownloader } from './downloader'; import { InterpreterData, InterpreterDataService } from './interpreterDataService'; @@ -82,7 +82,7 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { // tslint:disable-next-line:no-unused-variable private progressReporting: ProgressReporting | undefined; //tslint:disable-next-line:no-unused-variable - private languageServerStartupTelemetry: LanguageServerInstallOpTelemetry = {}; + private languageServerStartupTelemetry: LanguageServerTelemetry = {}; constructor(@inject(IServiceContainer) private readonly services: IServiceContainer) { this.context = this.services.get(IExtensionContext); @@ -152,7 +152,7 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { PYTHON_LANGUAGE_SERVER_STARTUP, this.languageServerStartupTelemetry, true, - (props?: LanguageServerInstallOpTelemetry) => props ? props.success = true : undefined + (props?: LanguageServerTelemetry) => props ? props.success = true : undefined ) private async startLanguageServer(clientOptions: LanguageClientOptions): Promise { // Determine if we are running MSIL/Universal via dotnet or self-contained app. diff --git a/src/client/telemetry/types.ts b/src/client/telemetry/types.ts index 239ce2c74640..2152a6d268b8 100644 --- a/src/client/telemetry/types.ts +++ b/src/client/telemetry/types.ts @@ -15,7 +15,7 @@ export type FormatTelemetry = { formatSelection: boolean; }; -export type LanguageServerInstallOpTelemetry = { +export type LanguageServerTelemetry = { success?: boolean; }; @@ -86,7 +86,7 @@ export type TerminalTelemetry = { interpreterType?: InterpreterType; }; export type TelemetryProperties = FormatTelemetry - | LanguageServerInstallOpTelemetry + | LanguageServerTelemetry | LintingTelemetry | EditorLoadTelemetry | PythonInterpreterTelemetry From 44c9396f42e83412781ad4687438c2161b087b40 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 22:14:57 -0700 Subject: [PATCH 04/11] Add telemetry collection for analysis reporting complete. - I believe this might fix #2297 (and related issues) - Calling 'beginProgress' from LS causes progress reporting to not get cleaned up properly. - Related fix for Microsoft/python-language-server#94 --- src/client/activation/progress.ts | 39 +++++++++++++++++++++++++++++++ src/client/telemetry/constants.ts | 2 ++ 2 files changed, 41 insertions(+) diff --git a/src/client/activation/progress.ts b/src/client/activation/progress.ts index a955cda92644..7026f3c54fb0 100644 --- a/src/client/activation/progress.ts +++ b/src/client/activation/progress.ts @@ -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 | undefined; + private progressTimer: StopWatch | undefined; + private progressTimeout: NodeJS.Timer | undefined; + private ANALYSIS_TIMEOUT_MS: number = 60000; 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. + return; + } + this.progressDeferred = createDeferred(); + this.progressTimer = new StopWatch(); + this.progressTimeout = setTimeout(this.handleTimeout, this.ANALYSIS_TIMEOUT_MS); + 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); + } } diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index d35e815dc204..e8147866b52b 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -32,10 +32,12 @@ export const UNITTEST_STOP = 'UNITTEST.STOP'; export const UNITTEST_RUN = 'UNITTEST.RUN'; export const UNITTEST_DISCOVER = 'UNITTEST.DISCOVER'; export const UNITTEST_VIEW_OUTPUT = 'UNITTEST.VIEW_OUTPUT'; +export const PYTHON_LANGUAGE_SERVER_ANALYSISTIME = 'PYTHON_LANGUAGE_SERVER.ANALYSIS_TIME'; export const PYTHON_LANGUAGE_SERVER_ENABLED = 'PYTHON_LANGUAGE_SERVER.ENABLED'; export const PYTHON_LANGUAGE_SERVER_EXTRACTED = 'PYTHON_LANGUAGE_SERVER.EXTRACTED'; export const PYTHON_LANGUAGE_SERVER_DOWNLOADED = 'PYTHON_LANGUAGE_SERVER.DOWNLOADED'; export const PYTHON_LANGUAGE_SERVER_ERROR = 'PYTHON_LANGUAGE_SERVER.ERROR'; export const PYTHON_LANGUAGE_SERVER_STARTUP = 'PYTHON_LANGUAGE_SERVER.STARTUP'; export const PYTHON_LANGUAGE_SERVER_PLATFORM_NOT_SUPPORTED = 'PYTHON_LANGUAGE_SERVER.PLATFORM_NOT_SUPPORTED'; + export const TERMINAL_CREATE = 'TERMINAL.CREATE'; From 9ea1c399b488932783a1e6a89c63da8a009a2db8 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 22:19:35 -0700 Subject: [PATCH 05/11] Add news file --- news/1 Enhancements/2461.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/1 Enhancements/2461.md diff --git a/news/1 Enhancements/2461.md b/news/1 Enhancements/2461.md new file mode 100644 index 000000000000..8586d58bf3bf --- /dev/null +++ b/news/1 Enhancements/2461.md @@ -0,0 +1 @@ +Add telemetry to download, extract, and analyze, phases of the Python Language Server From fa2376f7f85a8fe4c08db8d233110fa69456a11a Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 22:21:24 -0700 Subject: [PATCH 06/11] Add news file for 2297 as well --- news/2 Fixes/2297.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/2297.md diff --git a/news/2 Fixes/2297.md b/news/2 Fixes/2297.md new file mode 100644 index 000000000000..36219d665bfa --- /dev/null +++ b/news/2 Fixes/2297.md @@ -0,0 +1 @@ +Stop duplicate initializations of Python Language Server progress reporter. From 2f6cd5fb531f60a2eb9933190bf3bf59718df25b Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 10:52:56 -0700 Subject: [PATCH 07/11] Add constant for the extension ID - versus hard-coding the string in multiple places. --- src/client/common/constants.ts | 2 ++ src/test/aaFirstTest/aaFirstTest.test.ts | 3 ++- src/test/initialize.ts | 3 ++- src/test/performance/load.perf.test.ts | 3 ++- src/test/performanceTest.ts | 4 ++-- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/client/common/constants.ts b/src/client/common/constants.ts index bebb4777d065..7f8ceed0d6a4 100644 --- a/src/client/common/constants.ts +++ b/src/client/common/constants.ts @@ -6,6 +6,8 @@ export const PYTHON = [ { scheme: 'untitled', language: PYTHON_LANGUAGE } ]; +export const PVSC_EXTENSION_ID = 'ms-python.python'; + export namespace Commands { export const Set_Interpreter = 'python.setInterpreter'; export const Set_ShebangInterpreter = 'python.setShebangInterpreter'; diff --git a/src/test/aaFirstTest/aaFirstTest.test.ts b/src/test/aaFirstTest/aaFirstTest.test.ts index 887932d07669..0e2d101ff9e0 100644 --- a/src/test/aaFirstTest/aaFirstTest.test.ts +++ b/src/test/aaFirstTest/aaFirstTest.test.ts @@ -3,6 +3,7 @@ import { expect } from 'chai'; import { extensions } from 'vscode'; +import { PVSC_EXTENSION_ID } from '../../client/common/constants'; import { initialize } from '../initialize'; // NOTE: @@ -17,6 +18,6 @@ suite('Activate Extension', () => { await initialize(); }); test('Python extension has activated', async () => { - expect(extensions.getExtension('ms-python.python')!.isActive).to.equal(true, 'Extension has not been activated'); + expect(extensions.getExtension(PVSC_EXTENSION_ID)!.isActive).to.equal(true, 'Extension has not been activated'); }); }); diff --git a/src/test/initialize.ts b/src/test/initialize.ts index 588951b60e6e..7d5fd471a1ed 100644 --- a/src/test/initialize.ts +++ b/src/test/initialize.ts @@ -4,6 +4,7 @@ import * as path from 'path'; import * as vscode from 'vscode'; import { IExtensionApi } from '../client/api'; import { PythonSettings } from '../client/common/configSettings'; +import { PVSC_EXTENSION_ID } from '../client/common/constants'; import { clearPythonPathInWorkspaceFolder, PYTHON_PATH, resetGlobalPythonPathSetting, setPythonPathInWorkspaceRoot } from './common'; export * from './constants'; @@ -32,7 +33,7 @@ export async function initialize(): Promise { PythonSettings.dispose(); } export async function activateExtension() { - const extension = vscode.extensions.getExtension('ms-python.python')!; + const extension = vscode.extensions.getExtension(PVSC_EXTENSION_ID)!; if (extension.isActive) { return; } diff --git a/src/test/performance/load.perf.test.ts b/src/test/performance/load.perf.test.ts index 695acc5d5372..f3d8a31d1c04 100644 --- a/src/test/performance/load.perf.test.ts +++ b/src/test/performance/load.perf.test.ts @@ -10,6 +10,7 @@ import * as fs from 'fs-extra'; import { EOL } from 'os'; import * as path from 'path'; import { commands, extensions } from 'vscode'; +import { PVSC_EXTENSION_ID } from '../../client/common/constants'; import { StopWatch } from '../../utils/stopWatch'; const AllowedIncreaseInActivationDelayInMS = 500; @@ -22,7 +23,7 @@ suite('Activation Times', () => { return; } test(`Capture Extension Activation Times (Version: ${process.env.ACTIVATION_TIMES_EXT_VERSION}, sample: ${sampleCounter})`, async () => { - const pythonExtension = extensions.getExtension('ms-python.python'); + const pythonExtension = extensions.getExtension(PVSC_EXTENSION_ID); if (!pythonExtension) { throw new Error('Python Extension not found'); } diff --git a/src/test/performanceTest.ts b/src/test/performanceTest.ts index c9e5a1d13538..c7ea970a8995 100644 --- a/src/test/performanceTest.ts +++ b/src/test/performanceTest.ts @@ -19,7 +19,7 @@ import * as download from 'download'; import * as fs from 'fs-extra'; import * as path from 'path'; import * as request from 'request'; -import { EXTENSION_ROOT_DIR } from '../client/common/constants'; +import { EXTENSION_ROOT_DIR, PVSC_EXTENSION_ID } from '../client/common/constants'; const NamedRegexp = require('named-js-regexp'); const StreamZip = require('node-stream-zip'); @@ -123,7 +123,7 @@ class TestRunner { } private async getReleaseVersion(): Promise { - const url = 'https://marketplace.visualstudio.com/items?itemName=ms-python.python'; + const url = `https://marketplace.visualstudio.com/items?itemName=${PVSC_EXTENSION_ID}`; const content = await new Promise((resolve, reject) => { request(url, (error, response, body) => { if (error) { From 4fd934881c06a0d550cf3b2478100e953b381a81 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 14 Sep 2018 22:27:22 -0700 Subject: [PATCH 08/11] Use constant for extension ID --- src/client/telemetry/telemetry.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/client/telemetry/telemetry.ts b/src/client/telemetry/telemetry.ts index b208b0a220fc..6ae05f9de6c0 100644 --- a/src/client/telemetry/telemetry.ts +++ b/src/client/telemetry/telemetry.ts @@ -8,6 +8,7 @@ import { extensions } from 'vscode'; // tslint:disable-next-line:import-name import TelemetryReporter from 'vscode-extension-telemetry'; +import { PVSC_EXTENSION_ID } from '../common/constants'; // tslint:disable-next-line:no-any let telemetryReporter: TelemetryReporter; @@ -15,7 +16,7 @@ export function getTelemetryReporter() { if (telemetryReporter) { return telemetryReporter; } - const extensionId = 'ms-python.python'; + const extensionId = PVSC_EXTENSION_ID; // tslint:disable-next-line:no-non-null-assertion const extension = extensions.getExtension(extensionId)!; // tslint:disable-next-line:no-unsafe-any From 0dd4175b7f58e533f1fc2783428ad2d17f4d8a7f Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Mon, 17 Sep 2018 14:29:24 -0700 Subject: [PATCH 09/11] Remove use of captureTelemetry - to avoid pollute the generic nature of the decorator, it's best to make this solution explicit --- src/client/activation/downloader.ts | 47 ++++++++++++------------- src/client/activation/languageServer.ts | 16 +-------- src/client/telemetry/index.ts | 13 ++----- src/client/telemetry/types.ts | 2 +- 4 files changed, 27 insertions(+), 51 deletions(-) diff --git a/src/client/activation/downloader.ts b/src/client/activation/downloader.ts index 565efc0de90a..bc7704c7732e 100644 --- a/src/client/activation/downloader.ts +++ b/src/client/activation/downloader.ts @@ -7,14 +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 { captureTelemetry } from '../telemetry'; +import { sendTelemetryEvent } from '../telemetry'; import { - PYTHON_LANGUAGE_SERVER_DOWNLOADED, - PYTHON_LANGUAGE_SERVER_EXTRACTED + PYTHON_LANGUAGE_SERVER_DOWNLOADED } from '../telemetry/constants'; -import { LanguageServerTelemetry } from '../telemetry/types'; import { PlatformData, PlatformName } from './platformData'; import { IDownloadFileService } from './types'; @@ -34,10 +33,6 @@ export const DownloadLinks = { }; export class LanguageServerDownloader { - //tslint:disable-next-line:no-unused-variable - private lsDownloadTelemetry: LanguageServerTelemetry = {}; - //tslint:disable-next-line:no-unused-variable - private lsExtractTelemetry: LanguageServerTelemetry = {}; constructor( private readonly output: IOutputChannel, @@ -54,28 +49,38 @@ export class LanguageServerDownloader { public async downloadLanguageServer(context: IExtensionContext): Promise { 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 }); + await this.fs.deleteFile(localTempFilePath); } } - @captureTelemetry( - PYTHON_LANGUAGE_SERVER_DOWNLOADED, - {}, - true, - (props?: LanguageServerTelemetry) => props ? props.success = true : undefined - ) private async downloadFile(uri: string, title: string): Promise { this.output.append(`Downloading ${uri}... `); const tempFile = await this.fs.createTemporaryFile(downloadFileExtension); @@ -118,12 +123,6 @@ export class LanguageServerDownloader { return tempFile.filePath; } - @captureTelemetry( - PYTHON_LANGUAGE_SERVER_EXTRACTED, - this.languageServerStartupTelemetry, - true, - (props?: LanguageServerTelemetry) => props ? props.success = true : undefined - ) private async unpackArchive(extensionPath: string, tempFilePath: string): Promise { this.output.append('Unpacking archive... '); diff --git a/src/client/activation/languageServer.ts b/src/client/activation/languageServer.ts index ddf8c74e5322..2d31a9868c2d 100644 --- a/src/client/activation/languageServer.ts +++ b/src/client/activation/languageServer.ts @@ -30,15 +30,11 @@ import { import { IEnvironmentVariablesProvider } from '../common/variables/types'; import { IServiceContainer } from '../ioc/types'; import { LanguageServerSymbolProvider } from '../providers/symbolProvider'; -import { captureTelemetry } from '../telemetry'; import { - PYTHON_LANGUAGE_SERVER_DOWNLOADED, PYTHON_LANGUAGE_SERVER_ENABLED, - PYTHON_LANGUAGE_SERVER_ERROR, - PYTHON_LANGUAGE_SERVER_STARTUP + PYTHON_LANGUAGE_SERVER_ERROR } from '../telemetry/constants'; import { getTelemetryReporter } from '../telemetry/telemetry'; -import { LanguageServerTelemetry } from '../telemetry/types'; import { IUnitTestManagementService } from '../unittests/types'; import { LanguageServerDownloader } from './downloader'; import { InterpreterData, InterpreterDataService } from './interpreterDataService'; @@ -81,8 +77,6 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { private surveyBanner: IPythonExtensionBanner; // tslint:disable-next-line:no-unused-variable private progressReporting: ProgressReporting | undefined; - //tslint:disable-next-line:no-unused-variable - private languageServerStartupTelemetry: LanguageServerTelemetry = {}; constructor(@inject(IServiceContainer) private readonly services: IServiceContainer) { this.context = this.services.get(IExtensionContext); @@ -148,15 +142,8 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { (this.configuration.getSettings() as PythonSettings).removeListener('change', this.onSettingsChanged.bind(this)); } - @captureTelemetry( - PYTHON_LANGUAGE_SERVER_STARTUP, - this.languageServerStartupTelemetry, - true, - (props?: LanguageServerTelemetry) => props ? props.success = true : undefined - ) private async startLanguageServer(clientOptions: LanguageClientOptions): Promise { // Determine if we are running MSIL/Universal via dotnet or self-contained app. - const reporter = getTelemetryReporter(); reporter.sendTelemetryEvent(PYTHON_LANGUAGE_SERVER_ENABLED); @@ -177,7 +164,6 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { new RequestWithProxy(this.workspace.getConfiguration('http').get('proxy', '')), languageServerFolder); await downloader.downloadLanguageServer(this.context); - reporter.sendTelemetryEvent(PYTHON_LANGUAGE_SERVER_DOWNLOADED); } const serverModule = path.join(this.context.extensionPath, languageServerFolder, this.platformData.getEngineExecutableName()); diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 12318a0ecf58..92a183ffa863 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -29,11 +29,7 @@ export function sendTelemetryEvent(eventName: string, durationMs?: number, prope export function captureTelemetry( eventName: string, properties?: TelemetryProperties, - captureDuration: boolean = true, - // tslint:disable-next-line:no-any - beforeSuccessEmit?: (props?: any) => void, - // tslint:disable-next-line:no-any - beforeFailEmit?: (props?: any) => void + captureDuration: boolean = true ) { // tslint:disable-next-line:no-function-expression no-any return function (target: Object, propertyKey: string, descriptor: TypedPropertyDescriptor) { @@ -56,17 +52,12 @@ export function captureTelemetry( // tslint:disable-next-line:prefer-type-cast (result as Promise) .then(data => { - if (beforeSuccessEmit) { - beforeSuccessEmit(properties); - } sendTelemetryEvent(eventName, stopWatch.elapsedTime, properties); return data; }) // tslint:disable-next-line:promise-function-async .catch(ex => { - if (beforeFailEmit) { - beforeFailEmit(properties); - } + // tslint:disable-next-line:no-any sendTelemetryEvent(eventName, stopWatch.elapsedTime, properties); return Promise.reject(ex); }); diff --git a/src/client/telemetry/types.ts b/src/client/telemetry/types.ts index 2152a6d268b8..c7e5b6a0c717 100644 --- a/src/client/telemetry/types.ts +++ b/src/client/telemetry/types.ts @@ -16,7 +16,7 @@ export type FormatTelemetry = { }; export type LanguageServerTelemetry = { - success?: boolean; + success: boolean; }; export type LinterTrigger = 'auto' | 'save'; From ec73eb42f4b823722bc2606fbc103401ab2d4c58 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Mon, 17 Sep 2018 16:30:30 -0700 Subject: [PATCH 10/11] Simplify fixups a bit further. --- src/client/activation/downloader.ts | 24 +++++++++++++++--------- src/client/activation/progress.ts | 29 +++++++++++++---------------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/client/activation/downloader.ts b/src/client/activation/downloader.ts index bc7704c7732e..4443814175ee 100644 --- a/src/client/activation/downloader.ts +++ b/src/client/activation/downloader.ts @@ -12,7 +12,8 @@ import { IFileSystem } from '../common/platform/types'; import { IExtensionContext, IOutputChannel } from '../common/types'; import { sendTelemetryEvent } from '../telemetry'; import { - PYTHON_LANGUAGE_SERVER_DOWNLOADED + PYTHON_LANGUAGE_SERVER_DOWNLOADED, + PYTHON_LANGUAGE_SERVER_EXTRACTED } from '../telemetry/constants'; import { PlatformData, PlatformName } from './platformData'; import { IDownloadFileService } from './types'; @@ -55,28 +56,33 @@ export class LanguageServerDownloader { 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('download failed.'); this.output.appendLine(err); success = false; throw new Error(err); } finally { - sendTelemetryEvent(PYTHON_LANGUAGE_SERVER_DOWNLOADED, timer.elapsedTime, { success: success === true }); + sendTelemetryEvent( + PYTHON_LANGUAGE_SERVER_DOWNLOADED, + timer.elapsedTime, + { success } + ); } timer.reset(); try { await this.unpackArchive(context.extensionPath, localTempFilePath); - success = true; } catch (err) { - this.output.appendLine('failed.'); + this.output.appendLine('extraction failed.'); this.output.appendLine(err); - success = true; + success = false; throw new Error(err); } finally { - sendTelemetryEvent('DEREK2', timer.elapsedTime, { success: success === true }); + sendTelemetryEvent( + PYTHON_LANGUAGE_SERVER_EXTRACTED, + timer.elapsedTime, + { success } + ); await this.fs.deleteFile(localTempFilePath); } } diff --git a/src/client/activation/progress.ts b/src/client/activation/progress.ts index 7026f3c54fb0..fca8a8769f90 100644 --- a/src/client/activation/progress.ts +++ b/src/client/activation/progress.ts @@ -7,14 +7,14 @@ 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 | undefined; - private progressTimer: StopWatch | undefined; - private progressTimeout: NodeJS.Timer | undefined; + private progressTimer?: StopWatch; + // tslint:disable-next-line:no-unused-variable + private progressTimeout?: NodeJS.Timer; private ANALYSIS_TIMEOUT_MS: number = 60000; constructor(private readonly languageClient: LanguageClient) { @@ -26,13 +26,16 @@ 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. + if (this.progressDeferred) { return; } this.progressDeferred = createDeferred(); this.progressTimer = new StopWatch(); - this.progressTimeout = setTimeout(this.handleTimeout, this.ANALYSIS_TIMEOUT_MS); + this.progressTimeout = setTimeout( + this.handleTimeout.bind(this), + this.ANALYSIS_TIMEOUT_MS + ); window.withProgress({ location: ProgressLocation.Window, @@ -54,28 +57,22 @@ export class ProgressReporting { if (this.progressDeferred) { this.progressDeferred.resolve(); this.progressDeferred = undefined; - this.completeAnalysisTracking(true); this.progress = undefined; + this.completeAnalysisTracking(true); } }); } - private completeAnalysisTracking(isSuccess: boolean): void { + private completeAnalysisTracking(success: boolean): void { if (this.progressTimer) { - const lsAnalysisTelemetry: LanguageServerTelemetry = { - success: isSuccess - }; sendTelemetryEvent( PYTHON_LANGUAGE_SERVER_ANALYSISTIME, this.progressTimer.elapsedTime, - lsAnalysisTelemetry + { success } ); - this.progressTimer = undefined; - } - - if (this.progressTimeout) { - this.progressTimeout = undefined; } + this.progressTimer = undefined; + this.progressTimeout = undefined; } // tslint:disable-next-line:no-any From c211f0ffaa422b314febdd406eaa765d93ab4261 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Tue, 18 Sep 2018 08:09:23 -0700 Subject: [PATCH 11/11] PR suggestion fixup. --- src/client/activation/progress.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/client/activation/progress.ts b/src/client/activation/progress.ts index fca8a8769f90..cbef24abc39c 100644 --- a/src/client/activation/progress.ts +++ b/src/client/activation/progress.ts @@ -8,6 +8,10 @@ import { StopWatch } from '../../utils/stopWatch'; import { sendTelemetryEvent } from '../telemetry'; import { PYTHON_LANGUAGE_SERVER_ANALYSISTIME } from '../telemetry/constants'; +// Draw the line at Language Server analysis 'timing out' +// and becoming a failure-case at 1 minute: +const ANALYSIS_TIMEOUT_MS: number = 60000; + export class ProgressReporting { private statusBarMessage: Disposable | undefined; private progress: Progress<{ message?: string; increment?: number }> | undefined; @@ -15,7 +19,6 @@ export class ProgressReporting { private progressTimer?: StopWatch; // tslint:disable-next-line:no-unused-variable private progressTimeout?: NodeJS.Timer; - private ANALYSIS_TIMEOUT_MS: number = 60000; constructor(private readonly languageClient: LanguageClient) { this.languageClient.onNotification('python/setStatusBarMessage', (m: string) => { @@ -34,7 +37,7 @@ export class ProgressReporting { this.progressTimer = new StopWatch(); this.progressTimeout = setTimeout( this.handleTimeout.bind(this), - this.ANALYSIS_TIMEOUT_MS + ANALYSIS_TIMEOUT_MS ); window.withProgress({