Merge pull request #2307 from github/aeisenberg/cli-version-telemetry

Add the CLI version to telemetry events

Author: aeisenberg

extensions/ql-vscode/src/cli.ts

@@ -171,6 +171,8 @@ export type OnLineCallback = (
   line: string,
 ) => Promise<string | undefined> | string | undefined;
 
+type VersionChangedListener = (newVersion: SemVer | undefined) => void;
+
 /**
  * This class manages a cli server started by `codeql execute cli-server` to
  * run commands without the overhead of starting a new java
@@ -188,7 +190,9 @@ export class CodeQLCliServer implements Disposable {
   nullBuffer: Buffer;
 
   /** Version of current cli, lazily computed by the `getVersion()` method */
-  private _version: Promise<SemVer> | undefined;
+  private _version: SemVer | undefined;
+
+  private _versionChangedListeners: VersionChangedListener[] = [];
 
   /**
    * The languages supported by the current version of the CLI, computed by `getSupportedLanguages()`.
@@ -1417,15 +1421,36 @@ export class CodeQLCliServer implements Disposable {
 
   public async getVersion() {
     if (!this._version) {
-      this._version = this.refreshVersion();
-      // this._version is only undefined upon config change, so we reset CLI-based context key only when necessary.
-      await this.app.commands.execute(
-        "setContext",
-        "codeql.supportsEvalLog",
-        await this.cliConstraints.supportsPerQueryEvalLog(),
-      );
+      try {
+        const newVersion = await this.refreshVersion();
+        this._version = newVersion;
+        this._versionChangedListeners.forEach((listener) =>
+          listener(newVersion),
+        );
+
+        // this._version is only undefined upon config change, so we reset CLI-based context key only when necessary.
+        await this.app.commands.execute(
+          "setContext",
+          "codeql.supportsEvalLog",
+          newVersion.compare(
+            CliVersionConstraint.CLI_VERSION_WITH_PER_QUERY_EVAL_LOG,
+          ) >= 0,
+        );
+      } catch (e) {
+        this._versionChangedListeners.forEach((listener) =>
+          listener(undefined),
+        );
+        throw e;
+      }
+    }
+    return this._version;
+  }
+
+  public addVersionChangedListener(listener: VersionChangedListener) {
+    if (this._version) {
+      listener(this._version);
     }
-    return await this._version;
+    this._versionChangedListeners.push(listener);
   }
 
   private async refreshVersion() {

extensions/ql-vscode/src/extension.ts

@@ -317,7 +317,7 @@ export async function activate(
 
   const distributionConfigListener = new DistributionConfigListener();
   await initializeLogging(ctx);
-  await initializeTelemetry(extension, ctx);
+  const telemetryListener = await initializeTelemetry(extension, ctx);
   addUnhandledRejectionListener();
   install();
 
@@ -406,6 +406,9 @@ export async function activate(
     variantAnalysisViewSerializer.onExtensionLoaded(
       codeQlExtension.variantAnalysisManager,
     );
+    codeQlExtension.cliServer.addVersionChangedListener((ver) => {
+      telemetryListener.cliVersion = ver;
+    });
   }
 
   return codeQlExtension;

extensions/ql-vscode/src/telemetry.ts

@@ -19,6 +19,7 @@ import { extLogger } from "./common";
 import { UserCancellationException } from "./progress";
 import { showBinaryChoiceWithUrlDialog } from "./helpers";
 import { RedactableError } from "./pure/errors";
+import { SemVer } from "semver";
 
 // Key is injected at build time through the APP_INSIGHTS_KEY environment variable.
 const key = "REPLACE-APP-INSIGHTS-KEY";
@@ -51,11 +52,15 @@ const baseDataPropertiesToRemove = [
   "common.vscodesessionid",
 ];
 
+const NOT_SET_CLI_VERSION = "not-set";
+
 export class TelemetryListener extends ConfigListener {
   static relevantSettings = [ENABLE_TELEMETRY, CANARY_FEATURES];
 
   private reporter?: TelemetryReporter;
 
+  private cliVersionStr = NOT_SET_CLI_VERSION;
+
   constructor(
     private readonly id: string,
     private readonly version: string,
@@ -163,6 +168,7 @@ export class TelemetryListener extends ConfigListener {
         name,
         status,
         isCanary: isCanary().toString(),
+        cliVersion: this.cliVersionStr,
       },
       { executionTime },
     );
@@ -178,6 +184,7 @@ export class TelemetryListener extends ConfigListener {
       {
         name,
         isCanary: isCanary().toString(),
+        cliVersion: this.cliVersionStr,
       },
       {},
     );
@@ -193,6 +200,7 @@ export class TelemetryListener extends ConfigListener {
 
     const properties: { [key: string]: string } = {
       isCanary: isCanary().toString(),
+      cliVersion: this.cliVersionStr,
       message: error.redactedMessage,
       ...extraProperties,
     };
@@ -241,6 +249,10 @@ export class TelemetryListener extends ConfigListener {
     return this.reporter;
   }
 
+  set cliVersion(version: SemVer | undefined) {
+    this.cliVersionStr = version ? version.toString() : NOT_SET_CLI_VERSION;
+  }
+
   private disposeReporter() {
     if (this.reporter) {
       void this.reporter.dispose();
@@ -265,7 +277,7 @@ export let telemetryListener: TelemetryListener | undefined;
 export async function initializeTelemetry(
   extension: Extension<any>,
   ctx: ExtensionContext,
-): Promise<void> {
+): Promise<TelemetryListener> {
   if (telemetryListener !== undefined) {
     throw new Error("Telemetry is already initialized");
   }
@@ -279,4 +291,5 @@ export async function initializeTelemetry(
   // this is a particular problem during integration tests, which will hang if a modal popup is displayed.
   void telemetryListener.initialize();
   ctx.subscriptions.push(telemetryListener);
+  return telemetryListener;
 }

extensions/ql-vscode/test/vscode-tests/no-workspace/telemetry.test.ts

@@ -14,6 +14,7 @@ import { ENABLE_TELEMETRY } from "../../../src/config";
 import { createMockExtensionContext } from "./index";
 import { vscodeGetConfigurationMock } from "../test-config";
 import { redactableError } from "../../../src/pure/errors";
+import { SemVer } from "semver";
 
 // setting preferences can trigger lots of background activity
 // so need to bump up the timeout of this test.
@@ -193,6 +194,7 @@ describe("telemetry reporting", () => {
         name: "command-id",
         status: "Success",
         isCanary,
+        cliVersion: "not-set",
       },
       { executionTime: 1234 },
     );
@@ -215,13 +217,59 @@ describe("telemetry reporting", () => {
         name: "command-id",
         status: "Cancelled",
         isCanary,
+        cliVersion: "not-set",
       },
       { executionTime: 1234 },
     );
 
     expect(sendTelemetryExceptionSpy).not.toBeCalled();
   });
 
+  it("should send a command usage event with a cli version", async () => {
+    await telemetryListener.initialize();
+    telemetryListener.cliVersion = new SemVer("1.2.3");
+
+    telemetryListener.sendCommandUsage(
+      "command-id",
+      1234,
+      new UserCancellationException(),
+    );
+
+    expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
+      "command-usage",
+      {
+        name: "command-id",
+        status: "Cancelled",
+        isCanary,
+        cliVersion: "1.2.3",
+      },
+      { executionTime: 1234 },
+    );
+
+    expect(sendTelemetryExceptionSpy).not.toBeCalled();
+
+    // Verify that if the cli version is not set, then the telemetry falls back to "not-set"
+    sendTelemetryEventSpy.mockClear();
+    telemetryListener.cliVersion = undefined;
+
+    telemetryListener.sendCommandUsage(
+      "command-id",
+      5678,
+      new UserCancellationException(),
+    );
+
+    expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
+      "command-usage",
+      {
+        name: "command-id",
+        status: "Cancelled",
+        isCanary,
+        cliVersion: "not-set",
+      },
+      { executionTime: 5678 },
+    );
+  });
+
   it("should avoid sending an event when telemetry is disabled", async () => {
     await telemetryListener.initialize();
     await enableTelemetry("codeQL.telemetry", false);
@@ -246,6 +294,7 @@ describe("telemetry reporting", () => {
         name: "command-id",
         status: "Success",
         isCanary,
+        cliVersion: "not-set",
       },
       { executionTime: 1234 },
     );
@@ -402,6 +451,24 @@ describe("telemetry reporting", () => {
       {
         name: "test",
         isCanary,
+        cliVersion: "not-set",
+      },
+      {},
+    );
+  });
+
+  it("should send a ui-interaction telementry event with a cli version", async () => {
+    await telemetryListener.initialize();
+
+    telemetryListener.cliVersion = new SemVer("1.2.3");
+    telemetryListener.sendUIInteraction("test");
+
+    expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
+      "ui-interaction",
+      {
+        name: "test",
+        isCanary,
+        cliVersion: "1.2.3",
       },
       {},
     );
@@ -418,6 +485,25 @@ describe("telemetry reporting", () => {
         message: "test",
         isCanary,
         stack: expect.any(String),
+        cliVersion: "not-set",
+      },
+      {},
+    );
+  });
+
+  it("should send an error telementry event with a cli version", async () => {
+    await telemetryListener.initialize();
+    telemetryListener.cliVersion = new SemVer("1.2.3");
+
+    telemetryListener.sendError(redactableError`test`);
+
+    expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
+      "error",
+      {
+        message: "test",
+        isCanary,
+        stack: expect.any(String),
+        cliVersion: "1.2.3",
       },
       {},
     );
@@ -436,6 +522,7 @@ describe("telemetry reporting", () => {
         message:
           "test message with secret information: [REDACTED] and more [REDACTED] parts",
         isCanary,
+        cliVersion: "not-set",
         stack: expect.any(String),
       },
       {},