From 410fe51b29fbede8ce53f87d55d1c1695214b8f4 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 29 Apr 2020 11:31:43 -0700 Subject: [PATCH 01/11] Add pipenv discovery telemetry (update when getInterpreters signature change gets merged) --- news/3 Code Health/11128.md | 1 + .../locators/services/pipEnvService.ts | 15 +++++++++++++++ src/client/telemetry/constants.ts | 1 + src/client/telemetry/index.ts | 4 ++++ 4 files changed, 21 insertions(+) create mode 100644 news/3 Code Health/11128.md diff --git a/news/3 Code Health/11128.md b/news/3 Code Health/11128.md new file mode 100644 index 000000000000..0815e575cd62 --- /dev/null +++ b/news/3 Code Health/11128.md @@ -0,0 +1 @@ +Add telemetry for pipenv interpreter discovery. diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 4de14e4de07c..9065e7cf4901 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -9,7 +9,10 @@ import { traceError, traceWarning } from '../../../common/logger'; import { IFileSystem, IPlatformService } from '../../../common/platform/types'; import { IProcessServiceFactory } from '../../../common/process/types'; import { IConfigurationService, ICurrentProcess } from '../../../common/types'; +import { StopWatch } from '../../../common/utils/stopWatch'; import { IServiceContainer } from '../../../ioc/types'; +import { sendTelemetryEvent } from '../../../telemetry'; +import { EventName } from '../../../telemetry/constants'; import { IInterpreterHelper, InterpreterType, IPipEnvService, PythonInterpreter } from '../../contracts'; import { IPipEnvServiceHelper } from '../types'; import { CacheableLocatorService } from './cacheableLocatorService'; @@ -49,6 +52,18 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer return this.configService.getSettings().pipenvPath; } + public async getInterpreters(resource?: Uri, ignoreCache?: boolean): Promise { + const stopwatch = new StopWatch(); + const startDiscoveryTime = stopwatch.elapsedTime; + + const interpreters = await super.getInterpreters(resource, ignoreCache); + + const discoveryDuration = stopwatch.elapsedTime - startDiscoveryTime; + sendTelemetryEvent(EventName.PIPENV_INTERPRETER_DISCOVERY, discoveryDuration); + + return interpreters; + } + protected getInterpretersImplementation(resource?: Uri): Promise { const pipenvCwd = this.getPipenvWorkingDirectory(resource); if (!pipenvCwd) { diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index 1e5016d9da4d..f1bb3508b303 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -28,6 +28,7 @@ export enum EventName { PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES = 'PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES', PYTHON_INTERPRETER_ACTIVATION_FOR_RUNNING_CODE = 'PYTHON_INTERPRETER_ACTIVATION_FOR_RUNNING_CODE', PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL = 'PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL', + PIPENV_INTERPRETER_DISCOVERY = 'PIPENV_INTERPRETER_DISCOVERY', TERMINAL_SHELL_IDENTIFICATION = 'TERMINAL_SHELL_IDENTIFICATION', PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT = 'PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT', PYTHON_NOT_INSTALLED_PROMPT = 'PYTHON_NOT_INSTALLED_PROMPT', diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 1a7f32ba5d11..89a918976ca9 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1042,6 +1042,10 @@ export interface IEventNamePropertyMapping { */ interpreters?: number; }; + /** + * Telemetry event sent when pipenv interpreter discovery is executed. + */ + [EventName.PIPENV_INTERPRETER_DISCOVERY]: never | undefined; /** * Telemetry event sent with details when user clicks the prompt with the following message * `Prompt message` :- 'We noticed you're using a conda environment. If you are experiencing issues with this environment in the integrated terminal, we suggest the "terminal.integrated.inheritEnv" setting to be changed to false. Would you like to update this setting?' From d947bf319d61fcd029ba7e8ca81ef89255785719 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 29 Apr 2020 14:01:29 -0700 Subject: [PATCH 02/11] Update package.json --- package-lock.json | 84 +++++++++++++++++++++++++++++++++++++---------- package.json | 2 +- 2 files changed, 68 insertions(+), 18 deletions(-) diff --git a/package-lock.json b/package-lock.json index f22c8913328c..68b22adefa75 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4657,13 +4657,14 @@ } }, "applicationinsights": { - "version": "1.0.6", - "resolved": "https://registry.npmjs.org/applicationinsights/-/applicationinsights-1.0.6.tgz", - "integrity": "sha512-VQT3kBpJVPw5fCO5n+WUeSx0VHjxFtD7znYbILBlVgOS9/cMDuGFmV2Br3ObzFyZUDGNbEfW36fD1y2/vAiCKw==", + "version": "1.7.4", + "resolved": "https://registry.npmjs.org/applicationinsights/-/applicationinsights-1.7.4.tgz", + "integrity": "sha512-XFLsNlcanpjFhHNvVWEfcm6hr7lu9znnb6Le1Lk5RE03YUV9X2B2n2MfM4kJZRrUdV+C0hdHxvWyv+vWoLfY7A==", "requires": { + "cls-hooked": "^4.2.2", + "continuation-local-storage": "^3.2.1", "diagnostic-channel": "0.2.0", - "diagnostic-channel-publishers": "0.2.1", - "zone.js": "0.7.6" + "diagnostic-channel-publishers": "^0.3.3" } }, "aproba": { @@ -5230,11 +5231,28 @@ "integrity": "sha512-z/WhQ5FPySLdvREByI2vZiTWwCnF0moMJ1hK9YQwDTHKh6I7/uSckMetoRGb5UBZPC1z0jlw+n/XCgjeH7y1AQ==", "dev": true }, + "async-hook-jl": { + "version": "1.7.6", + "resolved": "https://registry.npmjs.org/async-hook-jl/-/async-hook-jl-1.7.6.tgz", + "integrity": "sha512-gFaHkFfSxTjvoxDMYqDuGHlcRyUuamF8s+ZTtJdDzqjws4mCt7v0vuV79/E2Wr2/riMQgtG4/yUtXWs1gZ7JMg==", + "requires": { + "stack-chain": "^1.3.7" + } + }, "async-limiter": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/async-limiter/-/async-limiter-1.0.0.tgz", "integrity": "sha512-jp/uFnooOiO+L211eZOoSyzpOITMXx1rBITauYykG3BRYPu8h0UcxsPNB04RR5vo4Tyz3+ay17tR6JVf9qzYWg==" }, + "async-listener": { + "version": "0.6.10", + "resolved": "https://registry.npmjs.org/async-listener/-/async-listener-0.6.10.tgz", + "integrity": "sha512-gpuo6xOyF4D5DE5WvyqZdPA3NGhiT6Qf07l7DCB0wwDEsLvDIbCr6j9S5aj5Ch96dLace5tXVzWBZkxU/c5ohw==", + "requires": { + "semver": "^5.3.0", + "shimmer": "^1.1.0" + } + }, "async-settle": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/async-settle/-/async-settle-1.0.0.tgz", @@ -6807,6 +6825,16 @@ } } }, + "cls-hooked": { + "version": "4.2.2", + "resolved": "https://registry.npmjs.org/cls-hooked/-/cls-hooked-4.2.2.tgz", + "integrity": "sha512-J4Xj5f5wq/4jAvcdgoGsL3G103BtWpZrMo8NEinRltN+xpTZdI+M38pyQqhuFU/P792xkMFvnKSf+Lm81U1bxw==", + "requires": { + "async-hook-jl": "^1.7.6", + "emitter-listener": "^1.0.1", + "semver": "^5.4.1" + } + }, "clsx": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/clsx/-/clsx-1.0.4.tgz", @@ -7148,6 +7176,15 @@ "resolved": "https://registry.npmjs.org/content-type/-/content-type-1.0.4.tgz", "integrity": "sha512-hIP3EEPs8tB9AT1L+NUqtwOAps4mk2Zob89MWXMHjHWg9milF/j4osnnQLXBCBFBk/tvIG/tUc9mOUJiPBhPXA==" }, + "continuation-local-storage": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/continuation-local-storage/-/continuation-local-storage-3.2.1.tgz", + "integrity": "sha512-jx44cconVqkCEEyLSKWwkvUXwO561jXMa3LPjTPsm5QR22PA0/mhe33FT4Xb5y74JDvt/Cq+5lm8S8rskLv9ZA==", + "requires": { + "async-listener": "^0.6.0", + "emitter-listener": "^1.1.1" + } + }, "convert-source-map": { "version": "1.6.0", "resolved": "https://registry.npmjs.org/convert-source-map/-/convert-source-map-1.6.0.tgz", @@ -8690,9 +8727,9 @@ } }, "diagnostic-channel-publishers": { - "version": "0.2.1", - "resolved": "https://registry.npmjs.org/diagnostic-channel-publishers/-/diagnostic-channel-publishers-0.2.1.tgz", - "integrity": "sha1-ji1geottef6IC1SLxYzGvrKIxPM=" + "version": "0.3.4", + "resolved": "https://registry.npmjs.org/diagnostic-channel-publishers/-/diagnostic-channel-publishers-0.3.4.tgz", + "integrity": "sha512-SZ1zMfFiEabf4Qx0Og9V1gMsRoqz3O+5ENkVcNOfI+SMJ3QhQsdEoKX99r0zvreagXot2parPxmrwwUM/ja8ug==" }, "diagnostics": { "version": "1.1.1", @@ -9059,6 +9096,14 @@ "minimalistic-crypto-utils": "^1.0.0" } }, + "emitter-listener": { + "version": "1.1.2", + "resolved": "https://registry.npmjs.org/emitter-listener/-/emitter-listener-1.1.2.tgz", + "integrity": "sha512-Bt1sBAGFHY9DKY+4/2cV6izcKJUf5T7/gkdmkxzX/qv9CcGH8xSwVRW5mtX03SWJtRTWSOpzCuWN9rBFYZepZQ==", + "requires": { + "shimmer": "^1.2.0" + } + }, "emoji-regex": { "version": "7.0.3", "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-7.0.3.tgz", @@ -20668,6 +20713,11 @@ } } }, + "shimmer": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/shimmer/-/shimmer-1.2.1.tgz", + "integrity": "sha512-sQTKC1Re/rM6XyFM6fIAGHRPVGvyXfgzIDvzoq608vM+jeyVD0Tu1E6Np0Kc2zAIFWIj963V2800iF/9LPieQw==" + }, "shortid": { "version": "2.2.14", "resolved": "https://registry.npmjs.org/shortid/-/shortid-2.2.14.tgz", @@ -21260,6 +21310,11 @@ "figgy-pudding": "^3.5.1" } }, + "stack-chain": { + "version": "1.3.7", + "resolved": "https://registry.npmjs.org/stack-chain/-/stack-chain-1.3.7.tgz", + "integrity": "sha1-0ZLJ/06moiyUxN1FkXHj8AzqEoU=" + }, "stack-trace": { "version": "0.0.10", "resolved": "https://registry.npmjs.org/stack-trace/-/stack-trace-0.0.10.tgz", @@ -24563,11 +24618,11 @@ "integrity": "sha512-+OMm11R1bGYbpIJ5eQIkwoDGFF4GvBz3Ztl6/VM+/RNNb2Gjk2c0Ku+oMmfhlTmTlPCpgHBsH4JqVCbUYhu5bA==" }, "vscode-extension-telemetry": { - "version": "0.1.0", - "resolved": "https://registry.npmjs.org/vscode-extension-telemetry/-/vscode-extension-telemetry-0.1.0.tgz", - "integrity": "sha512-WVCnP+uLxlqB6UD98yQNV47mR5Rf79LFxpuZhSPhEf0Sb4tPZed3a63n003/dchhOwyCTCBuNN4n8XKJkLEI1Q==", + "version": "0.1.4", + "resolved": "https://registry.npmjs.org/vscode-extension-telemetry/-/vscode-extension-telemetry-0.1.4.tgz", + "integrity": "sha512-9U2pUZ/YwZBfA8CkBrHwMxjnq9Ab+ng8daJWJzEQ6CAxlZyRhmck23bx2lqqpEwGWJCiuceQy4k0Me6llEB4zw==", "requires": { - "applicationinsights": "1.0.6" + "applicationinsights": "1.7.4" } }, "vscode-jsonrpc": { @@ -25681,11 +25736,6 @@ } } } - }, - "zone.js": { - "version": "0.7.6", - "resolved": "https://registry.npmjs.org/zone.js/-/zone.js-0.7.6.tgz", - "integrity": "sha1-+7w50+AmHQmG8boGMG6zrrDSIAk=" } } } diff --git a/package.json b/package.json index 0630899c5a9b..cc6d19bc15c2 100644 --- a/package.json +++ b/package.json @@ -2978,7 +2978,7 @@ "untildify": "^3.0.2", "vscode-debugadapter": "^1.28.0", "vscode-debugprotocol": "^1.28.0", - "vscode-extension-telemetry": "0.1.0", + "vscode-extension-telemetry": "0.1.4", "vscode-jsonrpc": "^5.0.1", "vscode-languageclient": "^6.2.0-next.2", "vscode-languageserver": "^6.2.0-next.2", From f829d5cde2971eb1e88375dd7705abc773f87142 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 29 Apr 2020 14:53:45 -0700 Subject: [PATCH 03/11] Use firstParty optional arg --- src/client/telemetry/index.ts | 2 +- src/client/telemetry/vscode-extension-telemetry.d.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 89a918976ca9..e68933d47edf 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -70,7 +70,7 @@ function getTelemetryReporter() { // tslint:disable-next-line:no-require-imports const reporter = require('vscode-extension-telemetry').default as typeof TelemetryReporter; - return (telemetryReporter = new reporter(extensionId, extensionVersion, AppinsightsKey)); + return (telemetryReporter = new reporter(extensionId, extensionVersion, AppinsightsKey, true)); } export function clearTelemetryReporter() { diff --git a/src/client/telemetry/vscode-extension-telemetry.d.ts b/src/client/telemetry/vscode-extension-telemetry.d.ts index a5d3d6294739..aafdaaf4a40a 100644 --- a/src/client/telemetry/vscode-extension-telemetry.d.ts +++ b/src/client/telemetry/vscode-extension-telemetry.d.ts @@ -8,9 +8,10 @@ declare module 'vscode-extension-telemetry' { * @param {string} extensionId All events will be prefixed with this event name * @param {string} extensionVersion Extension version to be reported with each event * @param {string} key The application insights key + * @param {boolean} firstParty Force the reporter to treat telemetry as coming from a first-party extension. This parameter will not override in the false direction. */ // tslint:disable-next-line:no-empty - constructor(extensionId: string, extensionVersion: string, key: string); + constructor(extensionId: string, extensionVersion: string, key: string, firstParty?: boolean); /** * Sends a telemetry event From 4c90c615a8b4c32d23fe439cfcb9f7247d412a0a Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 29 Apr 2020 15:00:00 -0700 Subject: [PATCH 04/11] News file --- news/3 Code Health/11436.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/3 Code Health/11436.md diff --git a/news/3 Code Health/11436.md b/news/3 Code Health/11436.md new file mode 100644 index 000000000000..4d5ba9f74640 --- /dev/null +++ b/news/3 Code Health/11436.md @@ -0,0 +1 @@ +Update telemetry on errors and exceptions to use [vscode-extension-telemetry](https://www.npmjs.com/package/vscode-extension-telemetry). From 2a56f591cc9d0b3510f6d5069130d3f0bfd45ce1 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 29 Apr 2020 17:31:02 -0700 Subject: [PATCH 05/11] Sanitize paths using telemetry package --- src/client/telemetry/index.ts | 53 +++++++------------ .../telemetry/vscode-extension-telemetry.d.ts | 34 ------------ 2 files changed, 18 insertions(+), 69 deletions(-) delete mode 100644 src/client/telemetry/vscode-extension-telemetry.d.ts diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index e68933d47edf..ad788922fee4 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1,15 +1,15 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -// tslint:disable:no-reference no-any import-name no-any function-name -/// + +// tslint:disable: no-any import type { JSONObject } from '@phosphor/coreutils'; -import { basename as pathBasename, sep as pathSep } from 'path'; import * as stackTrace from 'stack-trace'; -import TelemetryReporter from 'vscode-extension-telemetry'; +// tslint:disable-next-line: import-name +import TelemetryReporter from 'vscode-extension-telemetry/lib/telemetryReporter'; import { DiagnosticCodes } from '../application/diagnostics/constants'; import { IWorkspaceService } from '../common/application/types'; -import { AppinsightsKey, EXTENSION_ROOT_DIR, isTestExecution, PVSC_EXTENSION_ID } from '../common/constants'; +import { AppinsightsKey, isTestExecution, PVSC_EXTENSION_ID } from '../common/constants'; import { traceError, traceInfo } from '../common/logger'; import { TerminalShellType } from '../common/terminal/types'; import { StopWatch } from '../common/utils/stopWatch'; @@ -89,17 +89,20 @@ export function sendTelemetryEvent

= {}; - props.stackTrace = getStackTrace(ex); - props.originalEventName = (eventName as any) as string; - reporter.sendTelemetryEvent('ERROR', props, measures); + reporter.sendTelemetryErrorEvent( + eventName as string, + { originalEventName: eventName as string, stackTrace: serializeStackTrace(ex) }, + measures, + [] + ); } + const customProperties: Record = {}; if (properties) { // tslint:disable-next-line:prefer-type-cast no-any @@ -246,32 +249,12 @@ export function sendTelemetryWhenDone

${filename.substring(EXTENSION_ROOT_DIR.length)}`; - } else { - // We don't really care about files outside our extension. - filename = `${pathSep}${pathBasename(filename)}`; - } - return filename; -} - -function sanitizeName(name: string): string { - if (name.indexOf('/') === -1 && name.indexOf('\\') === -1) { - return name; - } else { - return ''; - } -} - -function getStackTrace(ex: Error): string { - // We aren't showing the error message (ex.message) since it might - // contain PII. +function serializeStackTrace(ex: Error): string { + // We aren't showing the error message (ex.message) since it might contain PII. let trace = ''; for (const frame of stackTrace.parse(ex)) { - let filename = frame.getFileName(); + const filename = frame.getFileName(); if (filename) { - filename = sanitizeFilename(filename); const lineno = frame.getLineNumber(); const colno = frame.getColumnNumber(); trace += `\n\tat ${getCallsite(frame)} ${filename}:${lineno}:${colno}`; @@ -297,7 +280,7 @@ function getCallsite(frame: stackTrace.StackFrame) { parts.push(frame.getFunctionName()); } } - return parts.map(sanitizeName).join('.'); + return parts.join('.'); } // Map all events to their properties diff --git a/src/client/telemetry/vscode-extension-telemetry.d.ts b/src/client/telemetry/vscode-extension-telemetry.d.ts deleted file mode 100644 index aafdaaf4a40a..000000000000 --- a/src/client/telemetry/vscode-extension-telemetry.d.ts +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -declare module 'vscode-extension-telemetry' { - export default class TelemetryReporter { - /** - * Constructs a new telemetry reporter - * @param {string} extensionId All events will be prefixed with this event name - * @param {string} extensionVersion Extension version to be reported with each event - * @param {string} key The application insights key - * @param {boolean} firstParty Force the reporter to treat telemetry as coming from a first-party extension. This parameter will not override in the false direction. - */ - // tslint:disable-next-line:no-empty - constructor(extensionId: string, extensionVersion: string, key: string, firstParty?: boolean); - - /** - * Sends a telemetry event - * @param {string} eventName The event name - * @param {object} properties An associative array of strings - * @param {object} measures An associative array of numbers - */ - // tslint:disable-next-line:member-access - public sendTelemetryEvent( - eventName: string, - properties?: { - [key: string]: string; - }, - measures?: { - [key: string]: number; - // tslint:disable-next-line:no-empty - } - ): void; - } -} From dfa18dc90859302f378744028c70b6eac854c121 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 29 Apr 2020 17:31:26 -0700 Subject: [PATCH 06/11] Remove outdated warnings, typos --- src/client/telemetry/index.ts | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-) diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index ad788922fee4..6b662b08a7de 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -31,7 +31,7 @@ import { LinterTrigger, TestTool } from './types'; /** * Checks whether telemetry is supported. * Its possible this function gets called within Debug Adapter, vscode isn't available in there. - * Withiin DA, there's a completely different way to send telemetry. + * Within DA, there's a completely different way to send telemetry. * @returns {boolean} */ function isTelemetrySupported(): boolean { @@ -63,9 +63,7 @@ function getTelemetryReporter() { const extensionId = PVSC_EXTENSION_ID; // tslint:disable-next-line:no-require-imports const extensions = (require('vscode') as typeof import('vscode')).extensions; - // tslint:disable-next-line:no-non-null-assertion const extension = extensions.getExtension(extensionId)!; - // tslint:disable-next-line:no-unsafe-any const extensionVersion = extension.packageJSON.version; // tslint:disable-next-line:no-require-imports @@ -105,7 +103,6 @@ export function sendTelemetryEvent

= {}; if (properties) { - // tslint:disable-next-line:prefer-type-cast no-any const data = properties as any; Object.getOwnPropertyNames(data).forEach((prop) => { if (data[prop] === undefined || data[prop] === null) { @@ -113,9 +110,8 @@ export function sendTelemetryEvent

= ( * @param lazyProperties A static function on the decorated class which returns extra properties to add to the event. * This can be used to provide properties which are only known at runtime (after the decorator has executed). */ -// tslint:disable-next-line:no-any function-name export function captureTelemetry( eventName: E, properties?: P[E], @@ -160,20 +155,18 @@ export function captureTelemetry P[E] ): TypedMethodDescriptor<(this: This, ...args: any[]) => any> { - // tslint:disable-next-line:no-function-expression no-any return function ( _target: Object, _propertyKey: string | symbol, descriptor: TypedPropertyDescriptor<(this: This, ...args: any[]) => any> ) { const originalMethod = descriptor.value!; - // tslint:disable-next-line:no-function-expression no-any + // tslint:disable-next-line: no-function-expression descriptor.value = function (this: This, ...args: any[]) { // Legacy case; fast path that sends event before method executes. // Does not set "failed" if the result is a Promise and throws an exception. if (!captureDuration && !lazyProperties) { sendTelemetryEvent(eventName, undefined, properties); - // tslint:disable-next-line:no-invalid-this return originalMethod.apply(this, args); } @@ -185,22 +178,16 @@ export function captureTelemetry) .then((data) => { sendTelemetryEvent(eventName, stopWatch?.elapsedTime, props()); return data; }) - // tslint:disable-next-line:promise-function-async .catch((ex) => { - // tslint:disable-next-line:no-any const failedProps: P[E] = props() || ({} as any); (failedProps as any).failed = true; sendTelemetryEvent( @@ -230,16 +217,12 @@ export function sendTelemetryWhenDone

).then( (data) => { - // tslint:disable-next-line:no-non-null-assertion sendTelemetryEvent(eventName, stopWatch!.elapsedTime, properties); return data; - // tslint:disable-next-line:promise-function-async }, (ex) => { - // tslint:disable-next-line:no-non-null-assertion sendTelemetryEvent(eventName, stopWatch!.elapsedTime, properties, ex); return Promise.reject(ex); } From 6aab4687267b8baa36a46269989ce8a3fca5f5b6 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 29 Apr 2020 17:55:20 -0700 Subject: [PATCH 07/11] More descriptive test names --- src/test/telemetry/index.unit.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/telemetry/index.unit.test.ts b/src/test/telemetry/index.unit.test.ts index aad20386af82..437d41dcd515 100644 --- a/src/test/telemetry/index.unit.test.ts +++ b/src/test/telemetry/index.unit.test.ts @@ -94,7 +94,7 @@ suite('Telemetry', () => { expect(Reporter.measures).to.deep.equal([measures]); expect(Reporter.properties).to.deep.equal([properties]); }); - test('Send Telemetry', () => { + test('Send Telemetry with properties', () => { rewiremock.enable(); rewiremock('vscode-extension-telemetry').with({ default: Reporter }); @@ -128,7 +128,7 @@ suite('Telemetry', () => { delete Reporter.properties[0].stackTrace; expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); }); - test('Send Error Telemetry', () => { + test('Send Error Telemetry with stack trace', () => { rewiremock.enable(); const error = new Error('Boo'); error.stack = [ From 6d7b732670f9e8f33a8bc739b82cd5acb428eab9 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Thu, 30 Apr 2020 12:50:08 -0700 Subject: [PATCH 08/11] Only send one event, fix unit tests --- src/client/telemetry/index.ts | 57 ++++++----- src/test/telemetry/index.unit.test.ts | 137 ++++++-------------------- 2 files changed, 59 insertions(+), 135 deletions(-) diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 6b662b08a7de..6c765238d5ef 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -86,6 +86,8 @@ export function sendTelemetryEvent

= {}; + let eventNameSent = eventName as string; if (ex) { // When sending telemetry events for exceptions no need to send custom properties. @@ -93,39 +95,36 @@ export function sendTelemetryEvent

{ + if (data[prop] === undefined || data[prop] === null) { + return; + } + try { + // If there are any errors in serializing one property, ignore that and move on. + // Else nothing will be sent. + customProperties[prop] = + typeof data[prop] === 'string' + ? data[prop] + : typeof data[prop] === 'object' + ? 'object' + : data[prop].toString(); + } catch (ex) { + traceError(`Failed to serialize ${prop} for ${eventName}`, ex); + } + }); + } + reporter.sendTelemetryEvent(eventNameSent, customProperties, measures); } - const customProperties: Record = {}; - if (properties) { - const data = properties as any; - Object.getOwnPropertyNames(data).forEach((prop) => { - if (data[prop] === undefined || data[prop] === null) { - return; - } - try { - // If there are any errors in serializing one property, ignore that and move on. - // Else nothing will be sent. - customProperties[prop] = - typeof data[prop] === 'string' - ? data[prop] - : typeof data[prop] === 'object' - ? 'object' - : data[prop].toString(); - } catch (ex) { - traceError(`Failed to serialize ${prop} for ${eventName}`, ex); - } - }); - } - reporter.sendTelemetryEvent(eventName as string, customProperties, measures); if (process.env && process.env.VSC_PYTHON_LOG_TELEMETRY) { traceInfo( - `Telemetry Event : ${eventName} Measures: ${JSON.stringify(measures)} Props: ${JSON.stringify( + `Telemetry Event : ${eventNameSent} Measures: ${JSON.stringify(measures)} Props: ${JSON.stringify( customProperties )} ` ); diff --git a/src/test/telemetry/index.unit.test.ts b/src/test/telemetry/index.unit.test.ts index 437d41dcd515..0823d7b341ef 100644 --- a/src/test/telemetry/index.unit.test.ts +++ b/src/test/telemetry/index.unit.test.ts @@ -23,16 +23,22 @@ suite('Telemetry', () => { public static eventName: string[] = []; public static properties: Record[] = []; public static measures: {}[] = []; + public static errorProps: string[] | undefined; public static clear() { Reporter.eventName = []; Reporter.properties = []; Reporter.measures = []; + Reporter.errorProps = undefined; } public sendTelemetryEvent(eventName: string, properties?: {}, measures?: {}) { Reporter.eventName.push(eventName); Reporter.properties.push(properties!); Reporter.measures.push(measures!); } + public sendTelemetryErrorEvent(eventName: string, properties?: {}, measures?: {}, errorProps?: string[]) { + this.sendTelemetryEvent(eventName, properties, measures); + Reporter.errorProps = errorProps; + } } setup(() => { @@ -122,11 +128,12 @@ suite('Telemetry', () => { originalEventName: eventName }; - expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]); - expect(Reporter.measures).to.deep.equal([measures, measures]); + expect(Reporter.eventName).to.deep.equal(['ERROR']); + expect(Reporter.measures).to.deep.equal([measures]); expect(Reporter.properties[0].stackTrace).to.be.length.greaterThan(1); delete Reporter.properties[0].stackTrace; - expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); + expect(Reporter.properties).to.deep.equal([expectedErrorProperties]); + expect(Reporter.errorProps).to.deep.equal([]); }); test('Send Error Telemetry with stack trace', () => { rewiremock.enable(); @@ -167,112 +174,30 @@ suite('Telemetry', () => { const stackTrace = Reporter.properties[0].stackTrace; delete Reporter.properties[0].stackTrace; - expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]); - expect(Reporter.measures).to.deep.equal([measures, measures]); - expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); - expect(stackTrace).to.be.length.greaterThan(1); - - const expectedStack = [ - 'at Context.test /src/test/telemetry/index.unit.test.ts:50:23\n\tat callFn /node_modules/mocha/lib/runnable.js:372:21', - 'at Test.Runnable.run /node_modules/mocha/lib/runnable.js:364:7', - 'at Runner.runTest /node_modules/mocha/lib/runner.js:455:10', - 'at /node_modules/mocha/lib/runner.js:573:12', - 'at next /node_modules/mocha/lib/runner.js:369:14', - 'at /node_modules/mocha/lib/runner.js:379:7', - 'at next /node_modules/mocha/lib/runner.js:303:14', - 'at /node_modules/mocha/lib/runner.js:342:7', - 'at done /node_modules/mocha/lib/runnable.js:319:5', - 'at callFn /node_modules/mocha/lib/runnable.js:395:7', - 'at Hook.Runnable.run /node_modules/mocha/lib/runnable.js:364:7', - 'at next /node_modules/mocha/lib/runner.js:317:10', - 'at Immediate /node_modules/mocha/lib/runner.js:347:5', - 'at runCallback /timers.js:789:20', - 'at tryOnImmediate /timers.js:751:5', - 'at processImmediate [as _immediateCallback] /timers.js:722:5' - ].join('\n\t'); - - expect(stackTrace).to.be.equal(expectedStack); - }); - test('Ensure non extension file paths are stripped from stack trace', () => { - rewiremock.enable(); - const error = new Error('Boo'); - error.stack = [ - 'Error: Boo', - `at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`, - 'at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)', - 'at Test.Runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)', - 'at Runner.runTest (\\wowwee/node_modules/mocha/lib/runner.js:455:10)', - `at Immediate. (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)` - ].join('\n\t'); - rewiremock('vscode-extension-telemetry').with({ default: Reporter }); - - const eventName = 'Testing'; - const properties = { hello: 'world', foo: 'bar' }; - const measures = { start: 123, end: 987 }; - - // tslint:disable-next-line:no-any - sendTelemetryEvent(eventName as any, measures, properties as any, error); - - const expectedErrorProperties = { - originalEventName: eventName - }; - - const stackTrace = Reporter.properties[0].stackTrace; - delete Reporter.properties[0].stackTrace; - - expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]); - expect(Reporter.measures).to.deep.equal([measures, measures]); - expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); - expect(stackTrace).to.be.length.greaterThan(1); - - const expectedStack = [ - 'at Context.test /src/test/telemetry/index.unit.test.ts:50:23', - 'at callFn /runnable.js:372:21', - 'at Test.Runnable.run /runnable.js:364:7', - 'at Runner.runTest /runner.js:455:10', - 'at Immediate /node_modules/mocha/lib/runner.js:347:5' - ].join('\n\t'); - - expect(stackTrace).to.be.equal(expectedStack); - }); - test('Ensure non function names containing file names (unlikely, but for sake of completeness) are stripped from stack trace', () => { - rewiremock.enable(); - const error = new Error('Boo'); - error.stack = [ - 'Error: Boo', - `at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`, - 'at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)', - 'at Test./usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)', - 'at Runner.runTest (\\wowwee/node_modules/mocha/lib/runner.js:455:10)', - `at Immediate. (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5)` - ].join('\n\t'); - rewiremock('vscode-extension-telemetry').with({ default: Reporter }); - - const eventName = 'Testing'; - const properties = { hello: 'world', foo: 'bar' }; - const measures = { start: 123, end: 987 }; - - // tslint:disable-next-line:no-any - sendTelemetryEvent(eventName as any, measures, properties as any, error); - - const expectedErrorProperties = { - originalEventName: eventName - }; - - const stackTrace = Reporter.properties[0].stackTrace; - delete Reporter.properties[0].stackTrace; - - expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]); - expect(Reporter.measures).to.deep.equal([measures, measures]); - expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); + expect(Reporter.eventName).to.deep.equal(['ERROR']); + expect(Reporter.measures).to.deep.equal([measures]); + expect(Reporter.properties).to.deep.equal([expectedErrorProperties]); expect(stackTrace).to.be.length.greaterThan(1); + expect(Reporter.errorProps).to.deep.equal([]); const expectedStack = [ - 'at Context.test /src/test/telemetry/index.unit.test.ts:50:23', - 'at callFn /runnable.js:372:21', - 'at .run /runnable.js:364:7', - 'at Runner.runTest /runner.js:455:10', - 'at Immediate /node_modules/mocha/lib/runner.js:347:5' + `at Context.test ${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23`, + `at callFn ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:372:21`, + `at Test.Runnable.run ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7`, + `at Runner.runTest ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:455:10`, + `at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:573:12`, + `at next ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:369:14`, + `at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:379:7`, + `at next ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:303:14`, + `at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:342:7`, + `at done ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:319:5`, + `at callFn ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:395:7`, + `at Hook.Runnable.run ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7`, + `at next ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:317:10`, + `at Immediate ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347:5`, + 'at runCallback timers.js:789:20', + 'at tryOnImmediate timers.js:751:5', + 'at processImmediate [as _immediateCallback] timers.js:722:5' ].join('\n\t'); expect(stackTrace).to.be.equal(expectedStack); From e6a0e84f6a2a33e23723e19f9b8d930efbba04e6 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Thu, 30 Apr 2020 13:04:18 -0700 Subject: [PATCH 09/11] Don't make unnecessary changes --- src/client/telemetry/index.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 6c765238d5ef..239e20f79dc3 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -119,6 +119,7 @@ export function sendTelemetryEvent

any> ) { const originalMethod = descriptor.value!; - // tslint:disable-next-line: no-function-expression + // tslint:disable-next-line:no-function-expression no-any descriptor.value = function (this: This, ...args: any[]) { // Legacy case; fast path that sends event before method executes. // Does not set "failed" if the result is a Promise and throws an exception. @@ -176,17 +177,22 @@ export function captureTelemetry) .then((data) => { sendTelemetryEvent(eventName, stopWatch?.elapsedTime, props()); return data; }) + // tslint:disable-next-line:promise-function-async .catch((ex) => { + // tslint:disable-next-line:no-any const failedProps: P[E] = props() || ({} as any); (failedProps as any).failed = true; sendTelemetryEvent( @@ -216,12 +222,16 @@ export function sendTelemetryWhenDone

).then( (data) => { + // tslint:disable-next-line:no-non-null-assertion sendTelemetryEvent(eventName, stopWatch!.elapsedTime, properties); return data; + // tslint:disable-next-line:promise-function-async }, (ex) => { + // tslint:disable-next-line:no-non-null-assertion sendTelemetryEvent(eventName, stopWatch!.elapsedTime, properties, ex); return Promise.reject(ex); } From 9263c1c27d4d458fae78853a9971f14688eafeaf Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Thu, 30 Apr 2020 13:06:35 -0700 Subject: [PATCH 10/11] Forgot some --- src/client/telemetry/index.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 239e20f79dc3..386f3fb352e9 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -// tslint:disable: no-any import type { JSONObject } from '@phosphor/coreutils'; import * as stackTrace from 'stack-trace'; // tslint:disable-next-line: import-name @@ -28,6 +27,8 @@ import { TestProvider } from '../testing/common/types'; import { EventName, PlatformErrors } from './constants'; import { LinterTrigger, TestTool } from './types'; +// tslint:disable: no-any + /** * Checks whether telemetry is supported. * Its possible this function gets called within Debug Adapter, vscode isn't available in there. @@ -148,6 +149,7 @@ type TypedMethodDescriptor = ( * @param lazyProperties A static function on the decorated class which returns extra properties to add to the event. * This can be used to provide properties which are only known at runtime (after the decorator has executed). */ +// tslint:disable-next-line:no-any function-name export function captureTelemetry( eventName: E, properties?: P[E], @@ -155,6 +157,7 @@ export function captureTelemetry P[E] ): TypedMethodDescriptor<(this: This, ...args: any[]) => any> { + // tslint:disable-next-line:no-function-expression no-any return function ( _target: Object, _propertyKey: string | symbol, @@ -167,6 +170,7 @@ export function captureTelemetry Date: Thu, 30 Apr 2020 18:19:14 -0700 Subject: [PATCH 11/11] Fix build warnings --- gulpfile.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index f2f91140595b..a290b648282a 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -287,7 +287,9 @@ function getAllowedWarningsForWebPack(buildConfig) { 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/validation.js', 'WARNING in ./node_modules/any-promise/register.js', 'WARNING in ./node_modules/log4js/lib/appenders/index.js', - 'WARNING in ./node_modules/log4js/lib/clustering.js' + 'WARNING in ./node_modules/log4js/lib/clustering.js', + 'WARNING in ./node_modules/diagnostic-channel-publishers/dist/src/azure-coretracing.pub.js', + 'WARNING in ./node_modules/applicationinsights/out/AutoCollection/NativePerformance.js' ]; case 'extension': return [ @@ -300,10 +302,16 @@ function getAllowedWarningsForWebPack(buildConfig) { 'remove-files-plugin@1.4.0:', 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/buffer-util.js', 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/validation.js', - 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/Validation.js' + 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/Validation.js', + 'WARNING in ./node_modules/diagnostic-channel-publishers/dist/src/azure-coretracing.pub.js', + 'WARNING in ./node_modules/applicationinsights/out/AutoCollection/NativePerformance.js' ]; case 'debugAdapter': - return ['WARNING in ./node_modules/vscode-uri/lib/index.js']; + return [ + 'WARNING in ./node_modules/vscode-uri/lib/index.js', + 'WARNING in ./node_modules/diagnostic-channel-publishers/dist/src/azure-coretracing.pub.js', + 'WARNING in ./node_modules/applicationinsights/out/AutoCollection/NativePerformance.js' + ]; default: throw new Error('Unknown WebPack Configuration'); }