Skip to content

Commit

Permalink
fix(telemetry): use awsRegion instead of region, update guess function (
Browse files Browse the repository at this point in the history
#5025)

Problem: Some telemetry (auth_addConnection) is using a 'region' field instead of the built in 'awsRegion' field. This provides misleading results in telemetry, since 'awsRegion' is guessed based on toolkit usage.

Solution: Remove 'region' field and just use 'awsRegion'. Also, updating the 'awsRegion' guess function to account for amazon Q.

- Remove checking tree nodes from the region guess function, since it was unused functionality.
- If a region can't be determined, return undefined. It is up to the caller to define the default instead now.
  • Loading branch information
hayemaxi committed May 22, 2024
1 parent f9c0197 commit b69e7c7
Show file tree
Hide file tree
Showing 9 changed files with 76 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class AmazonQLoginWebview extends CommonAuthWebview {
credentialSourceId:
conn.startUrl === builderIdStartUrl ? 'awsId' : 'iamIdentityCenter',
credentialStartUrl: conn.startUrl,
region: conn.ssoRegion,
awsRegion: conn.ssoRegion,
authEnabledFeatures: this.getAuthEnabledFeatures(conn),
})
}
Expand Down Expand Up @@ -135,7 +135,7 @@ export class AmazonQLoginWebview extends CommonAuthWebview {
if (!auto) {
this.storeMetricMetadata({
credentialStartUrl: conn.startUrl,
region: conn.ssoRegion,
awsRegion: conn.ssoRegion,
authEnabledFeatures: this.getAuthEnabledFeatures(newConn),
})
}
Expand Down Expand Up @@ -171,7 +171,7 @@ export class AmazonQLoginWebview extends CommonAuthWebview {
return await this.ssoSetup('startCodeWhispererEnterpriseSetup', async () => {
this.storeMetricMetadata({
credentialStartUrl: startUrl,
region,
awsRegion: region,
credentialSourceId: 'iamIdentityCenter',
authEnabledFeatures: 'codewhisperer',
isReAuth: false,
Expand Down
20 changes: 12 additions & 8 deletions packages/core/src/login/webview/vue/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ export abstract class CommonAuthWebview extends VueWebview {
emitAuthMetric() {
// We shouldn't report startUrl or region if we aren't reporting IdC
if (this.metricMetadata.credentialSourceId !== 'iamIdentityCenter') {
this.metricMetadata.region = undefined
this.metricMetadata.credentialStartUrl = undefined
delete this.metricMetadata.awsRegion
delete this.metricMetadata.credentialStartUrl
}
telemetry.auth_addConnection.emit({
...this.metricMetadata,
Expand Down Expand Up @@ -248,15 +248,19 @@ export abstract class CommonAuthWebview extends VueWebview {
* Get metadata about the current auth for reauthentication telemetry.
*/
getMetadataForExistingConn(conn = AuthUtil.instance.conn): TelemetryMetadata {
if (isBuilderIdConnection(conn)) {
if (conn === undefined) {
return {}
}

if (isIdcSsoConnection(conn)) {
return {
credentialSourceId: 'awsId',
credentialSourceId: 'iamIdentityCenter',
credentialStartUrl: conn?.startUrl,
awsRegion: conn?.ssoRegion,
}
} else if (isIdcSsoConnection(conn)) {
} else if (isBuilderIdConnection(conn)) {
return {
credentialSourceId: 'iamIdentityCenter',
credentialStartUrl: (conn as SsoConnection).startUrl,
region: (conn as SsoConnection).ssoRegion,
credentialSourceId: 'awsId',
}
} else if (isIamConnection(conn)) {
return {
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/login/webview/vue/login.vue
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ export default defineComponent({
} else if (this.selectedLoginOption === LoginOption.ENTERPRISE_SSO) {
this.stage = 'SSO_FORM'
this.$nextTick(() => document.getElementById('startUrl')!.focus())
await client.storeMetricMetadata({ region: this.selectedRegion })
await client.storeMetricMetadata({ awsRegion: this.selectedRegion })
} else if (this.selectedLoginOption >= LoginOption.EXISTING_LOGINS) {
this.stage = 'AUTHENTICATING'
const selectedConnection =
Expand Down Expand Up @@ -509,7 +509,7 @@ export default defineComponent({
handleRegionInput(event: any) {
this.handleUrlInput() // startUrl validity depends on region, see handleUriInput() for details
void client.storeMetricMetadata({
region: event.target.value,
awsRegion: event.target.value,
})
void client.emitUiClick('auth_regionSelection')
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export class ToolkitLoginWebview extends CommonAuthWebview {
const metadata: TelemetryMetadata = {
credentialSourceId: 'iamIdentityCenter',
credentialStartUrl: startUrl,
region,
awsRegion: region,
}

if (this.isCodeCatalystLogin) {
Expand Down
43 changes: 26 additions & 17 deletions packages/core/src/shared/regions/regionProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ const localize = nls.loadMessageBundle()
import * as vscode from 'vscode'
import { getLogger } from '../logger'
import { Endpoints, loadEndpoints, Region } from './endpoints'
import { AWSTreeNodeBase } from '../treeview/nodes/awsTreeNodeBase'
import { regionSettingKey } from '../constants'
import { AwsContext } from '../awsContext'
import { getIdeProperties, isCloud9 } from '../extensionUtilities'
import { getIdeProperties, isAmazonQ, isCloud9 } from '../extensionUtilities'
import { ResourceFetcher } from '../resourcefetcher/resourcefetcher'
import { isSsoConnection } from '../../auth/connection'
import { Auth } from '../../auth'

export const defaultRegion = 'us-east-1'
export const defaultPartition = 'aws'
Expand Down Expand Up @@ -90,27 +91,35 @@ export class RegionProvider {
}

/**
* @param node node on current command.
* @returns heuristic for default region based on
* last touched region in explorer, wizard response, and node passed in.
* last touched region in auth, explorer, wizard response.
*/
public guessDefaultRegion(node?: AWSTreeNodeBase): string {
const explorerRegions = this.getExplorerRegions()
public guessDefaultRegion(): string | undefined {
const conn = Auth.instance.activeConnection
if (isAmazonQ() && isSsoConnection(conn)) {
// Only the current auth region makes sense for Amazon Q use cases.
return conn.ssoRegion
}

if (conn?.type === 'sso') {
return conn.ssoRegion
}

if (node?.regionCode) {
return node.regionCode
} else if (explorerRegions.length === 1) {
const explorerRegions = this.getExplorerRegions()
if (explorerRegions.length === 1) {
return explorerRegions[0]
} else if (this.lastTouchedRegion) {
}

if (this.lastTouchedRegion) {
return this.lastTouchedRegion
} else {
const lastWizardResponse = this.globalState.get<Region>('lastSelectedRegion')
if (lastWizardResponse && lastWizardResponse.id) {
return lastWizardResponse.id
} else {
return this.defaultRegionId
}
}

const lastWizardResponse = this.globalState.get<Region>('lastSelectedRegion')
if (lastWizardResponse && lastWizardResponse.id) {
return lastWizardResponse.id
}

return undefined
}

public setLastTouchedRegion(region: string | undefined) {
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/shared/telemetry/telemetryService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ export class DefaultTelemetryService {
commonMetadata.push({ Key: computeRegionKey, Value: this.computeRegion })
}
if (!event?.Metadata?.some((m: any) => m?.Key === regionKey)) {
commonMetadata.push({ Key: regionKey, Value: globals.regionProvider.guessDefaultRegion() })
const guessedRegion = globals.regionProvider.guessDefaultRegion()
commonMetadata.push({ Key: regionKey, Value: guessedRegion ?? AccountStatus.NotSet })
}

if (event.Metadata) {
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/shared/telemetry/vscodeTelemetry.json
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@
"description": "Comma-delimited list of scopes that user has."
},
{
"name": "region",
"name": "awsRegion",
"type": "string",
"description": "An AWS region."
}
Expand Down Expand Up @@ -1249,7 +1249,7 @@
"required": false
},
{
"type": "region",
"type": "awsRegion",
"required": false
},
{
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/shared/ui/common/regionSubmenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class RegionSubmenu<T> extends Prompter<RegionSubmenuResponse<T>> {
private readonly dataOptions?: ExtendedQuickPickOptions<T>,
private readonly regionOptions?: ExtendedQuickPickOptions<T>,
private readonly separatorLabel: string = 'Items',
private currentRegion = globals.regionProvider.guessDefaultRegion()
private currentRegion = globals.regionProvider.guessDefaultRegion() ?? globals.regionProvider.defaultRegionId
) {
super()
}
Expand Down
38 changes: 27 additions & 11 deletions packages/core/src/test/shared/regions/regionProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@

import assert from 'assert'
import { RegionProvider } from '../../../shared/regions/regionProvider'
import { AWSTreeNodeBase } from '../../../shared/treeview/nodes/awsTreeNodeBase'
import { createRegionPrompter } from '../../../shared/ui/common/region'
import { FakeMemento } from '../../fakeExtensionContext'
import { createQuickPickPrompterTester } from '../ui/testUtils'
import { createSsoProfile, createTestAuth } from '../../credentials/testUtil'
import { Auth } from '../../../auth/auth'
import * as extUtils from '../../../shared/extensionUtilities'
import sinon from 'sinon'

const regionCode = 'someRegion'
const serviceId = 'someService'
Expand Down Expand Up @@ -162,6 +165,10 @@ describe('RegionProvider', async function () {
})

describe('guessDefaultRegion', function () {
afterEach(() => {
sinon.restore()
})

it('sets default region to last region from prompter', async function () {
const regionProvider = new RegionProvider()
const originalRegion = regionProvider.guessDefaultRegion()
Expand Down Expand Up @@ -198,19 +205,28 @@ describe('RegionProvider', async function () {
assert.strictEqual(regionProvider.guessDefaultRegion(), 'us-east-2')
})

it('chooses AWS node region when more than one exists in explorer', async function () {
it('returns undefined when unable to determine last used region', function () {
const regionProvider = new RegionProvider(endpoints, new FakeMemento())
await regionProvider.updateExplorerRegions(['us-east-1', 'us-east-2'])
regionProvider.setLastTouchedRegion('us-west-1')
assert.strictEqual(regionProvider.guessDefaultRegion(), undefined)
})

it('returns undefined when no active amazon Q connection', function () {
const regionProvider = new RegionProvider(endpoints, new FakeMemento())
sinon.stub(extUtils, 'isAmazonQ').returns(true)

assert.strictEqual(regionProvider.guessDefaultRegion(), undefined)
})

it('returns connection region with active amazon Q connection', async function () {
const region = 'us-west-2'
const regionProvider = new RegionProvider(endpoints, new FakeMemento())
const auth = createTestAuth()
await auth.useConnection(await auth.createConnection(createSsoProfile({ ssoRegion: region })))

const node = new (class extends AWSTreeNodeBase {
public override readonly regionCode = 'us-west-2'
public constructor() {
super('')
}
})()
sinon.stub(Auth, 'instance').value(auth)
sinon.stub(extUtils, 'isAmazonQ').returns(true)

assert.strictEqual(regionProvider.guessDefaultRegion(node), 'us-west-2')
assert.strictEqual(regionProvider.guessDefaultRegion(), region)
})
})
})

0 comments on commit b69e7c7

Please sign in to comment.