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 6 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
2 changes: 1 addition & 1 deletion extensions/ql-vscode/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@
"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)"
Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Sep 15, 2023

Choose a reason for hiding this comment

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

In the docs it says

Tag your custom telemetry setting with telemetry and usesOnlineServices if you have one.

But I couldn't quite work out what this means. I need to hunt down the general docs for these settings to see how to add tags.

Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg Sep 15, 2023

Choose a reason for hiding this comment

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

I don't know what that means either. Do you have a link for where that's said?

Copy link
Copy Markdown
Member

@koesie10 koesie10 Sep 18, 2023

Choose a reason for hiding this comment

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

I think this means that we need to add a tags property to our telemetry configuration options in the package.json. In this documentation for the configuration contribution points, they give this example:

{
  "contributes": {
    "configuration": {
      "title": "Git",
      "properties": {
        "git.alwaysSignOff": {
          "type": "boolean",
          "scope": "resource",
          "default": false,
          "description": "%config.alwaysSignOff%"
        },
        "git.ignoredRepositories": {
          "type": "array",
          "default": [],
          "scope": "window",
          "description": "%config.ignoredRepositories%"
        },
        "git.autofetch": {
          "type": ["boolean", "string"],
          "enum": [true, false, "all"],
          "scope": "resource",
          "markdownDescription": "%config.autofetch%",
          "default": false,
          "tags": ["usesOnlineServices"]
        }
      }
    }
  }
}

The tags property doesn't seem to be documented on that page though, so I'm not sure what other tags there are.

Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Sep 18, 2023

Choose a reason for hiding this comment

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

I don't know what that means either. Do you have a link for where that's said?

I read it at https://code.visualstudio.com/api/extension-guides/telemetry#dos-and-donts

In this documentation for the configuration contribution points, they give this example:

Thanks for finding that example. It's a shame there isn't more documentation on this but I think there's enough we can add the tags field. I believe it shouldn't cause any harm so we may as well add it.

},
"codeQL.telemetry.logTelemetry": {
"type": "boolean",
Expand Down
32 changes: 25 additions & 7 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 @@ -13,6 +13,7 @@ import {
LOG_TELEMETRY,
isIntegrationTestMode,
isCanary,
GLOBAL_TELEMETRY_LEVEL,
} from "../../config";
import * as appInsights from "applicationinsights";
import { extLogger } from "../logging/vscode";
Expand Down Expand Up @@ -59,8 +60,6 @@ export class ExtensionTelemetryListener
extends ConfigListener
implements AppTelemetry
{
static relevantSettings = [ENABLE_TELEMETRY, CANARY_FEATURES];

private reporter?: TelemetryReporter;

private cliVersionStr = NOT_SET_CLI_VERSION;
Expand Down Expand Up @@ -92,8 +91,9 @@ export class ExtensionTelemetryListener
e: ConfigurationChangeEvent,
): Promise<void> {
if (
e.affectsConfiguration("codeQL.telemetry.enableTelemetry") ||
e.affectsConfiguration("telemetry.enableTelemetry")
e.affectsConfiguration(ENABLE_TELEMETRY.qualifiedName) ||
e.affectsConfiguration(GLOBAL_ENABLE_TELEMETRY.qualifiedName) ||
e.affectsConfiguration(GLOBAL_TELEMETRY_LEVEL.qualifiedName)
) {
await this.initialize();
}
Expand All @@ -102,7 +102,7 @@ export class ExtensionTelemetryListener
// 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 +212,7 @@ export class ExtensionTelemetryListener
properties.stack = error.stack;
}

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

/**
Expand All @@ -224,7 +224,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() &&
isGlobalTelemetryEnabled() &&
// Avoid showing the dialog if we are in integration test mode.
!isIntegrationTestMode()
) {
Expand Down Expand Up @@ -297,3 +297,21 @@ export async function initializeTelemetry(
ctx.subscriptions.push(telemetryListener);
return telemetryListener;
}

function isGlobalTelemetryEnabled(): boolean {
Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Sep 15, 2023

Choose a reason for hiding this comment

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

The docs suggest not trying to implement this logic ourselves and instead using vscode.env.isTelemetryEnabled. I haven't yet looked into whether we can actually do that and it does what we want, or if we do indeed have to check both config settings.

Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg Sep 15, 2023

Choose a reason for hiding this comment

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

This method is complicated enough that it would be nice to use existing logic for it.

Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've done some testing of how isTelemetryEnabled behaves. The answer is:

  • It does indeed take into account both telemetry.enableTelemetry and telemetry.telemetryLevel
  • It only returns true if telemetry is FULLY enabled. So that means telemetryLevel is set to all, or enableTelemetry is set to true. If the level is set to error then isTelemetryEnabled will return false.

Maybe this is fine for us and the CodeQL extension doesn't need the granularity of only sending error reports and not other telemetry. We certainly aren't losing anything with this PR because we weren't checking the level at all before.

Are people happy with this limitation that if telemetryLevel is set to error then we won't send any telemetry?

// If "enableTelemetry" is set to false, no telemetry will be sent regardless of the value of "telemetryLevel"
Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Sep 15, 2023

Choose a reason for hiding this comment

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

When I have only telemetry.enableTelemetry set I get this popup:

Screenshot 2023-09-15 at 16 53 16

That's where I'm getting the logic that if telemetry.enableTelemetry is set to false then it'll ignore the value of telemetry.telemetryLevel.

const enableTelemetry: boolean | undefined =
GLOBAL_ENABLE_TELEMETRY.getValue();
if (enableTelemetry === false) {
return false;
}

// If a value for "telemetry.telemetryLevel" is provided, then use that
const telemetryLevel: string | undefined = GLOBAL_TELEMETRY_LEVEL.getValue();
if (telemetryLevel !== undefined) {
return telemetryLevel === "error" || telemetryLevel === "on";
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg Sep 15, 2023

Choose a reason for hiding this comment

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

These are the levels that I see:

Window_and_Settings_—_codeql-action

So, I think you want to change on to all.

Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Sep 18, 2023

Choose a reason for hiding this comment

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

You're right. I was originally reading https://code.visualstudio.com/updates/v1_61#_telemetry-settings but that's a changelog entry and is apparently not outdated. In my local vscode and some other docs I also see what you see.

}

// Otherwise fall back to the deprecated "telemetry.enableTelemetry" setting
return !!enableTelemetry;
}
20 changes: 12 additions & 8 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,26 +72,24 @@ 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,
);

const GLOBAL_TELEMETRY_SETTING = new Setting("telemetry");
export const GLOBAL_ENABLE_TELEMETRY = new Setting(
"enableTelemetry",
GLOBAL_TELEMETRY_SETTING,
);
export const GLOBAL_TELEMETRY_LEVEL = new Setting(
"telemetryLevel",
GLOBAL_TELEMETRY_SETTING,
);

// Distribution configuration
const DISTRIBUTION_SETTING = new Setting("cli", ROOT_SETTING);
Expand Down Expand Up @@ -475,6 +473,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 +483,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 +492,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