Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
robertbrignull merged 17 commits into from
Oct 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
90093fb
Rearrange settings so the telemetry settings are clearer
robertbrignull Sep 15, 2023
9c76ba3
Check both telemetry.telemetryLevel and telemetry.enableTelemetry
robertbrignull Sep 15, 2023
c7bd343
Use sendTelemetryErrorEvent when sending errors
robertbrignull Sep 15, 2023
c7a9337
Use .qualifiedName instead of writing out manually
robertbrignull Sep 15, 2023
f3e72a0
Delete unused relevantSettings field
robertbrignull Sep 15, 2023
a506276
Update custom telemetry setting description
robertbrignull Sep 15, 2023
af5547f
Use correct values of telemtetry level
robertbrignull Sep 15, 2023
5fc9a73
Add telemtry tag to settings
robertbrignull Sep 18, 2023
9ec6590
Use env.isTelemetryEnabled instead of checking configuration values o…
robertbrignull Sep 18, 2023
d403361
Use onDidChangeTelemetryEnabled instead of listening for config value…
robertbrignull Sep 18, 2023
177c770
Delete config values that we on longer read directly
robertbrignull Sep 18, 2023
48399a9
Merge branch 'main' into robertbrignull/telemetry
robertbrignull Sep 19, 2023
aad1fee
Stop mocking sendTelemetryException because we don't use that anywher…
robertbrignull Sep 19, 2023
ca96cdf
Mock sendTelemetryErrorEvent and check appropriately
robertbrignull Sep 19, 2023
4227ff6
Mock env.itTelemetryEnable
robertbrignull Sep 19, 2023
d499ff3
Merge branch 'main' into robertbrignull/telemetry
robertbrignull Oct 5, 2023
2b0bd84
Place setup before initialising listener
robertbrignull Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions extensions/ql-vscode/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
]
}
}
}
Expand Down
19 changes: 9 additions & 10 deletions extensions/ql-vscode/src/common/vscode/telemetry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -72,6 +70,10 @@ export class ExtensionTelemetryListener
private readonly ctx: ExtensionContext,
) {
super();

env.onDidChangeTelemetryEnabled(async () => {
await this.initialize();
});
}

/**
Expand All @@ -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()
) {
Expand Down Expand Up @@ -212,7 +211,7 @@ export class ExtensionTelemetryListener
properties.stack = error.stack;
}

this.reporter.sendTelemetryEvent("error", properties, {});
this.reporter.sendTelemetryErrorEvent("error", properties, {});
}

/**
Expand All @@ -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()
) {
Expand Down
20 changes: 7 additions & 13 deletions extensions/ql-vscode/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand All @@ -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.
*/
Expand All @@ -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.
*/
Expand Down
47 changes: 33 additions & 14 deletions extensions/ql-vscode/test/vscode-tests/no-workspace/telemetry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
workspace,
ConfigurationTarget,
window,
env,
} from "vscode";
import {
ExtensionTelemetryListener,
Expand All @@ -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
>;
Expand All @@ -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")
Expand All @@ -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);

Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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
});
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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();
Expand All @@ -268,6 +277,7 @@ describe("telemetry reporting", () => {
},
{ executionTime: 5678 },
);
expect(sendTelemetryErrorEventSpy).not.toBeCalled();
});

it("should avoid sending an event when telemetry is disabled", async () => {
Expand All @@ -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 () => {
Expand All @@ -298,6 +308,7 @@ describe("telemetry reporting", () => {
},
{ executionTime: 1234 },
);
expect(sendTelemetryErrorEventSpy).not.toBeCalled();
});

it("should filter undesired properties from telemetry payload", async () => {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -455,6 +469,7 @@ describe("telemetry reporting", () => {
},
{},
);
expect(sendTelemetryErrorEventSpy).not.toBeCalled();
});

it("should send a ui-interaction telementry event with a cli version", async () => {
Expand All @@ -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",
Expand All @@ -497,7 +514,8 @@ describe("telemetry reporting", () => {

telemetryListener.sendError(redactableError`test`);

expect(sendTelemetryEventSpy).toHaveBeenCalledWith(
expect(sendTelemetryEventSpy).not.toBeCalled();
expect(sendTelemetryErrorEventSpy).toHaveBeenCalledWith(
"error",
{
message: "test",
Expand All @@ -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:
Expand Down