-
Notifications
You must be signed in to change notification settings - Fork 747
fix(amazonq): listAvailableProfile calls are throttled when users have 2+ IDE instances #7134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
0c81733
9a39537
891643b
c727a9c
aedd1de
923785d
b503b8e
a03e306
ab3f345
2a32743
a32701f
e13221a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -28,6 +28,7 @@ import { parse } from '@aws-sdk/util-arn-parser' | |||
| import { isAwsError, ToolkitError } from '../../shared/errors' | ||||
| import { telemetry } from '../../shared/telemetry/telemetry' | ||||
| import { localize } from '../../shared/utilities/vsCodeUtils' | ||||
| import { waitUntil } from '../../shared/utilities/timeoutUtils' | ||||
|
|
||||
| // TODO: is there a better way to manage all endpoint strings in one place? | ||||
| export const defaultServiceConfig: CodeWhispererConfig = { | ||||
|
|
@@ -42,13 +43,41 @@ const endpoints = createConstantMap({ | |||
| }) | ||||
|
|
||||
| /** | ||||
| * 'user' -> users change the profile through Q menu | ||||
| * 'user' -> users change the profile through Q menu` | ||||
| * 'auth' -> users change the profile through webview profile selector page | ||||
| * 'update' -> plugin auto select the profile on users' behalf as there is only 1 profile | ||||
| * 'reload' -> on plugin restart, plugin will try to reload previous selected profile | ||||
| */ | ||||
| export type ProfileSwitchIntent = 'user' | 'auth' | 'update' | 'reload' | ||||
|
|
||||
| // Only "valid" state will have non null profiles | ||||
| type CachedApiResultWithLock = { | ||||
| // Lock | ||||
| isAcquired: boolean | ||||
| // Metadata | ||||
| result: SuccessResult | FailureResult | undefined | ||||
| } | ||||
|
|
||||
| interface ApiResult { | ||||
| type: 'success' | 'failure' | ||||
| timestamp: number | ||||
| } | ||||
|
|
||||
| interface SuccessResult extends ApiResult { | ||||
| type: 'success' | ||||
| profiles: RegionProfile[] | ||||
| } | ||||
|
|
||||
| interface FailureResult extends ApiResult { | ||||
| type: 'failure' | ||||
| // Error type can't be serialized | ||||
| errorMsg: string | ||||
| } | ||||
|
|
||||
| function now(): number { | ||||
| return globals.clock.Date.now() | ||||
| } | ||||
|
|
||||
| export class RegionProfileManager { | ||||
| private static logger = getLogger() | ||||
| private _activeRegionProfile: RegionProfile | undefined | ||||
|
|
@@ -110,7 +139,56 @@ export class RegionProfileManager { | |||
| if (conn === undefined || !isSsoConnection(conn)) { | ||||
| return [] | ||||
| } | ||||
|
|
||||
| // WaitUntil lock is acquired, the API tps is low, server can't afford multiple IDE instances making the calls simultaneiously | ||||
| const cached = await waitUntil( | ||||
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| async () => { | ||||
| const lock = await this.tryAcquireLock() | ||||
| RegionProfileManager.logger.info(`try obtaining cache lock %s`, lock) | ||||
| if (lock) { | ||||
| return lock | ||||
| } | ||||
| }, | ||||
| { timeout: 15000, interval: 1500, truthy: true } | ||||
| ) | ||||
|
|
||||
| RegionProfileManager.logger.info(`obtained cache lock %s`, cached) | ||||
|
|
||||
| const availableProfiles: RegionProfile[] = [] | ||||
| // Load cache and use it directly if it's in "valid" state | ||||
| // If it's pending, keep waiting until the ongoing pull finishes its job | ||||
|
|
||||
| const now = globals.clock.Date.now() | ||||
| if (cached?.result && now - cached.result.timestamp < 60000) { | ||||
| RegionProfileManager.logger.info( | ||||
| `cache hit, duration=%s, previous listAvailableProfile result=%s`, | ||||
| now - cached.result.timestamp, | ||||
| cached | ||||
| ) | ||||
|
|
||||
| // Release lock so that other ide instances can continue | ||||
| await this.releaeLock(cached) | ||||
|
|
||||
| if (cached.result.type === 'success') { | ||||
| availableProfiles.push(...(cached.result as SuccessResult).profiles) | ||||
| this._profiles = availableProfiles | ||||
| return availableProfiles | ||||
| } else if (cached.result.type === 'failure') { | ||||
| const errorMsg = (cached.result as FailureResult).errorMsg | ||||
| throw new Error(errorMsg) | ||||
| } | ||||
| } | ||||
|
|
||||
| if (cached?.result && now - cached.result.timestamp > 60000) { | ||||
| RegionProfileManager.logger.info( | ||||
| `listAvailableProfile cache hit but cached value is stale, invoking service API to pull the latest response` | ||||
| ) | ||||
| } else { | ||||
| RegionProfileManager.logger.info( | ||||
| `listAvailableProfile cache miss, invoking service API to pull the latest response` | ||||
| ) | ||||
| } | ||||
|
|
||||
| for (const [region, endpoint] of endpoints.entries()) { | ||||
| const client = await this.createQClient(region, endpoint, conn as SsoConnection) | ||||
| const requester = async (request: CodeWhispererUserClient.ListAvailableProfilesRequest) => | ||||
|
|
@@ -138,12 +216,17 @@ export class RegionProfileManager { | |||
| } catch (e) { | ||||
| const logMsg = isAwsError(e) ? `requestId=${e.requestId}; message=${e.message}` : (e as Error).message | ||||
| RegionProfileManager.logger.error(`failed to listRegionProfile: ${logMsg}`) | ||||
| // Should update cache even for failure path | ||||
| await this.updateCacheWithError(e as Error) | ||||
| throw e | ||||
| } | ||||
|
|
||||
| RegionProfileManager.logger.info(`available amazonq profiles: ${availableProfiles.length}`) | ||||
| } | ||||
|
|
||||
| // Update cache and save the result | ||||
| await this.updateCacheWithResult(availableProfiles) | ||||
|
|
||||
| this._profiles = availableProfiles | ||||
| return availableProfiles | ||||
| } | ||||
|
|
@@ -367,4 +450,63 @@ export class RegionProfileManager { | |||
|
|
||||
| return c | ||||
| } | ||||
|
|
||||
| // Should reset cache when users signout in case users want to change to a different connection | ||||
| async resetCache() { | ||||
| await globals.globalState.update('aws.amazonq.regionProfiles.cachedResult', { | ||||
| isAcquired: false, | ||||
| result: undefined, | ||||
| }) | ||||
| } | ||||
|
|
||||
| private async tryAcquireLock(): Promise<CachedApiResultWithLock | undefined> { | ||||
|
||||
| export class FileSystemState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw Justin, i'm going to use #7151 to replace this pr for this purpose (if everything works just fine there), because the diff are pretty large so would be easier to just have another PR.
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ export type globalKey = | |
| | 'aws.toolkit.lsp.manifest' | ||
| | 'aws.amazonq.customization.overrideV2' | ||
| | 'aws.amazonq.regionProfiles' | ||
| | 'aws.amazonq.regionProfiles.cachedResult' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not set a flag on the items in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this is to memorize previous selected option, so we will need a separate structure to cache previous api call response
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
but that could be a flag on the item(s) in the structure. why is a separate structure needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes sense, i will reuse the existing one then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @justinmk3 i encounter an issue or say concern when i am doing this exercise 3e50145#diff-f5716036a70a8b212e9dae829ec09e33731f181c173a3f2b9ad9d361d51af14bR54-R56, if I want to reuse the same global key, i might need to restructure the schema, in such case, users who already selected profile update their plugin, they might need to re-select the profile again. Given the sev2s we have right now, I am concerned if we want to do this now or is there another way to deal with such scenario?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely want to avoid a restructure. If a restructure is required, then I guess we need a new globalState field.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. noted |
||
| // Deprecated/legacy names. New keys should start with "aws.". | ||
| | '#sessionCreationDates' // Legacy name from `ssoAccessTokenProvider.ts`. | ||
| | 'CODECATALYST_RECONNECT' | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.