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

robertbrignull · 2023-09-15T15:49:23Z

This PR tries to make our telemetry better behaved and to fully support the telemetry.telemetryLevel setting, as well as the legacy telemetry.enableTelemetry setting.

Unfortunately it's a bit hard to work out the intended semantics of the settings. I've been using the following as reference:

I think we were already partially supporting telemetry.telemetryLevel because we're using the vscode-extension-telemetry package. This means it'll only send telemetry when the global settings are enabled. I think we weren't correctly supporting telemetryLevel: "error" though because we weren't calling sendTelemetryErrorEvent for errors and we were instead using sendTelemetryEvent for everything.

We also weren't respecting telemetry.telemetryLevel in requestTelemetryPermission when deciding whether or not to prompt the user to enable telemtry for our extension. If the user has set telemetryLevel: "off" we should now no longer prompt the user.

I also clear up a couple of small unrelated things in the telemetry.ts file that I noticed while doing this.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

robertbrignull · 2023-09-15T15:50:19Z

I've tried to test this by building the extension and installing it locally, but annoying my telemetry events don't seem to be showing up on the other side. I'll have another go at testing it next week.

robertbrignull · 2023-09-15T15:51:50Z

extensions/ql-vscode/package.json

@@ -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)"


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.

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

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.

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.

robertbrignull · 2023-09-15T15:52:54Z

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

@@ -297,3 +297,21 @@ export async function initializeTelemetry(
  ctx.subscriptions.push(telemetryListener);
  return telemetryListener;
 }
+
+function isGlobalTelemetryEnabled(): boolean {


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.

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

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?

robertbrignull · 2023-09-15T15:54:21Z

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

@@ -297,3 +297,21 @@ export async function initializeTelemetry(
  ctx.subscriptions.push(telemetryListener);
  return telemetryListener;
 }
+
+function isGlobalTelemetryEnabled(): boolean {
+  // If "enableTelemetry" is set to false, no telemetry will be sent regardless of the value of "telemetryLevel"


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

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.

aeisenberg · 2023-09-15T16:50:35Z

extensions/ql-vscode/package.json

@@ -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)"


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

aeisenberg · 2023-09-15T19:34:37Z

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

+  // 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";


These are the levels that I see:

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

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.

aeisenberg · 2023-09-15T19:35:50Z

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

@@ -297,3 +297,21 @@ export async function initializeTelemetry(
  ctx.subscriptions.push(telemetryListener);
  return telemetryListener;
 }
+
+function isGlobalTelemetryEnabled(): boolean {


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

…urselves

… changes

aeisenberg

This is a lot simpler (though still not quite simple). Nice.

…e in the program

robertbrignull · 2023-09-19T16:27:45Z

Fixing the tests was a little bit involved because:

We now call sendTelemetryErrorEvent as well as sendTelemetryEvent
We now need to mock env.isTelemetryEnabled as well as setting config values

robertbrignull · 2023-10-05T14:32:41Z

@aeisenberg, are you still ok with this PR or would you like to take another look?

All the changes since you last approved have been to test files. My plan is I'll merge this on Monday unless I see/hear reasons not to.

robertbrignull · 2023-10-09T10:07:17Z

Also see the update to the docs: github/codeql#14408

aeisenberg · 2023-10-10T16:59:16Z

@aeisenberg, are you still ok with this PR or would you like to take another look?

Sorry...I missed this. This looks fine to me.

robertbrignull added 6 commits September 15, 2023 15:01

Rearrange settings so the telemetry settings are clearer

90093fb

Check both telemetry.telemetryLevel and telemetry.enableTelemetry

9c76ba3

Use sendTelemetryErrorEvent when sending errors

c7bd343

Use .qualifiedName instead of writing out manually

c7a9337

Delete unused relevantSettings field

f3e72a0

Update custom telemetry setting description

a506276

robertbrignull requested a review from a team September 15, 2023 15:49

robertbrignull requested a review from a team as a code owner September 15, 2023 15:49

robertbrignull commented Sep 15, 2023

View reviewed changes

aeisenberg reviewed Sep 15, 2023

View reviewed changes

robertbrignull added 5 commits September 18, 2023 11:14

Use correct values of telemtetry level

af5547f

Add telemtry tag to settings

5fc9a73

Use env.isTelemetryEnabled instead of checking configuration values o…

9ec6590

…urselves

Use onDidChangeTelemetryEnabled instead of listening for config value…

d403361

… changes

Delete config values that we on longer read directly

177c770

aeisenberg approved these changes Sep 18, 2023

View reviewed changes

robertbrignull added 4 commits September 19, 2023 16:40

Merge branch 'main' into robertbrignull/telemetry

48399a9

Stop mocking sendTelemetryException because we don't use that anywher…

aad1fee

…e in the program

Mock sendTelemetryErrorEvent and check appropriately

ca96cdf

Mock env.itTelemetryEnable

4227ff6

robertbrignull added 2 commits October 5, 2023 15:19

Merge branch 'main' into robertbrignull/telemetry

d499ff3

Place setup before initialising listener

2b0bd84

robertbrignull merged commit b623f92 into main Oct 9, 2023

robertbrignull deleted the robertbrignull/telemetry branch October 9, 2023 09:59

robertbrignull mentioned this pull request Oct 9, 2023

Update about-telemetry-in-codeql-for-visual-studio-code.rst to mention telemtry.telemetryLevel github/codeql#14408

Merged

robertbrignull mentioned this pull request Oct 9, 2023

Update CHANGELOG.md for telemetry changes #2923

Merged

3 tasks

shati-patel mentioned this pull request Feb 14, 2024

Replace deprecated telemetry setting in the tests #3362

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

robertbrignull commented Sep 15, 2023

robertbrignull commented Sep 15, 2023

robertbrignull Sep 15, 2023

aeisenberg Sep 15, 2023

koesie10 Sep 18, 2023

robertbrignull Sep 18, 2023

robertbrignull Sep 15, 2023

aeisenberg Sep 15, 2023

robertbrignull Sep 18, 2023 •

edited

Loading

robertbrignull Sep 15, 2023

aeisenberg Sep 15, 2023

aeisenberg Sep 15, 2023

robertbrignull Sep 18, 2023

aeisenberg Sep 15, 2023

aeisenberg left a comment

robertbrignull commented Sep 19, 2023

robertbrignull commented Oct 5, 2023 •

edited

Loading

robertbrignull commented Oct 9, 2023

aeisenberg commented Oct 10, 2023

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

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

Conversation

robertbrignull commented Sep 15, 2023

Checklist

robertbrignull commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbrignull Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg left a comment

Choose a reason for hiding this comment

robertbrignull commented Sep 19, 2023

robertbrignull commented Oct 5, 2023 • edited Loading

robertbrignull commented Oct 9, 2023

aeisenberg commented Oct 10, 2023

robertbrignull Sep 18, 2023 •

edited

Loading

robertbrignull commented Oct 5, 2023 •

edited

Loading