telemetry(amazonq): Improving error handling and telemetry in unit test generation. #6187
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import { | |
| TelemetryHelper, | ||
| TestGenerationBuildStep, | ||
| testGenState, | ||
| tooManyRequestErrorMessage, | ||
| unitTestGenerationCancelMessage, | ||
| } from '../../../codewhisperer' | ||
| import { | ||
|
|
@@ -241,71 +242,84 @@ export class TestController { | |
| // eslint-disable-next-line unicorn/no-null | ||
| this.messenger.sendUpdatePromptProgress(data.tabID, null) | ||
| const session = this.sessionStorage.getSession() | ||
| const isCancel = data.error.uiMessage === unitTestGenerationCancelMessage | ||
| let telemetryErrorMessage = getTelemetryReasonDesc(data.error) | ||
| if (session.stopIteration) { | ||
| telemetryErrorMessage = getTelemetryReasonDesc(data.error.uiMessage.replaceAll('```', '')) | ||
| } | ||
| TelemetryHelper.instance.sendTestGenerationToolkitEvent( | ||
| session, | ||
| true, | ||
| isCancel ? 'Cancelled' : 'Failed', | ||
| session.startTestGenerationRequestId, | ||
| performance.now() - session.testGenerationStartTime, | ||
| telemetryErrorMessage, | ||
| data.error.statusCode ?? '0', | ||
| session.isCodeBlockSelected, | ||
| session.artifactsUploadDuration, | ||
| session.srcPayloadSize, | ||
| session.srcZipFileSize, | ||
| session.charsOfCodeAccepted, | ||
| session.numberOfTestsGenerated, | ||
| session.linesOfCodeAccepted, | ||
| session.charsOfCodeGenerated, | ||
| session.numberOfTestsGenerated, | ||
| session.linesOfCodeGenerated, | ||
| data.error.code | ||
| ) | ||
|
|
||
| if (session.stopIteration) { | ||
| // Error from Science | ||
| this.messenger.sendMessage(data.error.uiMessage.replaceAll('```', ''), data.tabID, 'answer') | ||
| } else { | ||
| isCancel | ||
| ? this.messenger.sendMessage(data.error.uiMessage, data.tabID, 'answer') | ||
| : this.sendErrorMessage(data) | ||
| } | ||
| await this.sessionCleanUp() | ||
| return | ||
| } | ||
| // Client side error messages | ||
| private sendErrorMessage(data: { | ||
| tabID: string | ||
| error: { uiMessage: string; message: string; code: string; statusCode: string } | ||
| }) { | ||
| const { error, tabID } = data | ||
|
|
||
| // If user reached monthly limit for builderId | ||
| if (error.code === 'CreateTestJobError') { | ||
| if (error.message.includes(CodeWhispererConstants.utgLimitReached)) { | ||
| getLogger().error('Monthly quota reached for QSDA actions.') | ||
| return this.messenger.sendMessage( | ||
| i18n('AWS.amazonq.featureDev.error.monthlyLimitReached'), | ||
| tabID, | ||
| 'answer' | ||
| ) | ||
| } | ||
| if (error.message.includes('Too many requests')) { | ||
| getLogger().error(error.message) | ||
| return this.messenger.sendErrorMessage(tooManyRequestErrorMessage, tabID) | ||
| } | ||
| } | ||
| if (isAwsError(error)) { | ||
| if (error.code === 'ThrottlingException') { | ||
| // TODO: use the explicitly modeled exception reason for quota vs throttle{ | ||
| getLogger().error(error.message) | ||
| this.messenger.sendErrorMessage(tooManyRequestErrorMessage, tabID) | ||
| return | ||
| } | ||
| // other service errors: | ||
| // AccessDeniedException - should not happen because access is validated before this point in the client | ||
| // ValidationException - shouldn't happen because client should not send malformed requests | ||
| // ConflictException - should not happen because the client will maintain proper state | ||
| // InternalServerException - shouldn't happen but needs to be caught | ||
| getLogger().error('Other error message: %s', error.message) | ||
| this.messenger.sendErrorMessage('', tabID) | ||
| return | ||
| } | ||
| // other unexpected errors (TODO enumerate all other failure cases) | ||
| getLogger().error('Other error message: %s', error.uiMessage) | ||
| this.messenger.sendErrorMessage('', tabID) | ||
| } | ||
|
|
||
| // This function handles actions if user clicked on any Button one of these cases will be executed | ||
|
|
@@ -714,13 +728,17 @@ export class TestController { | |
| // this.messenger.sendMessage('Accepted', message.tabID, 'prompt') | ||
| telemetry.ui_click.emit({ elementId: 'unitTestGeneration_acceptDiff' }) | ||
|
|
||
| getLogger().info( | ||
| `Generated unit tests are accepted for ${session.fileLanguage ?? 'plaintext'} language with jobId: ${session.listOfTestGenerationJobId[0]}, jobGroupName: ${session.testGenerationJobGroupName}, result: Succeeded` | ||
| ) | ||
| TelemetryHelper.instance.sendTestGenerationToolkitEvent( | ||
| session, | ||
| true, | ||
| 'Succeeded', | ||
| session.startTestGenerationRequestId, | ||
| session.latencyOfTestGeneration, | ||
| undefined, | ||
| '200', | ||
| session.isCodeBlockSelected, | ||
| session.artifactsUploadDuration, | ||
| session.srcPayloadSize, | ||
|
|
@@ -734,7 +752,6 @@ export class TestController { | |
| ) | ||
|
|
||
| await this.endSession(message, FollowUpTypes.SkipBuildAndFinish) | ||
| return | ||
|
|
||
| if (session.listOfTestGenerationJobId.length === 1) { | ||
|
|
@@ -842,6 +859,7 @@ export class TestController { | |
| session.startTestGenerationRequestId, | ||
| session.latencyOfTestGeneration, | ||
| undefined, | ||
| '200', | ||
| session.isCodeBlockSelected, | ||
| session.artifactsUploadDuration, | ||
| session.srcPayloadSize, | ||
|
|
@@ -853,16 +871,12 @@ export class TestController { | |
| session.numberOfTestsGenerated, | ||
| session.linesOfCodeGenerated | ||
| ) | ||
| telemetry.ui_click.emit({ elementId: 'unitTestGeneration_rejectDiff' }) | ||
| } | ||
|
|
||
| await this.sessionCleanUp() | ||
|
|
||
| // 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? |
||
| this.messenger.sendChatInputEnabled(data.tabID, true) | ||
| return | ||
| } | ||
|
|
@@ -1297,8 +1311,18 @@ export class TestController { | |
| 'Deleting output.log and temp result directory. testGenerationLogsDir: %s', | ||
| testGenerationLogsDir | ||
| ) | ||
| const outputLogPath = path.join(testGenerationLogsDir, 'output.log') | ||
| if (await fs.existsFile(outputLogPath)) { | ||
| await fs.delete(outputLogPath) | ||
| } | ||
| if ( | ||
| await fs | ||
| .stat(this.tempResultDirPath) | ||
| .then(() => true) | ||
| .catch(() => false) | ||
| ) { | ||
| await fs.delete(this.tempResultDirPath, { recursive: true }) | ||
| } | ||
| } | ||
|
|
||
| // TODO: return build command when product approves | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /*! | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| import { ToolkitError } from '../shared/errors' | ||
|
|
||
| export const technicalErrorCustomerFacingMessage = | ||
| 'I am experiencing technical difficulties at the moment. Please try again in a few minutes.' | ||
| const defaultTestGenErrorMessage = 'Amazon Q encountered an error while generating tests. Try again later.' | ||
| export class TestGenError extends ToolkitError { | ||
| 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. 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 |
||
| public uiMessage: string | ||
|
There was a problem hiding this comment. I think we don't need uiMessage. The 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 There was a problem hiding this comment. Can you look at existing code and figure out a way to make it work with 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. |
||
| ) { | ||
| super(error, { code }) | ||
| } | ||
| } | ||
| export class ProjectZipError extends TestGenError { | ||
| constructor(error: string) { | ||
| super(error, 'ProjectZipError', '400', defaultTestGenErrorMessage) | ||
| } | ||
| } | ||
| export class InvalidSourceZipError extends TestGenError { | ||
| constructor() { | ||
| super('Failed to create valid source zip', 'InvalidSourceZipError', '400', defaultTestGenErrorMessage) | ||
| } | ||
| } | ||
| export class CreateUploadUrlError extends TestGenError { | ||
| constructor(errorMessage: string, errorCode: string) { | ||
| super(errorMessage, 'CreateUploadUrlError', errorCode, technicalErrorCustomerFacingMessage) | ||
| } | ||
| } | ||
| export class UploadTestArtifactToS3Error extends TestGenError { | ||
| constructor(error: string, statusCode?: string) { | ||
| super(error, 'UploadTestArtifactToS3Error', statusCode ?? '400', technicalErrorCustomerFacingMessage) | ||
| } | ||
| } | ||
| export class CreateTestJobError extends TestGenError { | ||
| constructor(error: string, code: string) { | ||
| 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.
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() { | ||
| super( | ||
| 'Test generation failed. Amazon Q timed out.', | ||
| 'TestGenTimedOutError', | ||
| '500', | ||
| technicalErrorCustomerFacingMessage | ||
| ) | ||
| } | ||
| } | ||
| export class TestGenStoppedError extends TestGenError { | ||
| constructor() { | ||
| super('Unit test generation cancelled.', 'TestGenCancelled', '400', 'Unit test generation cancelled.') | ||
| } | ||
| } | ||
| export class TestGenFailedError extends TestGenError { | ||
| constructor(code: string, error?: string) { | ||
| super( | ||
| error ?? 'Test generation failed', | ||
| 'TestGenFailedError', | ||
| code, | ||
| error ?? technicalErrorCustomerFacingMessage | ||
| ) | ||
| } | ||
| } | ||
| export class ExportResultsArchiveError extends TestGenError { | ||
| constructor(error?: string, statusCode?: string) { | ||
| super( | ||
| error ?? 'Test generation failed', | ||
| 'ExportResultsArchiveError', | ||
| statusCode ?? '400', | ||
| technicalErrorCustomerFacingMessage | ||
| ) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is using featureDev message, is that intentional?
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.
Yes its intentional, Its a generic message for Q agent.