Use a CLI version changed listener for telemetry events

Set the CLI version in the telemetry listener whenever the version
changes.

A few things to note here:

1. In `CliServer::getVersion()`, avoid calling `supportsPerQueryEvalLog`
   directly. This avoids a recursive call to `CliServer::getVersion()`.
   Currently, it's always safe to do this, but I thought that it would
   be good to avoid recursion here in case we change things in the
   future.
2. Now, we are sending the CLI version with all telemetry events.

extensions/ql-vscode/src/cli.ts

@@ -155,6 +155,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
@@ -172,7 +174,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()`.
@@ -1373,15 +1377,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 await this._version;
+    return this._version;
+  }
+
+  public addVersionChangedListener(listener: VersionChangedListener) {
+    if (this._version) {
+      listener(this._version);
+    }
+    this._versionChangedListeners.push(listener);
   }
 
   private async refreshVersion() {

extensions/ql-vscode/src/common/vscode/commands.ts

@@ -75,7 +75,7 @@ export function registerCommandWithErrorHandling(
       return undefined;
     } finally {
       const executionTime = Date.now() - startTime;
-      void telemetryListener?.sendCommandUsage(commandId, executionTime, error);
+      telemetryListener?.sendCommandUsage(commandId, executionTime, error);
     }
   });
 }

extensions/ql-vscode/src/extension.ts

@@ -395,7 +395,9 @@ export async function activate(
     variantAnalysisViewSerializer.onExtensionLoaded(
       codeQlExtension.variantAnalysisManager,
     );
-    telemetryListener.cli = codeQlExtension.cliServer;
+    codeQlExtension.cliServer.addVersionChangedListener((ver) => {
+      telemetryListener.cliVersion = ver;
+    });
   }
 
   return codeQlExtension;

extensions/ql-vscode/src/telemetry.ts

@@ -19,7 +19,7 @@ import { extLogger } from "./common";
 import { UserCancellationException } from "./progress";
 import { showBinaryChoiceWithUrlDialog } from "./helpers";
 import { RedactableError } from "./pure/errors";
-import { CodeQLCliServer } from "./cli";
+import { SemVer } from "semver";
 
 // Key is injected at build time through the APP_INSIGHTS_KEY environment variable.
 const key = "REPLACE-APP-INSIGHTS-KEY";
@@ -52,12 +52,14 @@ 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 _cli?: CodeQLCliServer;
+  private cliVersionStr = NOT_SET_CLI_VERSION;
 
   constructor(
     private readonly id: string,
@@ -150,7 +152,7 @@ export class TelemetryListener extends ConfigListener {
     void this.reporter?.dispose();
   }
 
-  async sendCommandUsage(name: string, executionTime: number, error?: Error) {
+  sendCommandUsage(name: string, executionTime: number, error?: Error) {
     if (!this.reporter) {
       return;
     }
@@ -160,18 +162,13 @@ export class TelemetryListener extends ConfigListener {
       ? CommandCompletion.Cancelled
       : CommandCompletion.Failed;
 
-    const cliVersion = this._cli
-      ? (await this._cli.getVersion()).toString()
-      : // telemetry events that are sent before the cli is initialized will not have a version number
-        "not-set";
-
     this.reporter.sendTelemetryEvent(
       "command-usage",
       {
         name,
         status,
         isCanary: isCanary().toString(),
-        cliVersion,
+        cliVersion: this.cliVersionStr,
       },
       { executionTime },
     );
@@ -187,6 +184,7 @@ export class TelemetryListener extends ConfigListener {
       {
         name,
         isCanary: isCanary().toString(),
+        cliVersion: this.cliVersionStr,
       },
       {},
     );
@@ -202,6 +200,7 @@ export class TelemetryListener extends ConfigListener {
 
     const properties: { [key: string]: string } = {
       isCanary: isCanary().toString(),
+      cliVersion: this.cliVersionStr,
       message: error.redactedMessage,
       ...extraProperties,
     };
@@ -250,8 +249,8 @@ export class TelemetryListener extends ConfigListener {
     return this.reporter;
   }
 
-  set cli(cli: CodeQLCliServer) {
-    this._cli = cli;
+  set cliVersion(version: SemVer | undefined) {
+    this.cliVersionStr = version ? version.toString() : NOT_SET_CLI_VERSION;
   }
 
   private disposeReporter() {

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.
@@ -185,7 +186,7 @@ describe("telemetry reporting", () => {
   it("should send an event", async () => {
     await telemetryListener.initialize();
 
-    await telemetryListener.sendCommandUsage("command-id", 1234, undefined);
+    telemetryListener.sendCommandUsage("command-id", 1234, undefined);
 
     expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
       "command-usage",
@@ -204,7 +205,7 @@ describe("telemetry reporting", () => {
   it("should send a command usage event with an error", async () => {
     await telemetryListener.initialize();
 
-    await telemetryListener.sendCommandUsage(
+    telemetryListener.sendCommandUsage(
       "command-id",
       1234,
       new UserCancellationException(),
@@ -226,11 +227,9 @@ describe("telemetry reporting", () => {
 
   it("should send a command usage event with a cli version", async () => {
     await telemetryListener.initialize();
-    telemetryListener.cli = {
-      getVersion: () => Promise.resolve("1.2.3"),
-    } as any;
+    telemetryListener.cliVersion = new SemVer("1.2.3");
 
-    await telemetryListener.sendCommandUsage(
+    telemetryListener.sendCommandUsage(
       "command-id",
       1234,
       new UserCancellationException(),
@@ -248,14 +247,35 @@ describe("telemetry reporting", () => {
     );
 
     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);
 
-    await telemetryListener.sendCommandUsage("command-id", 1234, undefined);
-    await telemetryListener.sendCommandUsage("command-id", 1234, new Error());
+    telemetryListener.sendCommandUsage("command-id", 1234, undefined);
+    telemetryListener.sendCommandUsage("command-id", 1234, new Error());
 
     expect(sendTelemetryEventSpy).not.toBeCalled();
     expect(sendTelemetryExceptionSpy).not.toBeCalled();
@@ -266,7 +286,7 @@ describe("telemetry reporting", () => {
     await enableTelemetry("codeQL.telemetry", false);
     await enableTelemetry("codeQL.telemetry", true);
 
-    await telemetryListener.sendCommandUsage("command-id", 1234, undefined);
+    telemetryListener.sendCommandUsage("command-id", 1234, undefined);
 
     expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
       "command-usage",
@@ -431,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",
       },
       {},
     );
@@ -447,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",
       },
       {},
     );
@@ -465,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),
       },
       {},