From c13f1abb110fc756f9b3a6f16670df9cd9d4cf63 Mon Sep 17 00:00:00 2001 From: Logan Ramos Date: Wed, 13 Oct 2021 20:47:59 -0400 Subject: [PATCH] Better telemetry setting UI + clearer wording surrounding new setting (#135030) * Address #134660 * Further address #134660 * Clearer Settings UI for new telemetry setting (#135028) * Work on improving telemetry setting description * two options * third option * Add old crash reporter to getTelemetryLevel * More verbose wording + comments Co-authored-by: SteVen Batten * Update telemetry text * fix strings Co-authored-by: SteVen Batten --- src/vs/code/electron-main/app.ts | 5 +- src/vs/platform/telemetry/common/telemetry.ts | 4 +- .../telemetry/common/telemetryService.ts | 72 ++++++++++++------- .../telemetry/common/telemetryUtils.ts | 10 ++- .../electron-sandbox/desktop.contribution.ts | 2 +- 5 files changed, 58 insertions(+), 35 deletions(-) diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts index 1d71361b94274..d2341a7aee3bc 100644 --- a/src/vs/code/electron-main/app.ts +++ b/src/vs/code/electron-main/app.ts @@ -990,9 +990,8 @@ export class CodeApplication extends Disposable { const argvString = argvContent.value.toString(); const argvJSON = JSON.parse(stripComments(argvString)); if (argvJSON['enable-crash-reporter'] === undefined) { - const telemetryConfig = getTelemetryLevel(this.configurationService); - const enableCrashReporterSetting = telemetryConfig >= TelemetryLevel.ERROR; - const enableCrashReporter = typeof enableCrashReporterSetting === 'boolean' ? enableCrashReporterSetting : true; + const telemetryLevel = getTelemetryLevel(this.configurationService); + const enableCrashReporter = telemetryLevel >= TelemetryLevel.CRASH; const additionalArgvContent = [ '', ' // Allows to disable crash reporting.', diff --git a/src/vs/platform/telemetry/common/telemetry.ts b/src/vs/platform/telemetry/common/telemetry.ts index 2746301674ca2..4f0dfa36049cc 100644 --- a/src/vs/platform/telemetry/common/telemetry.ts +++ b/src/vs/platform/telemetry/common/telemetry.ts @@ -79,12 +79,14 @@ export const TELEMETRY_OLD_SETTING_ID = 'telemetry.enableTelemetry'; export const enum TelemetryLevel { NONE = 0, + CRASH = 1, ERROR = 2, USAGE = 3 } export const enum TelemetryConfiguration { OFF = 'off', + CRASH = 'crash', ERROR = 'error', - ON = 'on' + ON = 'all' } diff --git a/src/vs/platform/telemetry/common/telemetryService.ts b/src/vs/platform/telemetry/common/telemetryService.ts index dd93c6426b717..a3b17f5a82120 100644 --- a/src/vs/platform/telemetry/common/telemetryService.ts +++ b/src/vs/platform/telemetry/common/telemetryService.ts @@ -76,29 +76,12 @@ export class TelemetryService implements ITelemetryService { }; this.publicLog2<{ usingFallbackGuid: boolean }, MachineIdFallbackClassification>('machineIdFallback', { usingFallbackGuid: !isHashedId }); }); - - // TODO @sbatten @lramos15 bring this code in after one iteration - // Once the service initializes we update the telemetry value to the new format - // this._convertOldTelemetrySettingToNew(); - // this._configurationService.onDidChangeConfiguration(e => { - // if (e.affectsConfiguration(TELEMETRY_OLD_SETTING_ID)) { - // this._convertOldTelemetrySettingToNew(); - // } - // }, this); } setExperimentProperty(name: string, value: string): void { this._experimentProperties[name] = value; } - // TODO: @sbatten @lramos15 bring this code in after one iteration - // private _convertOldTelemetrySettingToNew(): void { - // const telemetryValue = this._configurationService.getValue(TELEMETRY_OLD_SETTING_ID); - // if (typeof telemetryValue === 'boolean') { - // this._configurationService.updateValue(TELEMETRY_SETTING_ID, telemetryValue ? 'true' : 'false'); - // } - // } - private _updateTelemetryLevel(): void { this._telemetryLevel = getTelemetryLevel(this._configurationService); } @@ -256,7 +239,44 @@ export class TelemetryService implements ITelemetryService { } } -const restartString = !isWeb ? ' ' + localize('telemetry.restart', 'Some features may require a restart to take effect.') : ''; +function getTelemetryLevelSettingDescription(): string { + const telemetryText = localize('telemetry.telemetryLevelMd', "Controls all core and first party extension telemetry. This helps us to better understand how {0} is performing, where improvements need to be made, and how features are being used.", product.nameLong); + const externalLinksStatement = !product.privacyStatementUrl ? + localize("telemetry.docsStatement", "Read more about the [data we collect]({0}).", 'https://aka.ms/vscode-telemetry') : + localize("telemetry.docsAndPrivacyStatement", "Read more about the [data we collect]({0}) and our [privacy statement]({1}).", 'https://aka.ms/vscode-telemetry', product.privacyStatementUrl); + const restartString = !isWeb ? localize('telemetry.restart', 'A full restart of the application is necessary for crash reporting changes to take effect.') : ''; + + const crashReportsHeader = localize('telemetry.crashReports', "Crash Reports"); + const errorsHeader = localize('telemetry.errors', "Error Telemetry"); + const usageHeader = localize('telemetry.usage', "Usage Data"); + + const telemetryTableDescription = localize('telemetry.telemetryLevel.tableDescription', "The following table outlines the data sent with each setting:"); + const telemetryTable = ` +| | ${crashReportsHeader} | ${errorsHeader} | ${usageHeader} | +|:------|:---------------------:|:---------------:|:--------------:| +| all | ✓ | ✓ | ✓ | +| error | ✓ | ✓ | - | +| crash | ✓ | - | - | +| off | - | - | - | +`; + + const deprecatedSettingNote = localize('telemetry.telemetryLevel.deprecated', "****Note:*** If this setting is 'off', no telemetry will be sent regardless of other telemetry settings. If this setting is set to anything except 'off' and telemetry is disabled with deprecated settings, no telemetry will be sent.*"); + const telemetryDescription = ` +${telemetryText} ${externalLinksStatement} ${restartString} + +  + +${telemetryTableDescription} +${telemetryTable} + +  + +${deprecatedSettingNote} +`; + + return telemetryDescription; +} + Registry.as(Extensions.Configuration).registerConfiguration({ 'id': TELEMETRY_SECTION_ID, 'order': 110, @@ -265,16 +285,14 @@ Registry.as(Extensions.Configuration).registerConfigurat 'properties': { [TELEMETRY_SETTING_ID]: { 'type': 'string', - 'enum': [TelemetryConfiguration.ON, TelemetryConfiguration.ERROR, TelemetryConfiguration.OFF], + 'enum': [TelemetryConfiguration.ON, TelemetryConfiguration.ERROR, TelemetryConfiguration.CRASH, TelemetryConfiguration.OFF], 'enumDescriptions': [ - localize('telemetry.enableTelemetry.default', "Enables all telemetry data to be collected."), - localize('telemetry.enableTelemetry.error', "Enables only error telemetry data and not general usage data."), - localize('telemetry.enableTelemetry.off', "Disables all product telemetry.") + localize('telemetry.telemetryLevel.default', "Sends usage data, errors, and crash reports."), + localize('telemetry.telemetryLevel.error', "Sends general error telemetry and crash reports."), + localize('telemetry.telemetryLevel.crash', "Sends OS level crash reports."), + localize('telemetry.telemetryLevel.off', "Disables all product telemetry.") ], - 'markdownDescription': - !product.privacyStatementUrl ? - localize('telemetry.enableTelemetry', "Enable diagnostic data to be collected. This helps us to better understand how {0} is performing and where improvements need to be made.", product.nameLong) + restartString : - localize('telemetry.enableTelemetryMd', "Enable diagnostic data to be collected. This helps us to better understand how {0} is performing and where improvements need to be made. [Read more]({1}) about what we collect and our privacy statement.", product.nameLong, product.privacyStatementUrl) + restartString, + 'markdownDescription': getTelemetryLevelSettingDescription(), 'default': TelemetryConfiguration.ON, 'restricted': true, 'scope': ConfigurationScope.APPLICATION, @@ -298,7 +316,7 @@ Registry.as(Extensions.Configuration).registerConfigurat localize('telemetry.enableTelemetryMd', "Enable diagnostic data to be collected. This helps us to better understand how {0} is performing and where improvements need to be made. [Read more]({1}) about what we collect and our privacy statement.", product.nameLong, product.privacyStatementUrl), 'default': true, 'restricted': true, - 'markdownDeprecationMessage': localize('enableTelemetryDeprecated', "Deprecated in favor of the {0} setting.", `\`#${TELEMETRY_SETTING_ID}#\``), + 'markdownDeprecationMessage': localize('enableTelemetryDeprecated', "If this setting is false, no telemetry will be sent regardless of the new setting's value. Deprecated in favor of the {0} setting.", `\`#${TELEMETRY_SETTING_ID}#\``), 'scope': ConfigurationScope.APPLICATION, 'tags': ['usesOnlineServices', 'telemetry'] } diff --git a/src/vs/platform/telemetry/common/telemetryUtils.ts b/src/vs/platform/telemetry/common/telemetryUtils.ts index a558f880a1dc6..6ab2158d01ef0 100644 --- a/src/vs/platform/telemetry/common/telemetryUtils.ts +++ b/src/vs/platform/telemetry/common/telemetryUtils.ts @@ -119,18 +119,22 @@ export function supportsTelemetry(productService: IProductService, environmentSe */ export function getTelemetryLevel(configurationService: IConfigurationService): TelemetryLevel { const newConfig = configurationService.getValue(TELEMETRY_SETTING_ID); - const oldConfig = configurationService.getValue(TELEMETRY_OLD_SETTING_ID); + const crashReporterConfig = configurationService.getValue('telemetry.enableCrashReporter'); + const oldConfig = configurationService.getValue(TELEMETRY_OLD_SETTING_ID); - // Check old config for disablement - if (oldConfig !== undefined && oldConfig === false) { + // If `telemetry.enableCrashReporter` is false or `telemetry.enableTelemetry' is false, disable telemetry + if (oldConfig === false || crashReporterConfig === false) { return TelemetryLevel.NONE; } + // Maps new telemetry setting to a telemetry level switch (newConfig ?? TelemetryConfiguration.ON) { case TelemetryConfiguration.ON: return TelemetryLevel.USAGE; case TelemetryConfiguration.ERROR: return TelemetryLevel.ERROR; + case TelemetryConfiguration.CRASH: + return TelemetryLevel.CRASH; case TelemetryConfiguration.OFF: return TelemetryLevel.NONE; } diff --git a/src/vs/workbench/electron-sandbox/desktop.contribution.ts b/src/vs/workbench/electron-sandbox/desktop.contribution.ts index 29031833f45cc..d0e1b87402cfb 100644 --- a/src/vs/workbench/electron-sandbox/desktop.contribution.ts +++ b/src/vs/workbench/electron-sandbox/desktop.contribution.ts @@ -235,7 +235,7 @@ import { TELEMETRY_SETTING_ID } from 'vs/platform/telemetry/common/telemetry'; 'description': localize('telemetry.enableCrashReporting', "Enable crash reports to be collected. This helps us improve stability. \nThis option requires restart to take effect."), 'default': true, 'tags': ['usesOnlineServices', 'telemetry'], - 'markdownDeprecationMessage': localize('enableCrashReporterDeprecated', "Deprecated due to being combined into the {0} setting.", `\`#${TELEMETRY_SETTING_ID}#\``), + 'markdownDeprecationMessage': localize('enableCrashReporterDeprecated', "If this setting is false, no telemetry will be sent regardless of the new setting's value. Deprecated due to being combined into the {0} setting.", `\`#${TELEMETRY_SETTING_ID}#\``), } } });