telemetry(amazonq): Improving error handling and telemetry in unit test generation. #6187
Conversation
|
laileni-aws
changed the title
laileni-aws
changed the title
laileni-aws
changed the title
| error: string, | ||
| code: string, | ||
| public statusCode: string, | ||
| public customerFacingMessage: string |
There was a problem hiding this comment.
Name it uiMessage. This is really a special case, and will lead to confusion and hard-to-follow code, and inconsistent UX. Why can't customers see the actual error message? This will cause drift and confusion, and adds steps to troubleshooting.
There was a problem hiding this comment.
Why can't customers see the actual error message
Unfortunately error messages received from the backend is not aligning to the UX messages. So IDE need to handle this explicitly.
I will update customerFacingMessage to uiMessage
| getLogger().info( | ||
| session.fileLanguage ?? 'plaintext', | ||
| session.listOfTestGenerationJobId[0], | ||
| session.testGenerationJobGroupName, | ||
| 'Succeeded', | ||
| AuthUtil.instance.startUrl, | ||
| '200' | ||
| ) |
There was a problem hiding this comment.
Does this format work? I think only the first arg will be logged
|
|
||
| this.messenger.sendMessage(message, data.tabID, 'prompt') | ||
| this.messenger.sendMessage(`Unit test generation workflow is completed.`, data.tabID, 'answer') | ||
| // this.messenger.sendMessage(`Unit test generation workflow is completed.`, data.tabID, 'answer') |
There was a problem hiding this comment.
Existing public plugin does not reach this part of code as IDE throws an error at sessionCleanUp
There was a problem hiding this comment.
Now that you fixed it, do we still want to comment this out?
There was a problem hiding this comment.
We need to check whether we need to have this text after Accept or Reject action?
As JB does show this text after Accept/Reject but VSC don't.
We can add this accordingly.
| if (error.message.includes(CodeWhispererConstants.utgLimitReached)) { | ||
| getLogger().error('Monthly quota reached for QSDA actions.') | ||
| return this.messenger.sendMessage( | ||
| i18n('AWS.amazonq.featureDev.error.monthlyLimitReached'), |
There was a problem hiding this comment.
This is using featureDev message, is that intentional?
There was a problem hiding this comment.
Yes its intentional, Its a generic message for Q agent.
"AWS.amazonq.featureDev.error.monthlyLimitReached": "You've reached the monthly quota for Amazon Q Developer's agent capabilities. You can try again next month. For more information on usage limits, see the <a href=\"https://aws.amazon.com/q/developer/pricing/\" target=\"_blank\">Amazon Q Developer pricing page</a>.",
| 'answer' | ||
| ) | ||
| } else { | ||
| getLogger().error('Too many requests.') |
There was a problem hiding this comment.
Does this path always mean "Too many requests"? I would have expected something more generic
| @@ -736,10 +747,10 @@ export class TestController { | |||
| isSupportedLanguage: true, | |||
| credentialStartUrl: AuthUtil.instance.startUrl, | |||
| result: 'Succeeded', | |||
| httpStatusCode: '200', | |||
There was a problem hiding this comment.
Should this be a number or a string? I think in the previous block you are using a number type
There was a problem hiding this comment.
In MetricBase, It was mentioned as string readonly httpStatusCode?: string.
I am making change from
httpStatusCode: data.error.statusCode ?? 0
to
httpStatusCode: data.error.statusCode ?? '0',
|
|
||
| this.messenger.sendMessage(message, data.tabID, 'prompt') | ||
| this.messenger.sendMessage(`Unit test generation workflow is completed.`, data.tabID, 'answer') | ||
| // this.messenger.sendMessage(`Unit test generation workflow is completed.`, data.tabID, 'answer') |
There was a problem hiding this comment.
Now that you fixed it, do we still want to comment this out?
| @@ -343,6 +345,7 @@ export function throwIfCancelled(scope: CodeWhispererConstants.CodeAnalysisScope | |||
| export async function uploadArtifactToS3( | |||
There was a problem hiding this comment.
Can we create a separate function for testGen in testGenHandler.ts?
There was a problem hiding this comment.
I want to cover these changes as part of Code Refactoring PR.
I added a task for this in milestone doc as there is some good amount of refactoring, needs to be done.
| } else { | ||
| errorMessage = errorDesc ?? defaultMessage | ||
| } | ||
| throw isCodeScan ? new UploadArtifactToS3Error(errorMessage) : new UploadTestArtifactToS3Error(errorMessage) |
There was a problem hiding this comment.
It looks like error description can have either 4xx or 5xx. But UploadTestArtifactToS3Error always has 500 hardcoded?
There was a problem hiding this comment.
Updated this with new UploadTestArtifactToS3Error(errorMessage, (error as any).statusCode)
| error: string, | ||
| code: string, | ||
| public statusCode: string, | ||
| public uiMessage: string |
There was a problem hiding this comment.
I think we don't need uiMessage. The ToolkitError.message field is explicitly documented as "potentially shown to the user". You can store the lower-level message on ToolkitError.details
This has far-reaching benefits. It allows all the existing architecture to treat these standard fields in a consistent way.
There was a problem hiding this comment.
Looks like details has different type readonly details?: Record<string, unknown>
There was a problem hiding this comment.
Can you look at existing code and figure out a way to make it work with details. It shouldn't matter that the type isn't exactly the same.
There was a problem hiding this comment.
Saw the usage of this but I think we can use the existing way as we use same structure for Reviewbird error handling too.
This change neither impacts other features nor functionality or telemetry.
| super(error, 'CreateTestJobError', code, technicalErrorCustomerFacingMessage) | ||
| } | ||
| } | ||
| export class TestGenTimedOutError extends TestGenError { |
There was a problem hiding this comment.
What is the plan here? Maintaining one-to-one errors with all the service-side errors is not maintainable. Can you find something more generalizable.
There was a problem hiding this comment.
TestGenTimedOutError is client side error if plugin did not get response for limited time we throw this error. This helps us understand the number of requests timed out from generic backend errors.
There was a problem hiding this comment.
I am not asking about this error only, I'm referring to all of the errors in this file. It is not maintainable to create special errors for every service-side error.
| constructor( | ||
| error: string, | ||
| code: string, | ||
| public statusCode: string, |
There was a problem hiding this comment.
Thought of using number but in our Telemetry statusCode is declared as String.
aws-toolkit-common
If required I can use number here and can convert to string while emitting?
There was a problem hiding this comment.
Addressed this as part of this commit
Implementation:
httpStatusCodefield toamazonq_utgGenerateTestsevent.feature/xbranches will not be squash-merged at release time.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.