Skip to content

Remove extension telemetry setting and use VS Code setting instead #3853

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
merged 4 commits into from
Dec 6, 2024
Merged

Remove extension telemetry setting and use VS Code setting instead #3853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52beaed
Remove extension telemetry setting and use VS Code setting instead
koesie10 Nov 6, 2024
036c60b
Add notification when enabling telemetry
koesie10 Dec 5, 2024
9506c17
Update CHANGELOG
koesie10 Dec 5, 2024
de9fd27
Remove telemetry step from E2E tests
koesie10 Dec 5, 2024
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
3 changes: 2 additions & 1 deletion extensions/ql-vscode/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

## [UNRELEASED]

- Add a palette command that allows importing all databases directly inside of a parent folder. [3797](https://github.com/github/vscode-codeql/pull/3797)
- Add a palette command that allows importing all databases directly inside of a parent folder. [#3797](https://github.com/github/vscode-codeql/pull/3797)
- Only use VS Code telemetry settings instead of using `codeQL.telemetry.enableTelemetry` [#3853](https://github.com/github/vscode-codeql/pull/3853)

## 1.16.1 - 6 November 2024

Expand Down
10 changes: 0 additions & 10 deletions extensions/ql-vscode/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,16 +497,6 @@
"title": "Telemetry",
"order": 11,
"properties": {
"codeQL.telemetry.enableTelemetry": {
"type": "boolean",
"default": false,
"scope": "application",
"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,
Expand Down
46 changes: 1 addition & 45 deletions extensions/ql-vscode/src/common/vscode/dialog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { env, Uri, window } from "vscode";
import { window } from "vscode";

/**
* Opens a modal dialog for the user to make a yes/no choice.
Expand Down Expand Up @@ -34,50 +34,6 @@ export async function showBinaryChoiceDialog(
return chosenItem?.title === yesItem.title;
}

/**
* Opens a modal dialog for the user to make a yes/no choice.
*
* @param message The message to show.
* @param modal If true (the default), show a modal dialog box, otherwise dialog is non-modal and can
* be closed even if the user does not make a choice.
*
* @return
* `true` if the user clicks 'Yes',
* `false` if the user clicks 'No' or cancels the dialog,
* `undefined` if the dialog is closed without the user making a choice.
*/
export async function showBinaryChoiceWithUrlDialog(
message: string,
url: string,
): Promise<boolean | undefined> {
const urlItem = { title: "More Information", isCloseAffordance: false };
const yesItem = { title: "Yes", isCloseAffordance: false };
const noItem = { title: "No", isCloseAffordance: true };
let chosenItem;

// Keep the dialog open as long as the user is clicking the 'more information' option.
// To prevent an infinite loop, if the user clicks 'more information' 5 times, close the dialog and return cancelled
let count = 0;
do {
chosenItem = await window.showInformationMessage(
message,
{ modal: true },
urlItem,
yesItem,
noItem,
);
if (chosenItem === urlItem) {
await env.openExternal(Uri.parse(url, true));
}
count++;
} while (chosenItem === urlItem && count < 5);

if (!chosenItem || chosenItem.title === urlItem.title) {
return undefined;
}
return chosenItem.title === yesItem.title;
}

/**
* Show an information message with a customisable action.
* @param message The message to show.
Expand Down
218 changes: 71 additions & 147 deletions extensions/ql-vscode/src/common/vscode/telemetry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,15 @@
import type {
Extension,
ExtensionContext,
ConfigurationChangeEvent,
} from "vscode";
import { ConfigurationTarget, env } from "vscode";
import type { Extension, ExtensionContext } from "vscode";
import { ConfigurationTarget, env, Uri, window } from "vscode";
import TelemetryReporter from "vscode-extension-telemetry";
import {
ConfigListener,
CANARY_FEATURES,
ENABLE_TELEMETRY,
LOG_TELEMETRY,
isIntegrationTestMode,
isCanary,
} from "../../config";
import { ENABLE_TELEMETRY, isCanary, LOG_TELEMETRY } from "../../config";
import type { TelemetryClient } from "applicationinsights";
import { extLogger } from "../logging/vscode";
import { UserCancellationException } from "./progress";
import { showBinaryChoiceWithUrlDialog } from "./dialog";
import type { RedactableError } from "../errors";
import type { SemVer } from "semver";
import type { AppTelemetry } from "../telemetry";
import type { EnvelopeTelemetry } from "applicationinsights/out/Declarations/Contracts";
import type { Disposable } from "../disposable-object";

// Key is injected at build time through the APP_INSIGHTS_KEY environment variable.
const key = "REPLACE-APP-INSIGHTS-KEY";
Expand Down Expand Up @@ -55,80 +44,25 @@ const baseDataPropertiesToRemove = [

const NOT_SET_CLI_VERSION = "not-set";

export class ExtensionTelemetryListener
extends ConfigListener
implements AppTelemetry
{
private reporter?: TelemetryReporter;
export class ExtensionTelemetryListener implements AppTelemetry, Disposable {
private readonly reporter: TelemetryReporter;

private cliVersionStr = NOT_SET_CLI_VERSION;

constructor(
private readonly id: string,
private readonly version: string,
private readonly key: string,
private readonly ctx: ExtensionContext,
) {
super();

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

/**
* This function handles changes to relevant configuration elements. There are 2 configuration
* ids that this function cares about:
*
* * `codeQL.telemetry.enableTelemetry`: If this one has changed, then we need to re-initialize
* the reporter and the reporter may wind up being removed.
* * `codeQL.canary`: A change here could possibly re-trigger a dialog popup.
*
* Note that the global telemetry setting also gate-keeps whether or not to send telemetry events
* to Application Insights. However, this gatekeeping happens inside of the vscode-extension-telemetry
* package. So, this does not need to be handled here.
*
* @param e the configuration change event
*/
async handleDidChangeConfiguration(
e: ConfigurationChangeEvent,
): Promise<void> {
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(CANARY_FEATURES.qualifiedName) &&
CANARY_FEATURES.getValue() &&
!ENABLE_TELEMETRY.getValue()
) {
await this.setTelemetryRequested(false);
await this.requestTelemetryPermission();
}
}

async initialize() {
await this.requestTelemetryPermission();

this.disposeReporter();

if (ENABLE_TELEMETRY.getValue<boolean>()) {
this.createReporter();
}
}

private createReporter() {
constructor(id: string, version: string, key: string) {
// We can always initialize this and send events using it because the vscode-extension-telemetry will check
// whether the `telemetry.telemetryLevel` setting is enabled.
this.reporter = new TelemetryReporter(
this.id,
this.version,
this.key,
id,
version,
key,
/* anonymize stack traces */ true,
);
this.push(this.reporter);

this.addTelemetryProcessor();
}

private addTelemetryProcessor() {
// The appInsightsClient field is private but we want to access it anyway
const client = this.reporter["appInsightsClient"] as TelemetryClient;
if (client) {
Expand All @@ -151,14 +85,10 @@ export class ExtensionTelemetryListener
}

dispose() {
super.dispose();
void this.reporter?.dispose();
void this.reporter.dispose();
}

sendCommandUsage(name: string, executionTime: number, error?: Error): void {
if (!this.reporter) {
return;
}
const status = !error
? CommandCompletion.Success
: error instanceof UserCancellationException
Expand All @@ -178,10 +108,6 @@ export class ExtensionTelemetryListener
}

sendUIInteraction(name: string): void {
if (!this.reporter) {
return;
}

this.reporter.sendTelemetryEvent(
"ui-interaction",
{
Expand All @@ -197,10 +123,6 @@ export class ExtensionTelemetryListener
error: RedactableError,
extraProperties?: { [key: string]: string },
): void {
if (!this.reporter) {
return;
}

const properties: { [key: string]: string } = {
isCanary: isCanary().toString(),
cliVersion: this.cliVersionStr,
Expand All @@ -215,10 +137,6 @@ export class ExtensionTelemetryListener
}

sendConfigInformation(config: Record<string, string>): void {
if (!this.reporter) {
return;
}

this.reporter.sendTelemetryEvent(
"config",
{
Expand All @@ -230,37 +148,6 @@ export class ExtensionTelemetryListener
);
}

/**
* Displays a popup asking the user if they want to enable telemetry
* for this extension.
*/
async requestTelemetryPermission() {
if (!this.wasTelemetryRequested()) {
// if global telemetry is disabled, avoid showing the dialog or making any changes
let result = undefined;
if (
env.isTelemetryEnabled &&
// Avoid showing the dialog if we are in integration test mode.
!isIntegrationTestMode()
) {
// Extension won't start until this completes.
result = await showBinaryChoiceWithUrlDialog(
"Does the CodeQL Extension by GitHub have your permission to collect usage data and metrics to help us improve CodeQL for VSCode?",
"https://codeql.github.com/docs/codeql-for-visual-studio-code/about-telemetry-in-codeql-for-visual-studio-code",
);
}
if (result !== undefined) {
await Promise.all([
this.setTelemetryRequested(true),
ENABLE_TELEMETRY.updateValue<boolean>(
result,
ConfigurationTarget.Global,
),
]);
}
}
}

/**
* Exposed for testing
*/
Expand All @@ -271,21 +158,45 @@ export class ExtensionTelemetryListener
set cliVersion(version: SemVer | undefined) {
this.cliVersionStr = version ? version.toString() : NOT_SET_CLI_VERSION;
}
}

private disposeReporter() {
if (this.reporter) {
void this.reporter.dispose();
this.reporter = undefined;
async function notifyTelemetryChange() {
const continueItem = { title: "Continue", isCloseAffordance: false };
const vsCodeTelemetryItem = {
title: "More Information about VS Code Telemetry",
isCloseAffordance: false,
};
const codeqlTelemetryItem = {
title: "More Information about CodeQL Telemetry",
isCloseAffordance: false,
};
let chosenItem;

do {
chosenItem = await window.showInformationMessage(
Copy link
Contributor

@robertbrignull robertbrignull Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's slightly annoying you get a "cancel" button but if you click it the dialog just comes back. Sadly I can't see a way to get rid of it and there's nothing sensible we can really do to "cancel" this update, so I agree I think we just leave it like this. Users will only see it once at least.

Screenshot 2024-12-05 at 16 41 03

Copy link
Member Author

@koesie10 koesie10 Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that this is quite annoying, but I also couldn't find a way to hide it. An alternative solution would be to keep the old setting for a few releases while showing this dialog, where the cancel button would simply keep using the old setting. However, we hope this scenario is quite rare and don't think it's worth the effort to have a "transition" like that which would require some additional work in a few releases.

"The CodeQL extension now follows VS Code's telemetry settings. VS Code telemetry is currently enabled. Learn how to update your telemetry settings by clicking the links below.",
{ modal: true },
continueItem,
vsCodeTelemetryItem,
codeqlTelemetryItem,
);
if (chosenItem === vsCodeTelemetryItem) {
await env.openExternal(
Uri.parse(
"https://code.visualstudio.com/docs/getstarted/telemetry",
true,
),
);
}
}

private wasTelemetryRequested(): boolean {
return !!this.ctx.globalState.get<boolean>("telemetry-request-viewed");
}

private async setTelemetryRequested(newValue: boolean): Promise<void> {
await this.ctx.globalState.update("telemetry-request-viewed", newValue);
}
if (chosenItem === codeqlTelemetryItem) {
await env.openExternal(
Uri.parse(
"https://docs.github.com/en/code-security/codeql-for-vs-code/using-the-advanced-functionality-of-the-codeql-for-vs-code-extension/telemetry-in-codeql-for-visual-studio-code",
true,
),
);
}
} while (chosenItem !== continueItem);
}

/**
Expand All @@ -301,15 +212,28 @@ export async function initializeTelemetry(
if (telemetryListener !== undefined) {
throw new Error("Telemetry is already initialized");
}

if (ENABLE_TELEMETRY.getValue<boolean | undefined>() === false) {
if (env.isTelemetryEnabled) {
// Await this so that the user is notified before any telemetry is sent
await notifyTelemetryChange();
}

// Remove the deprecated telemetry setting
ENABLE_TELEMETRY.updateValue(undefined, ConfigurationTarget.Global);
ENABLE_TELEMETRY.updateValue(undefined, ConfigurationTarget.Workspace);
ENABLE_TELEMETRY.updateValue(
undefined,
ConfigurationTarget.WorkspaceFolder,
);
}

telemetryListener = new ExtensionTelemetryListener(
extension.id,
extension.packageJSON.version,
key,
ctx,
);
// do not await initialization, since doing so will sometimes cause a modal popup.
// 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;
}
2 changes: 2 additions & 0 deletions extensions/ql-vscode/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ const ROOT_SETTING = new Setting("codeQL");
const TELEMETRY_SETTING = new Setting("telemetry", ROOT_SETTING);

export const LOG_TELEMETRY = new Setting("logTelemetry", TELEMETRY_SETTING);

// Legacy setting that is no longer used, but is used for showing a message when the user upgrades.
export const ENABLE_TELEMETRY = new Setting(
"enableTelemetry",
TELEMETRY_SETTING,
Expand Down
Loading
Loading