telemetry: enforce result and reason properties for emitted metrics (#4103)

* telemetry: enforce `result` and `reason` properties for emitted metrics

Problem: Different integrations within the toolkit publish metrics differently. The 'result' and 'reason' fields are meant
to be standardized, but some integrations are adding their own fields.
Solution: Verify that any metrics emitted using `emit()` contain a result property. If the result property is 'Failed', then
a reason property must also exists. Any tests that violate this code are failed. We do not want to fail in general because
it should not create an exception during production toolkit usage.

* refactor: telemetry property check no longer uses spy

* Update src/test/globalSetup.test.ts

Co-authored-by: Justin M. Keyes <[email protected]>

* Update src/test/globalSetup.test.ts

Co-authored-by: Justin M. Keyes <[email protected]>

* refactor: move telemetry validation out of tests

* Update spans.test.ts

* Update TESTPLAN.md

* Apply suggestions from code review

Co-authored-by: Justin M. Keyes <[email protected]>

* finish implementation of PR suggestions

* update metrics exempt list

* fix: pass validation for metric 'aws_modifySetting'

---------

Co-authored-by: Justin M. Keyes <[email protected]>

Author: hayemaxi

docs/TESTPLAN.md

@@ -81,6 +81,7 @@ modifications/workarounds in `src/test/testRunner.ts`.
 -   Testing AWS SDK client functionality is cumbersome, verbose, and low-yield.
 -   Test code uses multiple “mocking” frameworks, which is confusing, error-prone, hard to onboard, and hard to use.
 -   Coverage not counted for integ tests (because of unresolved tooling issue).
+-   [Backlog](https://github.com/aws/aws-toolkit-vscode/blob/0f3685ab0dc8af3a214136ebfa233829d5d72b2c/src/shared/telemetry/exemptMetrics.ts) of metrics that do not pass validation but are temporarily exempted to not fail CI.
 
 ## Window
 

src/codewhisperer/commands/basicCommands.ts

@@ -41,13 +41,19 @@ import { FeatureConfigProvider } from '../service/featureConfigProvider'
 export const toggleCodeSuggestions = Commands.declare(
     { id: 'aws.codeWhisperer.toggleCodeSuggestion', compositeKey: { 1: 'source' } },
     (suggestionState: CodeSuggestionsState) => async (_: VsCodeCommandArg, source: CodeWhispererSource) => {
-        const isSuggestionsEnabled = await suggestionState.toggleSuggestions()
-        await vscode.commands.executeCommand('aws.codeWhisperer.refresh')
-        telemetry.aws_modifySetting.emit({
-            settingId: CodeWhispererConstants.autoSuggestionConfig.settingId,
-            settingState: isSuggestionsEnabled
-                ? CodeWhispererConstants.autoSuggestionConfig.activated
-                : CodeWhispererConstants.autoSuggestionConfig.deactivated,
+        await telemetry.aws_modifySetting.run(async span => {
+            span.record({
+                settingId: CodeWhispererConstants.autoSuggestionConfig.settingId,
+            })
+
+            const isSuggestionsEnabled = await suggestionState.toggleSuggestions()
+            span.record({
+                settingState: isSuggestionsEnabled
+                    ? CodeWhispererConstants.autoSuggestionConfig.activated
+                    : CodeWhispererConstants.autoSuggestionConfig.deactivated,
+            })
+
+            await vscode.commands.executeCommand('aws.codeWhisperer.refresh')
         })
     }
 )

src/shared/telemetry/exemptMetrics.ts

@@ -0,0 +1,61 @@
+/*!
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/**
+ * TODO: Fix calls to these metrics and remove this file.
+ * NOTE: Do NOT add additional metrics here, your PR will be rejected. All new metrics should pass validation.
+ *
+ * Checks were added to all emitted telemetry metrics to ensure they contain the correct properties.
+ * This check does not exist outside test/automated runtimes because the error is not useful
+ * for the user and doesn't break telemetry.
+ *
+ * These are metrics that fail validation and are emitted by code that is covered in test cases.
+ * This allowlist exists to fix these incrementally and not have CI fail in the meantime.
+ */
+const validationExemptMetrics: Set<string> = new Set([
+    'amazonq_runCommand',
+    'amazonq_isReviewedChanges',
+    'apigateway_copyUrl',
+    'aws_loadCredentials',
+    'aws_validateCredentials',
+    'cloudwatchlogs_download',
+    'codeTransform_isDoubleClickedToTriggerInvalidProject',
+    'codeTransform_jobIsCancelledByUser',
+    'codeTransform_jobStatusChanged',
+    'codeTransform_logApiError',
+    'codeTransform_logApiLatency',
+    'codewhisperer_userDecision',
+    'codewhisperer_codeScanIssueHover',
+    'codewhisperer_codePercentage',
+    'codewhisperer_userModification',
+    'codewhisperer_userTriggerDecision',
+    'dynamicresource_selectResources',
+    'dynamicresource_copyIdentifier',
+    'dynamicresource_mutateResource',
+    'dynamicresource_getResource',
+    'dynamicresource_listResource',
+    'ecr_copyRepositoryUri',
+    'ecr_copyTagUri',
+    'ecr_createRepository',
+    'ecr_deleteRepository',
+    'ecr_deleteTags',
+    'feedback_result',
+    'lambda_delete',
+    's3_uploadObject',
+    'sam_deploy',
+    'session_start',
+    'ssm_deleteDocument',
+    'ssm_openDocument',
+    'ssm_publishDocument',
+    'ssm_updateDocumentVersion',
+    'stepfunctions_previewstatemachine',
+    'ui_click',
+    'vscode_activeRegions',
+    'vscode_executeCommand',
+])
+
+export function isValidationExemptMetric(metricName: string) {
+    return validationExemptMetrics.has(metricName)
+}

src/shared/telemetry/telemetryLogger.ts

@@ -30,7 +30,7 @@ function isValidEntry(datum: MetadataEntry): datum is Required<MetadataEntry> {
 /**
  * Telemetry currently sends metadata as an array of key/value pairs, but this is unintuitive for JS
  */
-const mapMetadata = (excludeKeys: string[]) => (metadata: Required<MetricDatum>['Metadata']) => {
+export const mapMetadata = (excludeKeys: string[]) => (metadata: Required<MetricDatum>['Metadata']) => {
     const result: Metadata = {}
     return metadata
         .filter(isValidEntry)

src/shared/telemetry/telemetryService.ts

@@ -17,7 +17,7 @@ import { accountMetadataKey, AccountStatus, computeRegionKey } from './telemetry
 import { TelemetryLogger } from './telemetryLogger'
 import globals from '../extensionGlobals'
 import { ClassToInterfaceType } from '../utilities/tsUtils'
-import { getClientId } from './util'
+import { getClientId, validateMetricEvent } from './util'
 import { telemetry } from './telemetry'
 import { FileSystemCommon } from '../../srcShared/fs'
 
@@ -148,6 +148,10 @@ export class DefaultTelemetryService {
 
     public record(event: MetricDatum, awsContext?: AwsContext): void {
         if (this.telemetryEnabled) {
+            // TODO: While this catches cases in code that is tested, untested code will still release incomplete metrics.
+            // Consider using custom lint rules to address these cases if possible.
+            validateMetricEvent(event, isAutomation())
+
             const actualAwsContext = awsContext || this.awsContext
             const eventWithCommonMetadata = this.injectCommonMetadata(event, actualAwsContext)
             this._eventQueue.push(eventWithCommonMetadata)

src/shared/telemetry/util.ts

@@ -11,6 +11,10 @@ import { extensionVersion, isAutomation } from '../vscode/env'
 import { v4 as uuidv4 } from 'uuid'
 import { addTypeName } from '../utilities/typeConstructors'
 import globals from '../extensionGlobals'
+import { mapMetadata } from './telemetryLogger'
+import { Result } from './telemetry.gen'
+import { MetricDatum } from './clienttelemetry'
+import { isValidationExemptMetric } from './exemptMetrics'
 
 const legacySettingsTelemetryValueDisable = 'Disable'
 const legacySettingsTelemetryValueEnable = 'Enable'
@@ -85,3 +89,40 @@ export async function getUserAgent(
 
     return pairs.join(' ')
 }
+
+/**
+ * Validates that emitted telemetry metrics
+ * 1. contain a result property and
+ * 2. contain a reason propery if result = 'Failed'.
+ */
+export function validateMetricEvent(event: MetricDatum, fatal: boolean) {
+    const failedStr: Result = 'Failed'
+    const telemetryRunDocsStr =
+        'Consider using `.run()` instead of `.emit()`, which will set these properties automatically. ' +
+        'See https://github.com/aws/aws-toolkit-vscode/blob/master/docs/telemetry.md#guidelines'
+
+    if (!isValidationExemptMetric(event.MetricName) && event.Metadata) {
+        const metadata = mapMetadata([])(event.Metadata)
+
+        try {
+            if (metadata.result === undefined) {
+                throw new Error(
+                    `Metric \`${event.MetricName}\` was emitted without the \`result\` property. ` +
+                        `This property is always required. ${telemetryRunDocsStr}`
+                )
+            }
+
+            if (metadata.result === failedStr && metadata.reason === undefined) {
+                throw new Error(
+                    `Metric \`${event.MetricName}\` was emitted without the \`reason\` property. ` +
+                        `This property is always required when \`result\` = 'Failed'. ${telemetryRunDocsStr}`
+                )
+            }
+        } catch (err: any) {
+            if (fatal) {
+                throw err
+            }
+            getLogger().warn(`Metric Event did not pass validation: ${(err as Error).message}`)
+        }
+    }
+}

src/test/shared/telemetry/spans.test.ts

@@ -22,9 +22,9 @@ describe('TelemetrySpan', function () {
     })
 
     it('removes passive and value from the metadata', function () {
-        new TelemetrySpan('foo').emit({ passive: false, value: 100, reason: 'bar' })
+        new TelemetrySpan('foo').emit({ passive: false, value: 100, result: 'Succeeded', reason: 'bar' })
 
-        assertTelemetry('foo' as MetricName, { reason: 'bar' })
+        assertTelemetry('foo' as MetricName, { result: 'Succeeded', reason: 'bar' })
     })
 
     it('records duration if a start time is available', function () {