Merge pull request #2824 from github/robertbrignull/telemetry

Respect "telemetry.telemetryLevel" as well as "telemetry.enableTelemetry"

Author: robertbrignull

extensions/ql-vscode/package.json

@@ -446,13 +446,20 @@
             "type": "boolean",
             "default": false,
             "scope": "application",
-            "markdownDescription": "Specifies whether to send CodeQL usage telemetry. This setting AND the global `#telemetry.enableTelemetry#` setting must be checked for telemetry to be sent to GitHub. For more information, see the [telemetry documentation](https://codeql.github.com/docs/codeql-for-visual-studio-code/about-telemetry-in-codeql-for-visual-studio-code)"
+            "markdownDescription": "Specifies whether to send CodeQL usage telemetry. This setting AND the one of the global telemetry settings (`#telemetry.enableTelemetry#` or `#telemetry.telemetryLevel#`) must be enabled for telemetry to be sent to GitHub. For more information, see the [telemetry documentation](https://codeql.github.com/docs/codeql-for-visual-studio-code/about-telemetry-in-codeql-for-visual-studio-code)",
+            "tags": [
+              "telemetry",
+              "usesOnlineServices"
+            ]
           },
           "codeQL.telemetry.logTelemetry": {
             "type": "boolean",
             "default": false,
             "scope": "application",
-            "description": "Specifies whether or not to write telemetry events to the extension log."
+            "description": "Specifies whether or not to write telemetry events to the extension log.",
+            "tags": [
+              "telemetry"
+            ]
           }
         }
       }

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

@@ -3,13 +3,13 @@ import {
   Extension,
   ExtensionContext,
   ConfigurationChangeEvent,
+  env,
 } from "vscode";
 import TelemetryReporter from "vscode-extension-telemetry";
 import {
   ConfigListener,
   CANARY_FEATURES,
   ENABLE_TELEMETRY,
-  GLOBAL_ENABLE_TELEMETRY,
   LOG_TELEMETRY,
   isIntegrationTestMode,
   isCanary,
@@ -59,8 +59,6 @@ export class ExtensionTelemetryListener
   extends ConfigListener
   implements AppTelemetry
 {
-  static relevantSettings = [ENABLE_TELEMETRY, CANARY_FEATURES];
-
   private reporter?: TelemetryReporter;
 
   private cliVersionStr = NOT_SET_CLI_VERSION;
@@ -72,6 +70,10 @@ export class ExtensionTelemetryListener
     private readonly ctx: ExtensionContext,
   ) {
     super();
+
+    env.onDidChangeTelemetryEnabled(async () => {
+      await this.initialize();
+    });
   }
 
   /**
@@ -91,18 +93,15 @@ export class ExtensionTelemetryListener
   async handleDidChangeConfiguration(
     e: ConfigurationChangeEvent,
   ): Promise<void> {
-    if (
-      e.affectsConfiguration("codeQL.telemetry.enableTelemetry") ||
-      e.affectsConfiguration("telemetry.enableTelemetry")
-    ) {
+    if (e.affectsConfiguration(ENABLE_TELEMETRY.qualifiedName)) {
       await this.initialize();
     }
 
     // Re-request telemetry so that users can see the dialog again.
     // Re-request if codeQL.canary is being set to `true` and telemetry
     // is not currently enabled.
     if (
-      e.affectsConfiguration("codeQL.canary") &&
+      e.affectsConfiguration(CANARY_FEATURES.qualifiedName) &&
       CANARY_FEATURES.getValue() &&
       !ENABLE_TELEMETRY.getValue()
     ) {
@@ -212,7 +211,7 @@ export class ExtensionTelemetryListener
       properties.stack = error.stack;
     }
 
-    this.reporter.sendTelemetryEvent("error", properties, {});
+    this.reporter.sendTelemetryErrorEvent("error", properties, {});
   }
 
   /**
@@ -224,7 +223,7 @@ export class ExtensionTelemetryListener
       // if global telemetry is disabled, avoid showing the dialog or making any changes
       let result = undefined;
       if (
-        GLOBAL_ENABLE_TELEMETRY.getValue() &&
+        env.isTelemetryEnabled &&
         // Avoid showing the dialog if we are in integration test mode.
         !isIntegrationTestMode()
       ) {

extensions/ql-vscode/src/config.ts

@@ -72,27 +72,15 @@ export const VSCODE_SAVE_BEFORE_START_SETTING = new Setting(
 
 const ROOT_SETTING = new Setting("codeQL");
 
-// Global configuration
+// Telemetry configuration
 const TELEMETRY_SETTING = new Setting("telemetry", ROOT_SETTING);
-const AST_VIEWER_SETTING = new Setting("astViewer", ROOT_SETTING);
-const CONTEXTUAL_QUERIES_SETTINGS = new Setting(
-  "contextualQueries",
-  ROOT_SETTING,
-);
-const GLOBAL_TELEMETRY_SETTING = new Setting("telemetry");
-const LOG_INSIGHTS_SETTING = new Setting("logInsights", ROOT_SETTING);
 
 export const LOG_TELEMETRY = new Setting("logTelemetry", TELEMETRY_SETTING);
 export const ENABLE_TELEMETRY = new Setting(
   "enableTelemetry",
   TELEMETRY_SETTING,
 );
 
-export const GLOBAL_ENABLE_TELEMETRY = new Setting(
-  "enableTelemetry",
-  GLOBAL_TELEMETRY_SETTING,
-);
-
 // Distribution configuration
 const DISTRIBUTION_SETTING = new Setting("cli", ROOT_SETTING);
 export const CUSTOM_CODEQL_PATH_SETTING = new Setting(
@@ -475,6 +463,7 @@ export function allowCanaryQueryServer() {
   return value === undefined ? true : !!value;
 }
 
+const LOG_INSIGHTS_SETTING = new Setting("logInsights", ROOT_SETTING);
 export const JOIN_ORDER_WARNING_THRESHOLD = new Setting(
   "joinOrderWarningThreshold",
   LOG_INSIGHTS_SETTING,
@@ -484,6 +473,7 @@ export function joinOrderWarningThreshold(): number {
   return JOIN_ORDER_WARNING_THRESHOLD.getValue<number>();
 }
 
+const AST_VIEWER_SETTING = new Setting("astViewer", ROOT_SETTING);
 /**
  * Hidden setting: Avoids caching in the AST viewer if the user is also a canary user.
  */
@@ -492,6 +482,10 @@ export const NO_CACHE_AST_VIEWER = new Setting(
   AST_VIEWER_SETTING,
 );
 
+const CONTEXTUAL_QUERIES_SETTINGS = new Setting(
+  "contextualQueries",
+  ROOT_SETTING,
+);
 /**
  * Hidden setting: Avoids caching in jump to def and find refs contextual queries if the user is also a canary user.
  */

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

@@ -4,6 +4,7 @@ import {
   workspace,
   ConfigurationTarget,
   window,
+  env,
 } from "vscode";
 import {
   ExtensionTelemetryListener,
@@ -30,13 +31,18 @@ describe("telemetry reporting", () => {
   let sendTelemetryEventSpy: jest.SpiedFunction<
     typeof TelemetryReporter.prototype.sendTelemetryEvent
   >;
-  let sendTelemetryExceptionSpy: jest.SpiedFunction<
-    typeof TelemetryReporter.prototype.sendTelemetryException
+  let sendTelemetryErrorEventSpy: jest.SpiedFunction<
+    typeof TelemetryReporter.prototype.sendTelemetryErrorEvent
   >;
   let disposeSpy: jest.SpiedFunction<
     typeof TelemetryReporter.prototype.dispose
   >;
 
+  let isTelemetryEnabledSpy: jest.SpyInstance<
+    typeof env.isTelemetryEnabled,
+    []
+  >;
+
   let showInformationMessageSpy: jest.SpiedFunction<
     typeof window.showInformationMessage
   >;
@@ -56,8 +62,8 @@ describe("telemetry reporting", () => {
       sendTelemetryEventSpy = jest
         .spyOn(TelemetryReporter.prototype, "sendTelemetryEvent")
         .mockReturnValue(undefined);
-      sendTelemetryExceptionSpy = jest
-        .spyOn(TelemetryReporter.prototype, "sendTelemetryException")
+      sendTelemetryErrorEventSpy = jest
+        .spyOn(TelemetryReporter.prototype, "sendTelemetryErrorEvent")
         .mockReturnValue(undefined);
       disposeSpy = jest
         .spyOn(TelemetryReporter.prototype, "dispose")
@@ -78,6 +84,9 @@ describe("telemetry reporting", () => {
         .get<boolean>("codeQL.canary")).toString();
 
       // each test will default to telemetry being enabled
+      isTelemetryEnabledSpy = jest
+        .spyOn(env, "isTelemetryEnabled", "get")
+        .mockReturnValue(true);
       await enableTelemetry("telemetry", true);
       await enableTelemetry("codeQL.telemetry", true);
 
@@ -116,6 +125,7 @@ describe("telemetry reporting", () => {
   });
 
   it("should initialize telemetry when global option disabled", async () => {
+    isTelemetryEnabledSpy.mockReturnValue(false);
     await enableTelemetry("telemetry", false);
     await telemetryListener.initialize();
     expect(telemetryListener._reporter).toBeDefined();
@@ -133,6 +143,7 @@ describe("telemetry reporting", () => {
 
   it("should not initialize telemetry when both options disabled", async () => {
     await enableTelemetry("codeQL.telemetry", false);
+    isTelemetryEnabledSpy.mockReturnValue(false);
     await enableTelemetry("telemetry", false);
     await telemetryListener.initialize();
     expect(telemetryListener._reporter).toBeUndefined();
@@ -179,6 +190,7 @@ describe("telemetry reporting", () => {
     const reporter: any = telemetryListener._reporter;
     expect(reporter.userOptIn).toBe(true); // enabled
 
+    isTelemetryEnabledSpy.mockReturnValue(false);
     await enableTelemetry("telemetry", false);
     expect(reporter.userOptIn).toBe(false); // disabled
   });
@@ -198,8 +210,7 @@ describe("telemetry reporting", () => {
       },
       { executionTime: 1234 },
     );
-
-    expect(sendTelemetryExceptionSpy).not.toBeCalled();
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
   });
 
   it("should send a command usage event with an error", async () => {
@@ -221,8 +232,7 @@ describe("telemetry reporting", () => {
       },
       { executionTime: 1234 },
     );
-
-    expect(sendTelemetryExceptionSpy).not.toBeCalled();
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
   });
 
   it("should send a command usage event with a cli version", async () => {
@@ -245,8 +255,7 @@ describe("telemetry reporting", () => {
       },
       { executionTime: 1234 },
     );
-
-    expect(sendTelemetryExceptionSpy).not.toBeCalled();
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
 
     // Verify that if the cli version is not set, then the telemetry falls back to "not-set"
     sendTelemetryEventSpy.mockClear();
@@ -268,6 +277,7 @@ describe("telemetry reporting", () => {
       },
       { executionTime: 5678 },
     );
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
   });
 
   it("should avoid sending an event when telemetry is disabled", async () => {
@@ -278,7 +288,7 @@ describe("telemetry reporting", () => {
     telemetryListener.sendCommandUsage("command-id", 1234, new Error());
 
     expect(sendTelemetryEventSpy).not.toBeCalled();
-    expect(sendTelemetryExceptionSpy).not.toBeCalled();
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
   });
 
   it("should send an event when telemetry is re-enabled", async () => {
@@ -298,6 +308,7 @@ describe("telemetry reporting", () => {
       },
       { executionTime: 1234 },
     );
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
   });
 
   it("should filter undesired properties from telemetry payload", async () => {
@@ -345,6 +356,8 @@ describe("telemetry reporting", () => {
       resolveArg(3 /* "yes" item */),
     );
     await ctx.globalState.update("telemetry-request-viewed", false);
+    expect(env.isTelemetryEnabled).toBe(true);
+
     await enableTelemetry("codeQL.telemetry", false);
 
     await telemetryListener.initialize();
@@ -411,6 +424,7 @@ describe("telemetry reporting", () => {
     // If the user ever turns global telemetry back on, then we can
     // show the dialog.
 
+    isTelemetryEnabledSpy.mockReturnValue(false);
     await enableTelemetry("telemetry", false);
     await ctx.globalState.update("telemetry-request-viewed", false);
 
@@ -455,6 +469,7 @@ describe("telemetry reporting", () => {
       },
       {},
     );
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
   });
 
   it("should send a ui-interaction telementry event with a cli version", async () => {
@@ -472,14 +487,16 @@ describe("telemetry reporting", () => {
       },
       {},
     );
+    expect(sendTelemetryErrorEventSpy).not.toBeCalled();
   });
 
   it("should send an error telementry event", async () => {
     await telemetryListener.initialize();
 
     telemetryListener.sendError(redactableError`test`);
 
-    expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
+    expect(sendTelemetryEventSpy).not.toBeCalled();
+    expect(sendTelemetryErrorEventSpy).toHaveBeenCalledWith(
       "error",
       {
         message: "test",
@@ -497,7 +514,8 @@ describe("telemetry reporting", () => {
 
     telemetryListener.sendError(redactableError`test`);
 
-    expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
+    expect(sendTelemetryEventSpy).not.toBeCalled();
+    expect(sendTelemetryErrorEventSpy).toHaveBeenCalledWith(
       "error",
       {
         message: "test",
@@ -516,7 +534,8 @@ describe("telemetry reporting", () => {
       redactableError`test message with secret information: ${42} and more ${"secret"} parts`,
     );
 
-    expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
+    expect(sendTelemetryEventSpy).not.toBeCalled();
+    expect(sendTelemetryErrorEventSpy).toHaveBeenCalledWith(
       "error",
       {
         message: