Skip to content

telemetry(amazonq): implement codewhisperer_clientComponentLatency #7234

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 9, 2025
Merged

telemetry(amazonq): implement codewhisperer_clientComponentLatency #7234

merged 1 commit into from
May 9, 2025

Conversation

@jpinkney-aws
Copy link
Contributor

@jpinkney-aws jpinkney-aws commented May 6, 2025

Problem

the only metric it looks like we're missing for inline on the vscode side is codewhisperer_clientComponentLatency

Solution

codewhisperer_clientComponentLatency uses a very similar implementation as before the only differences are:

  1. codewhispererCredentialFetchingLatency is no longer relevant because the token is always injected into the language server and it doesn't need to build the client on demand like before.
    • This causes the preprocessing latency to decrease, because that used to contain the time it takes to fetch the credentials
  2. postProcessing latency is way lower because once we get the result vscode instantly displays it -- we no longer have control of that

example metric now:

2025-05-06 11:53:59.858 [debug] telemetry: codewhisperer_clientComponentLatency {
  Metadata: {
    codewhispererAllCompletionsLatency: '792.7122090000048',
    codewhispererCompletionType: 'Line',
    codewhispererCredentialFetchingLatency: '0',
    codewhispererCustomizationArn: 'arn:aws:codewhisperer:us-east-1:12345678910:customization/AAAAAAAAAA',
    codewhispererEndToEndLatency: '792.682249999998',
    codewhispererFirstCompletionLatency: '792.6440000000002',
    codewhispererLanguage: 'java',
    codewhispererPostprocessingLatency: '0.019500000002153683',
    codewhispererPreprocessingLatency: '0.007166999996115919',
    codewhispererRequestId: 'XXXXXXXXXXXXXXXXXXXXXXXXXXX',
    codewhispererTriggerType: 'AutoTrigger',
    credentialStartUrl: 'https://XXXXX.XXXXX.com/start',
    awsAccount: 'not-set',
    awsRegion: 'us-east-1'
  },
  Value: 1,
  Unit: 'None',
  Passive: true
}
  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jpinkney-aws jpinkney-aws requested a review from a team as a code owner May 6, 2025 15:55
@github-actions
Copy link

github-actions bot commented May 6, 2025

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

opieter-aws
opieter-aws approved these changes May 6, 2025
View reviewed changes
Copy link
Contributor

@opieter-aws opieter-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some unit tests need updating with these changes

Hweinstock
Hweinstock reviewed May 6, 2025
View reviewed changes
packages/amazonq/src/app/inline/recommendationService.ts Outdated
TelemetryHelper.instance.setSdkApiCallStartTime()

// Handle first request
const firstResult: InlineCompletionListWithReferences = await languageClient.sendRequest(
Copy link
Contributor

@Hweinstock Hweinstock May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to work here, but do we need to encrypt this request?

Copy link
Contributor Author

@jpinkney-aws jpinkney-aws May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 good question, I guess I probably should

Copy link
Contributor Author

@jpinkney-aws jpinkney-aws May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm asking internally for more clarification, but judging from the types it looks like inline can't be encrypted

packages/amazonq/src/app/inline/recommendationService.ts Outdated

// Set telemetry data for the first response
TelemetryHelper.instance.setSdkApiCallEndTime()
TelemetryHelper.instance.setFirstResponseRequestId(firstResult.sessionId)
Copy link
Contributor

@Hweinstock Hweinstock May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this requestId intended to be for the backend request? Based on my understanding from here it sounds like sessionId is something else?

Copy link
Contributor Author

@jpinkney-aws jpinkney-aws May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh true, I might have to update it to be the first recommendation id

Copy link
Contributor Author

@jpinkney-aws jpinkney-aws May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT itemId might be the one we're looking for?

Copy link
Contributor

@Hweinstock Hweinstock May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be surprised if this information isn't part of the response since I haven't seen the backend requestID included in any Flare responses so far. This might mean some of this telemetry will need to live in flare or we sacrifice having requestId for now.

This issue of Flare having some telemetry fields, and the IDE having others is becoming increasingly problematic but there's no time/bandwidth for someone to look into it atm. :(

Copy link
Contributor Author

@jpinkney-aws jpinkney-aws May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost all of the telemetry for amazon q now lives on the flare side and will have access to that request id, I think this is the rare exception because it's purely tracking client side latency

@jpinkney-aws jpinkney-aws force-pushed the clientComponentLatency branch from 708b841 to 363779d Compare May 7, 2025 12:03
@jpinkney-aws jpinkney-aws force-pushed the clientComponentLatency branch from 70c2505 to cd82ada Compare May 9, 2025 02:04
@jpinkney-aws jpinkney-aws merged commit 6995107 into aws:feature/flare-inline May 9, 2025
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Hweinstock Hweinstock Hweinstock left review comments

@opieter-aws opieter-aws opieter-aws approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jpinkney-aws @Hweinstock @opieter-aws