Use vscode-extension-telemetry for our exceptions & error telemetry (#…

…11524) * Add pipenv discovery telemetry (update when getInterpreters signature change gets merged) * Update package.json * Use firstParty optional arg * News file * Sanitize paths using telemetry package * Remove outdated warnings, typos * More descriptive test names * Only send one event, fix unit tests * Don't make unnecessary changes * Forgot some * Fix build warnings
microsoft · May 4, 2020 · d5065e6 · d5065e6
1 parent 87c4584
commit d5065e6
Show file tree

Hide file tree

Showing 7 changed files with 157 additions and 226 deletions.
diff --git 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');
     }

diff --git a/news/3 Code Health/11436.md 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).
diff --git a/package-lock.json b/package-lock.json
diff --git 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",

diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts
@@ -1,15 +1,14 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
-// tslint:disable:no-reference no-any import-name no-any function-name
-/// <reference path="./vscode-extension-telemetry.d.ts" />
+
 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';
@@ -28,10 +27,12 @@ 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.
- * 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,14 +64,12 @@ 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
     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() {
@@ -88,45 +87,46 @@ export function sendTelemetryEvent<P extends IEventNamePropertyMapping, E extend
     }
     const reporter = getTelemetryReporter();
     const measures = typeof durationMs === 'number' ? { duration: durationMs } : durationMs ? durationMs : undefined;
+    let customProperties: Record<string, string> = {};
+    let eventNameSent = eventName as string;
 
-    if (ex && (eventName as any) !== 'ERROR') {
-        // When sending `ERROR` telemetry event no need to send custom properties.
+    if (ex) {
+        // When sending telemetry events for exceptions no need to send custom properties.
         // Else we have to review all properties every time as part of GDPR.
         // Assume we have 10 events all with their own properties.
         // As we have errors for each event, those properties are treated as new data items.
         // Hence they need to be classified as part of the GDPR process, and thats unnecessary and onerous.
-        const props: Record<string, string> = {};
-        props.stackTrace = getStackTrace(ex);
-        props.originalEventName = (eventName as any) as string;
-        reporter.sendTelemetryEvent('ERROR', props, measures);
-    }
-    const customProperties: Record<string, string> = {};
-    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) {
-                return;
-            }
-            try {
-                // If there are any errors in serializing one property, ignore that and move on.
-                // Else nothign will be sent.
-                // tslint:disable-next-line:prefer-type-cast no-any  no-unsafe-any
-                (customProperties as any)[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);
-            }
-        });
+        eventNameSent = 'ERROR';
+        customProperties = { originalEventName: eventName as string, stackTrace: serializeStackTrace(ex) };
+        reporter.sendTelemetryErrorEvent(eventNameSent, customProperties, measures, []);
+    } else {
+        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(eventNameSent, customProperties, measures);
     }
-    reporter.sendTelemetryEvent((eventName as any) 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
             )} `
         );
@@ -246,32 +246,12 @@ export function sendTelemetryWhenDone<P extends IEventNamePropertyMapping, E ext
     }
 }
 
-function sanitizeFilename(filename: string): string {
-    if (filename.startsWith(EXTENSION_ROOT_DIR)) {
-        filename = `<pvsc>${filename.substring(EXTENSION_ROOT_DIR.length)}`;
-    } else {
-        // We don't really care about files outside our extension.
-        filename = `<hidden>${pathSep}${pathBasename(filename)}`;
-    }
-    return filename;
-}
-
-function sanitizeName(name: string): string {
-    if (name.indexOf('/') === -1 && name.indexOf('\\') === -1) {
-        return name;
-    } else {
-        return '<hidden>';
-    }
-}
-
-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 +277,7 @@ function getCallsite(frame: stackTrace.StackFrame) {
             parts.push(frame.getFunctionName());
         }
     }
-    return parts.map(sanitizeName).join('.');
+    return parts.join('.');
 }
 
 // Map all events to their properties