fix(settings): improve error messages and robustness (#2700)

JadenSimon · web-flow · commit b603c0750559 · 2022-07-25T10:57:21.000-07:00
* Clean up logging/errors for `settings.ts`
* Improve `onDidChange` fidelity for C9
diff --git a/src/extension.ts b/src/extension.ts
@@ -113,9 +113,8 @@ export async function activate(context: vscode.ExtensionContext) {
         const settings = Settings.instance
 
         await initializeCredentials(context, awsContext, settings)
-
         await activateTelemetry(context, awsContext, settings)
-        await globals.telemetry.start()
+
         await globals.schemaService.start()
         awsFiletypes.activate()
 
diff --git a/src/shared/settings.ts b/src/shared/settings.ts
@@ -9,8 +9,9 @@ import { getLogger } from './logger'
 import { cast, FromDescriptor, TypeConstructor, TypeDescriptor } from './utilities/typeConstructors'
 import { ClassToInterfaceType, keys } from './utilities/tsUtils'
 import { toRecord } from './utilities/collectionUtils'
-import { isAutomation, isNameMangled } from './vscode/env'
+import { isNameMangled } from './vscode/env'
 import { once } from './utilities/functionUtils'
+import { UnknownError } from './toolkitError'
 
 type Workspace = Pick<typeof vscode.workspace, 'getConfiguration' | 'onDidChangeConfiguration'>
 
@@ -181,11 +182,7 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
         protected readonly log = makeLogger(Object.getPrototypeOf(this)?.constructor?.name, 'debug')
         protected readonly logErr = makeLogger(Object.getPrototypeOf(this)?.constructor?.name, 'error')
 
-        public constructor(
-            private readonly settings: ClassToInterfaceType<Settings> = Settings.instance,
-            /** Throw an exception if the user config is invalid or the caller type doesn't match the stored type. */
-            private readonly throwInvalid: boolean = isAutomation()
-        ) {}
+        public constructor(private readonly settings: ClassToInterfaceType<Settings> = Settings.instance) {}
 
         public get onDidChange() {
             return this.getChangedEmitter().event
@@ -195,12 +192,14 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
             try {
                 return this.getOrThrow(key, defaultValue)
             } catch (e) {
-                if (this.throwInvalid || defaultValue === undefined) {
-                    throw new Error(`Failed to read key "${key}": ${e}`)
+                const message = UnknownError.cast(e)
+                if (arguments.length === 1) {
+                    throw new Error(`Failed to read key "${section}.${key}": ${message}`)
                 }
-                this.logErr(`using default for key "${key}", read failed: ${(e as Error).message ?? '?'}`)
 
-                return defaultValue
+                this.logErr(`using default for key "${key}", read failed: ${message}`)
+
+                return defaultValue as Inner[K]
             }
         }
 
@@ -252,21 +251,26 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
             //   - the "key" is `bar`
             //
             // So if `aws.foo.bar` changed, this would fire with data `{ key: 'bar' }`
+            //
+            // Note that `undefined` is not a valid JSON value. So using it as a default
+            // value is a valid way to express that the key exists but no (valid) value is set.
+
             const props = keys(descriptor)
-            const store = toRecord(props, p => this.get(p))
+            const store = toRecord(props, p => this.get(p, undefined))
             const emitter = new vscode.EventEmitter<{ readonly key: keyof T }>()
             const listener = this.settings.onDidChangeSection(section, event => {
-                const isDifferent = (p: keyof T & string) => event.affectsConfiguration(p) || store[p] !== this.get(p)
+                const isDifferent = (p: keyof T & string) => {
+                    const isDifferentLazy = () => {
+                        const previous = store[p]
+                        return previous !== (store[p] = this.get(p, undefined))
+                    }
+
+                    return event.affectsConfiguration(p) || isDifferentLazy()
+                }
 
                 for (const key of props.filter(isDifferent)) {
                     this.log(`key "${key}" changed`)
-                    try {
-                        store[key] = this.get(key)
-                        emitter.fire({ key })
-                    } catch {
-                        // getChangedEmitter() can't provide a default value so
-                        // we must silence errors here. Logging is done by this.get().
-                    }
+                    emitter.fire({ key })
                 }
             })
 
diff --git a/src/shared/telemetry/activation.ts b/src/shared/telemetry/activation.ts
@@ -15,6 +15,8 @@ import { getLogger } from '../logger'
 import { getComputeRegion, getIdeProperties, isCloud9 } from '../extensionUtilities'
 import { openSettings, Settings } from '../settings'
 import { TelemetryConfig } from './util'
+import { isAutomation, isReleaseVersion } from '../vscode/env'
+import { UnknownError } from '../toolkitError'
 
 export const noticeResponseViewSettings = localize('AWS.telemetry.notificationViewSettings', 'Settings')
 export const noticeResponseOk = localize('AWS.telemetry.notificationOk', 'OK')
@@ -32,28 +34,35 @@ const CURRENT_TELEMETRY_NOTICE_VERSION = 2
  * Sets up the Metrics system and initializes globals.telemetry
  */
 export async function activate(extensionContext: vscode.ExtensionContext, awsContext: AwsContext, settings: Settings) {
+    const config = new TelemetryConfig(settings)
     globals.telemetry = new DefaultTelemetryService(extensionContext, awsContext, getComputeRegion())
 
-    const config = new TelemetryConfig(
-        settings,
-        // Disable `throwInvalid` because:
+    try {
+        globals.telemetry.telemetryEnabled = config.isEnabled()
+
+        extensionContext.subscriptions.push(
+            config.onDidChange(event => {
+                if (event.key === 'telemetry') {
+                    globals.telemetry.telemetryEnabled = config.isEnabled()
+                }
+            })
+        )
+
+        // Prompt user about telemetry if they haven't been
+        if (!isCloud9() && !hasUserSeenTelemetryNotice(extensionContext)) {
+            showTelemetryNotice(extensionContext)
+        }
+
+        await globals.telemetry.start()
+    } catch (e) {
+        // Only throw in a production build because:
         //   1. Telemetry must never prevent normal Toolkit operation.
-        //   2. For tests, bad data is intentional, so these errors add unwanted noise in the test logs.
-        false
-    )
-    globals.telemetry.telemetryEnabled = config.isEnabled()
-
-    extensionContext.subscriptions.push(
-        config.onDidChange(event => {
-            if (event.key === 'telemetry') {
-                globals.telemetry.telemetryEnabled = config.isEnabled()
-            }
-        })
-    )
+        //   2. We want to know if something is not working ASAP during development.
+        if (isAutomation() || !isReleaseVersion()) {
+            throw e
+        }
 
-    // Prompt user about telemetry if they haven't been
-    if (!isCloud9() && !hasUserSeenTelemetryNotice(extensionContext)) {
-        showTelemetryNotice(extensionContext)
+        getLogger().error(`telemetry: failed to activate: ${UnknownError.cast(e).message}`)
     }
 }
 
diff --git a/src/shared/telemetry/util.ts b/src/shared/telemetry/util.ts
@@ -9,11 +9,14 @@ import { fromExtensionManifest } from '../settings'
 import { shared } from '../utilities/functionUtils'
 import { isAutomation } from '../vscode/env'
 import { v4 as uuidv4 } from 'uuid'
+import { addTypeName } from '../utilities/typeConstructors'
 
 const LEGACY_SETTINGS_TELEMETRY_VALUE_DISABLE = 'Disable'
 const LEGACY_SETTINGS_TELEMETRY_VALUE_ENABLE = 'Enable'
 
-export class TelemetryConfig extends fromExtensionManifest('aws', { telemetry: convertLegacy }) {
+const TelemetryFlag = addTypeName('boolean', convertLegacy)
+
+export class TelemetryConfig extends fromExtensionManifest('aws', { telemetry: TelemetryFlag }) {
     public isEnabled(): boolean {
         return this.get('telemetry', true)
     }
diff --git a/src/test/shared/settingsConfiguration.test.ts b/src/test/shared/settingsConfiguration.test.ts
@@ -8,14 +8,15 @@ import * as vscode from 'vscode'
 import { DevSettings, Experiments, fromExtensionManifest, PromptSettings, Settings } from '../../shared/settings'
 import { TestSettings } from '../utilities/testSettingsConfiguration'
 import { ClassToInterfaceType } from '../../shared/utilities/tsUtils'
+import { Optional } from '../../shared/utilities/typeConstructors'
 
 const SETTINGS_TARGET = vscode.ConfigurationTarget.Workspace
 
 describe('Settings', function () {
     // These tests use an actual extension setting, because vscode.WorkspaceConfiguration fails when
     // you attempt to update one that isn't defined in package.json. We will restore the setting value
     // at the end of the tests.
-    const SETTING_KEY = 'aws.telemetry'
+    const SETTING_KEY = 'aws.samcli.lambdaTimeout'
 
     let sut: Settings
 
@@ -67,11 +68,10 @@ describe('Settings', function () {
             //
             // Setting exists but has wrong type:
             //
-            await settings.update(SETTING_KEY, true, SETTINGS_TARGET)
+            await settings.update(SETTING_KEY, 123, SETTINGS_TARGET)
             assert.throws(() => sut.get(SETTING_KEY, String))
-            assert.throws(() => sut.get(SETTING_KEY, Number))
             assert.throws(() => sut.get(SETTING_KEY, Object))
-            assert.throws(() => sut.get(SETTING_KEY, Number))
+            assert.throws(() => sut.get(SETTING_KEY, Boolean))
         })
     })
 
@@ -140,6 +140,11 @@ describe('Settings', function () {
             assert.strictEqual(instance.get('profile', 'bar'), 'bar')
         })
 
+        it('can use `undefined` as a default value', function () {
+            const OptionalProfile = fromExtensionManifest('aws', { profile: Optional(String) })
+            assert.strictEqual(new OptionalProfile(settings).get('profile', undefined), undefined)
+        })
+
         it('can use a saved setting', async function () {
             await settings.update('aws.profile', 'foo')
             assert.strictEqual(instance.get('profile'), 'foo')
@@ -150,6 +155,11 @@ describe('Settings', function () {
             assert.strictEqual(instance.get('profile', 'bar'), 'foo')
         })
 
+        it('uses the default value if the setting is invalid', async function () {
+            await settings.update('aws.profile', true)
+            assert.strictEqual(instance.get('profile', 'foo'), 'foo')
+        })
+
         it('throws when the types do not match', async function () {
             assert.throws(() => instance.get('profile'))
 
@@ -200,17 +210,6 @@ describe('DevSetting', function () {
         assert.strictEqual(sut.get(TEST_SETTING, false), true)
         assert.deepStrictEqual(await state, { [TEST_SETTING]: true })
     })
-
-    it('bubbles up errors when in automation', async function () {
-        await settings.update(`aws.dev.${TEST_SETTING}`, 'junk')
-        assert.throws(() => sut.get(TEST_SETTING, false))
-    })
-
-    it('only throws in automation', async function () {
-        const noAutomation = new DevSettings(settings, false)
-        await settings.update(`aws.dev.${TEST_SETTING}`, 'junk')
-        assert.strictEqual(noAutomation.get(TEST_SETTING, true), true)
-    })
 })
 
 describe('PromptSetting', function () {
diff --git a/src/test/shared/telemetry/util.test.ts b/src/test/shared/telemetry/util.test.ts
@@ -11,19 +11,12 @@ import { FakeMemento } from '../../fakeExtensionContext'
 
 describe('TelemetryConfig', function () {
     const SETTING_KEY = 'aws.telemetry'
-
-    const target = ConfigurationTarget.Workspace
-    const settings = new Settings(target)
+    const settings = new Settings(ConfigurationTarget.Workspace)
 
     let sut: TelemetryConfig
 
     beforeEach(function () {
-        sut = new TelemetryConfig(
-            settings,
-            // Disable `throwInvalid`. These tests intentionally try invalid
-            // data, so the errors are unwanted noise in the test logs.
-            false
-        )
+        sut = new TelemetryConfig(settings)
     })
 
     afterEach(async function () {
