Only send telemetry events if not disabled in settings.json #55
Conversation
jonathanwangg
requested review from
smmatte,
Christellah,
LukeSlev,
markAtMicrosoft,
adclements,
FMounz and
abmahdy
|
|
||
| constructor(vscodeContext: vscode.ExtensionContext) { | ||
| TelemetryAI.telemetryReporter = this.createTelemetryReporter(vscodeContext); | ||
| TelemetryAI.enableTelemetry = vscode.workspace.getConfiguration().get("telemetry.enableTelemetry"); |
There was a problem hiding this comment.
If we need this to be instantiated for this to work, can we make the methods not static and then just have a global telemetry reporter in extension rather than instantiating a reporter and then using static methods? Because that's pretty funky
There was a problem hiding this comment.
Talked offline, will make the telemetry reporter a global which will change the way how telemetry events are being sent.
src/telemetry/telemetryAI.ts
Outdated
| @@ -5,7 +5,9 @@ import getPackageInfo from "./getPackageInfo"; | |||
| // tslint:disable-next-line:export-name | |||
| export default class TelemetryAI { | |||
| public static trackFeatureUsage(eventName: string, eventProperties?: { [key: string]: string }) { | |||
| TelemetryAI.telemetryReporter.sendTelemetryEvent(eventName, eventProperties); | |||
| if (TelemetryAI.enableTelemetry) { | |||
There was a problem hiding this comment.
It would be nice if there as few 'if' checks on TelemetryAI.enableTelemetry as possible. Can you find a way to put the check somewhere in TelemetryAI.telemetryReporter so that it is more automatic? Or make a dummy telemetryReporter object that does nothing when telemetry is disabled?
There was a problem hiding this comment.
I can take a look into this, but will probably need your input after I re-write it!
There was a problem hiding this comment.
I agree, ideally with a single point of telemetry you should only check in one place if it is enabled. Having that code all over the place duplicated makes it a maintenance challenge.
src/telemetry/telemetryAI.ts
Outdated
| @@ -5,7 +5,9 @@ import getPackageInfo from "./getPackageInfo"; | |||
| // tslint:disable-next-line:export-name | |||
| export default class TelemetryAI { | |||
| public static trackFeatureUsage(eventName: string, eventProperties?: { [key: string]: string }) { | |||
| TelemetryAI.telemetryReporter.sendTelemetryEvent(eventName, eventProperties); | |||
| if (TelemetryAI.enableTelemetry) { | |||
There was a problem hiding this comment.
I agree, ideally with a single point of telemetry you should only check in one place if it is enabled. Having that code all over the place duplicated makes it a maintenance challenge.
| @@ -35,8 +25,25 @@ export default class TelemetryAI { | |||
| return extensionVersion; | |||
| } | |||
|
|
|||
| public trackEventTime(eventName: string, startTime: number, endTime: number = Date.now(), eventProperties?: { [key: string]: string }) { | |||
| this.trackTimeDuration(eventName, startTime, endTime, eventProperties); | |||
| public sendTelemetryIfEnabled(eventName: string, properties?: { [key: string]: string }, measurements?: { [key: string]: number }) { | |||
Description:
Using the
telemetry.enableTelemetryproperty in the User levelsettings.json(which is accessed inPreferences), the user can disable telemetry and telemetry events will not be sent.Type of change
Testing:
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
telemetry.enableTelemetrytofalsein the User levelsettings.json) and make sure that https://portal.azure.com/ does NOT log your telemetry event.Checklist: