From a5ab6c330a365d33d7d87292b8f870c423e97394 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Tue, 18 Feb 2025 16:09:49 -0500 Subject: [PATCH 1/6] refactor: simplify isFirstUse for Q Avoid any invalid logic involved with hasExistingConnections() Signed-off-by: nkomonen-amazon --- packages/core/src/auth/utils.ts | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/packages/core/src/auth/utils.ts b/packages/core/src/auth/utils.ts index 7c2adbe8086..58174073ba1 100644 --- a/packages/core/src/auth/utils.ts +++ b/packages/core/src/auth/utils.ts @@ -688,17 +688,24 @@ export class ExtensionUse { return this.isFirstUseCurrentSession } - this.isFirstUseCurrentSession = globals.globalState.get('isExtensionFirstUse') - if (this.isFirstUseCurrentSession === undefined) { + // This is for sure not their first use + const isFirstUse = globals.globalState.tryGet('isExtensionFirstUse', Boolean) + if (isFirstUse === false) { + this.isFirstUseCurrentSession = isFirstUse + return this.isFirstUseCurrentSession + } + + if (isAmazonQ()) { + this.isFirstUseCurrentSession = true + } else { // The variable in the store is not defined yet, fallback to checking if they have existing connections. this.isFirstUseCurrentSession = !hasExistingConnections() - - getLogger().debug( - `isFirstUse: State not found, marking user as '${ - this.isFirstUseCurrentSession ? '' : 'NOT ' - }first use' since they 'did ${this.isFirstUseCurrentSession ? 'NOT ' : ''}have existing connections'.` - ) } + getLogger().debug( + `isFirstUse: State not found, marking user as '${ + this.isFirstUseCurrentSession ? '' : 'NOT ' + }first use' since they 'did ${this.isFirstUseCurrentSession ? 'NOT ' : ''}have existing connections'.` + ) // Update state, so next time it is not first use this.updateMemento('isExtensionFirstUse', false) From 6f6fedb19b9db6f037b17a89036a5d670aaff4a9 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Tue, 18 Feb 2025 17:19:17 -0500 Subject: [PATCH 2/6] refactor: sync isFirstUse w/ ClientId Problem: We were seeing some cases where the ClientId was seen as new, but ifFirstUse was reporting as not new. Solution: Validate that if we have a new ClientId, that isFirstUse MUST be true. Otherwise we emit an error, that should help us debug certain cases. Signed-off-by: nkomonen-amazon --- packages/core/src/auth/utils.ts | 14 +++++++- packages/core/src/shared/telemetry/util.ts | 14 ++++++++ .../src/test/shared/telemetry/util.test.ts | 32 ++++++++++++++++--- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/packages/core/src/auth/utils.ts b/packages/core/src/auth/utils.ts index 58174073ba1..5529d9d80a3 100644 --- a/packages/core/src/auth/utils.ts +++ b/packages/core/src/auth/utils.ts @@ -58,7 +58,7 @@ import { EcsCredentialsProvider } from './providers/ecsCredentialsProvider' import { EnvVarsCredentialsProvider } from './providers/envVarsCredentialsProvider' import { showMessageWithUrl } from '../shared/utilities/messages' import { credentialHelpUrl } from '../shared/constants' -import { ExtStartUpSources, ExtStartUpSource } from '../shared/telemetry/util' +import { ExtStartUpSources, ExtStartUpSource, hadClientIdOnStartup } from '../shared/telemetry/util' // iam-only excludes Builder ID and IAM Identity Center from the list of valid connections // TODO: Understand if "iam" should include these from the list at all @@ -695,6 +695,18 @@ export class ExtensionUse { return this.isFirstUseCurrentSession } + /** + * SANITY CHECK: If the clientId already existed on startup, then isFirstUse MUST be false. So + * there is a bug in the state. + */ + if (hadClientIdOnStartup(globals.globalState)) { + telemetry.function_call.emit({ + result: 'Failed', + functionName: 'isFirstUse', + reason: 'ClientIdAlreadyExisted', + }) + } + if (isAmazonQ()) { this.isFirstUseCurrentSession = true } else { diff --git a/packages/core/src/shared/telemetry/util.ts b/packages/core/src/shared/telemetry/util.ts index c73a62a631f..5fbdfa8c967 100644 --- a/packages/core/src/shared/telemetry/util.ts +++ b/packages/core/src/shared/telemetry/util.ts @@ -30,6 +30,7 @@ import { asStringifiedStack, FunctionEntry } from './spans' import { telemetry } from './telemetry' import { v5 as uuidV5 } from 'uuid' import { ToolkitError } from '../errors' +import { GlobalState } from '../globalState' const legacySettingsTelemetryValueDisable = 'Disable' const legacySettingsTelemetryValueEnable = 'Enable' @@ -177,6 +178,8 @@ export const getClientId = memoize( const localClientId = globalState.tryGet('telemetryClientId', String) // local to extension, despite accessing "global" state let clientId: string + _hadClientIdOnStartup = !!globalClientId || !!localClientId + if (isWeb()) { const machineId = vscode.env.machineId clientId = localClientId ?? machineId @@ -210,6 +213,17 @@ export const getClientId = memoize( } ) +let _hadClientIdOnStartup = false +/** + * Returns true if the ClientID existed before this session started + */ +export const hadClientIdOnStartup = (globalState: GlobalState) => { + // triggers the flow that will update the state, if not done already + getClientId(globalState) + + return _hadClientIdOnStartup +} + export const platformPair = () => `${env.appName.replace(/\s/g, '-')}/${version}` /** diff --git a/packages/core/src/test/shared/telemetry/util.test.ts b/packages/core/src/test/shared/telemetry/util.test.ts index 4c19e924585..14eb2c4c3d9 100644 --- a/packages/core/src/test/shared/telemetry/util.test.ts +++ b/packages/core/src/test/shared/telemetry/util.test.ts @@ -10,6 +10,7 @@ import { convertLegacy, getClientId, getUserAgent, + hadClientIdOnStartup, platformPair, SessionId, telemetryClientIdEnvKey, @@ -128,20 +129,24 @@ describe('getSessionId', function () { describe('getClientId', function () { before(function () { - delete process.env[telemetryClientIdEnvKey] + setClientIdEnvVar(undefined) }) afterEach(function () { - delete process.env[telemetryClientIdEnvKey] + setClientIdEnvVar(undefined) }) function testGetClientId(globalState: GlobalState) { return getClientId(globalState, true, false, randomUUID()) } + function setClientIdEnvVar(val: string | undefined) { + process.env[telemetryClientIdEnvKey] = val + } + it('generates a unique id if no other id is available', function () { const c1 = testGetClientId(new GlobalState(new FakeMemento())) - delete process.env[telemetryClientIdEnvKey] + setClientIdEnvVar(undefined) const c2 = testGetClientId(new GlobalState(new FakeMemento())) assert.notStrictEqual(c1, c2) }) @@ -160,7 +165,7 @@ describe('getClientId', function () { const e = new GlobalState(new FakeMemento()) await e.update('telemetryClientId', randomUUID()) - process.env[telemetryClientIdEnvKey] = expectedClientId + setClientIdEnvVar(expectedClientId) assert.strictEqual(testGetClientId(new GlobalState(new FakeMemento())), expectedClientId) }) @@ -218,6 +223,25 @@ describe('getClientId', function () { const clientId = getClientId(new GlobalState(new FakeMemento()), false, false) assert.strictEqual(clientId, '11111111-1111-1111-1111-111111111111') }) + + describe('hadClientIdOnStartup', async function () { + it('returns false when no existing clientId', async function () { + const globalState = new GlobalState(new FakeMemento()) + assert.strictEqual(hadClientIdOnStartup(globalState), false) + }) + + it('returns true when existing env var clientId', async function () { + const globalState = new GlobalState(new FakeMemento()) + setClientIdEnvVar('aaa-111') + assert.strictEqual(hadClientIdOnStartup(globalState), true) + }) + + it('returns true when existing state clientId', async function () { + const globalState = new GlobalState(new FakeMemento()) + await globalState.update('telemetryClientId', 'bbb-222') + assert.strictEqual(hadClientIdOnStartup(globalState), true) + }) + }) }) describe('getUserAgent', function () { From 0231f0fac0456501bc1b4fa0fff4464f7f7ec60c Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Tue, 18 Feb 2025 17:58:24 -0500 Subject: [PATCH 3/6] telemetry: add isFirstUse to session_start, auth_signInPageOpened, auth_userState Signed-off-by: nkomonen-amazon --- packages/amazonq/src/extension.ts | 2 +- .../login/webview/commonAuthViewProvider.ts | 13 +++++++++-- .../src/shared/telemetry/telemetryService.ts | 5 ++++- .../src/shared/telemetry/vscodeTelemetry.json | 22 +++++++++++++++++++ 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/packages/amazonq/src/extension.ts b/packages/amazonq/src/extension.ts index a4b53dbf66d..c351b7ba9eb 100644 --- a/packages/amazonq/src/extension.ts +++ b/packages/amazonq/src/extension.ts @@ -142,7 +142,7 @@ export async function activateAmazonQCommon(context: vscode.ExtensionContext, is // Give time for the extension to finish initializing. globals.clock.setTimeout(async () => { CommonAuthWebview.authSource = ExtStartUpSources.firstStartUp - void focusAmazonQPanel.execute(placeholder, 'firstStartUp') + void focusAmazonQPanel.execute(placeholder, ExtStartUpSources.firstStartUp) }, 1000) } } diff --git a/packages/core/src/login/webview/commonAuthViewProvider.ts b/packages/core/src/login/webview/commonAuthViewProvider.ts index c96b79bea8c..12caf3c2ad5 100644 --- a/packages/core/src/login/webview/commonAuthViewProvider.ts +++ b/packages/core/src/login/webview/commonAuthViewProvider.ts @@ -46,6 +46,7 @@ import { AuthSources } from './util' import { AuthFlowStates } from './vue/types' import { getTelemetryMetadataForConn } from '../../auth/connection' import { AuthUtil } from '../../codewhisperer/util/authUtil' +import { ExtensionUse } from '../../auth/utils' export class CommonAuthViewProvider implements WebviewViewProvider { public readonly viewType: string @@ -83,14 +84,22 @@ export class CommonAuthViewProvider implements WebviewViewProvider { ) { // Our callback won't fire on the first view. if (webviewView.visible) { - telemetry.auth_signInPageOpened.emit({ result: 'Succeeded', passive: true }) + telemetry.auth_signInPageOpened.emit({ + result: 'Succeeded', + passive: true, + source: ExtensionUse.instance.sourceForTelemetry(), + }) } // This will fire whenever the user opens or closes the login page from 'somewhere else' // i.e. NOT when switching from/to the chat window, which uses the same view area. webviewView.onDidChangeVisibility(async () => { if (webviewView.visible) { - telemetry.auth_signInPageOpened.emit({ result: 'Succeeded', passive: true }) + telemetry.auth_signInPageOpened.emit({ + result: 'Succeeded', + passive: true, + source: ExtensionUse.instance.sourceForTelemetry(), + }) } else { telemetry.auth_signInPageClosed.emit({ result: 'Succeeded', passive: true }) diff --git a/packages/core/src/shared/telemetry/telemetryService.ts b/packages/core/src/shared/telemetry/telemetryService.ts index c3395be1120..6c82f23bda2 100644 --- a/packages/core/src/shared/telemetry/telemetryService.ts +++ b/packages/core/src/shared/telemetry/telemetryService.ts @@ -22,6 +22,7 @@ import { telemetry, MetricBase } from './telemetry' import fs from '../fs/fs' import fsNode from 'fs/promises' import * as collectionUtil from '../utilities/collectionUtils' +import { ExtensionUse } from '../../auth/utils' export type TelemetryService = ClassToInterfaceType @@ -98,7 +99,9 @@ export class DefaultTelemetryService { // TODO: `readEventsFromCache` should be async this._eventQueue.push(...(await DefaultTelemetryService.readEventsFromCache(this.persistFilePath))) this._endOfCache = this._eventQueue[this._eventQueue.length - 1] - telemetry.session_start.emit() + telemetry.session_start.emit({ + source: ExtensionUse.instance.sourceForTelemetry(), + }) this.startFlushInterval() } diff --git a/packages/core/src/shared/telemetry/vscodeTelemetry.json b/packages/core/src/shared/telemetry/vscodeTelemetry.json index 65c42a2b523..3487d7b4157 100644 --- a/packages/core/src/shared/telemetry/vscodeTelemetry.json +++ b/packages/core/src/shared/telemetry/vscodeTelemetry.json @@ -1131,6 +1131,17 @@ ], "passive": true }, + { + "name": "auth_signInPageOpened", + "description": "When the Amazon Q sign in page is opened and focused.", + "metadata": [ + { + "type": "source", + "required": true + } + ], + "passive": true + }, { "name": "function_call", "description": "Represents a function call. In most cases this should wrap code with a run(), then you can add context.", @@ -1207,6 +1218,17 @@ } ] }, + { + "name": "session_start", + "description": "Called when starting the plugin", + "metadata": [ + { + "type": "source", + "required": false + } + ], + "passive": true + }, { "name": "session_end", "description": "Called when stopping the IDE on a best effort basis", From 969f1eca3c7f4277414a0e85b0ce858487192bce Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Wed, 19 Feb 2025 12:29:30 -0500 Subject: [PATCH 4/6] check if existing connections caused isFirstUse discrepancy Before, on a users first use, it was technically possible for Q to indicate it was NOT the first use if somehow there were existing connections detected. A previous commit removed that case. But we want to now emit telemetry if we detected that we WOULD have run in to that case when not expected. Signed-off-by: nkomonen-amazon --- packages/core/src/auth/utils.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/core/src/auth/utils.ts b/packages/core/src/auth/utils.ts index 5529d9d80a3..9da35fa06e5 100644 --- a/packages/core/src/auth/utils.ts +++ b/packages/core/src/auth/utils.ts @@ -709,6 +709,13 @@ export class ExtensionUse { if (isAmazonQ()) { this.isFirstUseCurrentSession = true + if (hasExistingConnections()) { + telemetry.function_call.emit({ + result: 'Failed', + functionName: 'isFirstUse', + reason: 'UnexpectedConnections', + }) + } } else { // The variable in the store is not defined yet, fallback to checking if they have existing connections. this.isFirstUseCurrentSession = !hasExistingConnections() From 022d85832ecd305cca418f1edaa606010b9acb60 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Wed, 19 Feb 2025 12:02:55 -0500 Subject: [PATCH 5/6] telemetry(auth): emit metric when Auth webview loaded Problem: The inital problem is that when we focused chat, it returned succeeded even though it didn't have any idea if the UI was actually rendered to the user. Solution: This new solution will have the UI emit a telemetry metric once per session that indicates when the UI has loaded. It will also distinguish between the login and reauth page. The metric is `webview_load` and the `webviewName` field will distinguish between login and reauth NOTE: When Q chat itself has its UI ready we already emit a `webview_load` metric. So use all of these metrics to determine what the user actually saw Signed-off-by: nkomonen-amazon --- packages/core/src/login/webview/vue/backend.ts | 16 ++++++++++++++++ packages/core/src/login/webview/vue/login.vue | 2 ++ .../src/login/webview/vue/reauthenticate.vue | 3 +++ 3 files changed, 21 insertions(+) diff --git a/packages/core/src/login/webview/vue/backend.ts b/packages/core/src/login/webview/vue/backend.ts index 2b80013acca..9bc6c5ae339 100644 --- a/packages/core/src/login/webview/vue/backend.ts +++ b/packages/core/src/login/webview/vue/backend.ts @@ -61,6 +61,22 @@ export abstract class CommonAuthWebview extends VueWebview { return globals.regionProvider.getRegions().reverse() } + private didCall: { login: boolean; reauth: boolean } = { login: false, reauth: false } + public setUiReady(state: 'login' | 'reauth') { + // Prevent telemetry spam, since showing/hiding chat triggers this each time. + // So only emit once. + if (this.didCall[state]) { + return + } + + telemetry.webview_load.emit({ + passive: true, + webviewName: state, + result: 'Succeeded', + }) + this.didCall[state] = true + } + /** * This wraps the execution of the given setupFunc() and handles common errors from the SSO setup process. * diff --git a/packages/core/src/login/webview/vue/login.vue b/packages/core/src/login/webview/vue/login.vue index 4c9f65a2f6a..d21bf911802 100644 --- a/packages/core/src/login/webview/vue/login.vue +++ b/packages/core/src/login/webview/vue/login.vue @@ -368,6 +368,8 @@ export default defineComponent({ // Pre-select the first available login option await this.preselectLoginOption() await this.handleUrlInput() // validate the default startUrl + + await client.setUiReady('login') }, methods: { toggleItemSelection(itemId: number) { diff --git a/packages/core/src/login/webview/vue/reauthenticate.vue b/packages/core/src/login/webview/vue/reauthenticate.vue index 32e60b97049..3fc810f36b7 100644 --- a/packages/core/src/login/webview/vue/reauthenticate.vue +++ b/packages/core/src/login/webview/vue/reauthenticate.vue @@ -127,6 +127,9 @@ export default defineComponent({ this.doShow = true }, + async mounted() { + await client.setUiReady('reauth') + }, methods: { async reauthenticate() { client.emitUiClick('auth_reauthenticate') From b2b639abf3610b51f890a30c4fdd6bc39999a648 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Wed, 19 Feb 2025 13:28:11 -0500 Subject: [PATCH 6/6] fix unit tests Signed-off-by: nkomonen-amazon --- packages/core/src/shared/telemetry/util.ts | 9 +++++++-- packages/core/src/test/credentials/utils.test.ts | 10 +++++----- packages/core/src/test/shared/telemetry/util.test.ts | 11 ++++++++--- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/core/src/shared/telemetry/util.ts b/packages/core/src/shared/telemetry/util.ts index 5fbdfa8c967..91b6f89a902 100644 --- a/packages/core/src/shared/telemetry/util.ts +++ b/packages/core/src/shared/telemetry/util.ts @@ -217,9 +217,14 @@ let _hadClientIdOnStartup = false /** * Returns true if the ClientID existed before this session started */ -export const hadClientIdOnStartup = (globalState: GlobalState) => { +export const hadClientIdOnStartup = ( + globalState: GlobalState, + update = (globalState: GlobalState) => { + getClientId(globalState) + } +) => { // triggers the flow that will update the state, if not done already - getClientId(globalState) + update(globalState) return _hadClientIdOnStartup } diff --git a/packages/core/src/test/credentials/utils.test.ts b/packages/core/src/test/credentials/utils.test.ts index 7fc06aef897..ff4dc1046bc 100644 --- a/packages/core/src/test/credentials/utils.test.ts +++ b/packages/core/src/test/credentials/utils.test.ts @@ -11,23 +11,23 @@ import globals from '../../shared/extensionGlobals' describe('ExtensionUse.isFirstUse()', function () { let instance: ExtensionUse + const notHasExistingConnections = () => false beforeEach(async function () { instance = new ExtensionUse() - await globals.globalState.update(ExtensionUse.instance.isExtensionFirstUseKey, true) + await makeStateValueNotExist() }) it('is true only on first startup', function () { - assert.strictEqual(instance.isFirstUse(), true, 'Failed on first call.') - assert.strictEqual(instance.isFirstUse(), true, 'Failed on second call.') + assert.strictEqual(instance.isFirstUse(notHasExistingConnections), true, 'Failed on first call.') + assert.strictEqual(instance.isFirstUse(notHasExistingConnections), true, 'Failed on second call.') const nextStartup = nextExtensionStartup() - assert.strictEqual(nextStartup.isFirstUse(), false, 'Failed on new startup.') + assert.strictEqual(nextStartup.isFirstUse(notHasExistingConnections), false, 'Failed on new startup.') }) it('true when: (state value not exists + NOT has existing connections)', async function () { await makeStateValueNotExist() - const notHasExistingConnections = () => false assert.strictEqual( instance.isFirstUse(notHasExistingConnections), true, diff --git a/packages/core/src/test/shared/telemetry/util.test.ts b/packages/core/src/test/shared/telemetry/util.test.ts index 14eb2c4c3d9..27cfcb6b4f7 100644 --- a/packages/core/src/test/shared/telemetry/util.test.ts +++ b/packages/core/src/test/shared/telemetry/util.test.ts @@ -141,6 +141,11 @@ describe('getClientId', function () { } function setClientIdEnvVar(val: string | undefined) { + if (val === undefined) { + delete process.env[telemetryClientIdEnvKey] + return + } + process.env[telemetryClientIdEnvKey] = val } @@ -227,19 +232,19 @@ describe('getClientId', function () { describe('hadClientIdOnStartup', async function () { it('returns false when no existing clientId', async function () { const globalState = new GlobalState(new FakeMemento()) - assert.strictEqual(hadClientIdOnStartup(globalState), false) + assert.strictEqual(hadClientIdOnStartup(globalState, testGetClientId), false) }) it('returns true when existing env var clientId', async function () { const globalState = new GlobalState(new FakeMemento()) setClientIdEnvVar('aaa-111') - assert.strictEqual(hadClientIdOnStartup(globalState), true) + assert.strictEqual(hadClientIdOnStartup(globalState, testGetClientId), true) }) it('returns true when existing state clientId', async function () { const globalState = new GlobalState(new FakeMemento()) await globalState.update('telemetryClientId', 'bbb-222') - assert.strictEqual(hadClientIdOnStartup(globalState), true) + assert.strictEqual(hadClientIdOnStartup(globalState, testGetClientId), true) }) }) })