Skip to content
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

fix(amazonq): Error handling and telemetry for Unit test generation. #5192

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

laileni-aws
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Description

  • Adding 4XX vs 5XX httpStatusCode field to amazonq_utgGenerateTests event.
  • Improving error handling in unit test generation.

Checklist

  • My code follows the code style of this project
  • I have added tests to cover my changes
  • A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
  • I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

@laileni-aws laileni-aws requested review from a team as code owners December 11, 2024 19:21
"UploadTestArtifactToS3Error: $errorMessage",
"403",
"UploadTestArtifactToS3Error",
message("testgen.error.generic_technical_error_message")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
"CreateUploadUrlError: $errorMessage",
"500",
"CreateUploadUrlError",
message("testgen.error.generic_technical_error_message")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
@laileni-aws laileni-aws marked this pull request as draft December 11, 2024 19:43

package software.aws.toolkits.jetbrains.services.codewhisperer.codetest

import software.aws.toolkits.resources.message

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

Remove deprecated symbol import
throw CodeTestException(message("codewhisperer.codescan.file_too_large_telemetry"), "400", "ProjectZipError")

internal fun cannotFindFile(errorMessage: String, filepath: String): Nothing =
error(message("codewhisperer.codescan.file_not_found", filepath, errorMessage))

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
@laileni-aws laileni-aws marked this pull request as ready for review December 13, 2024 22:34
Comment on lines 135 to 137
throw codeTestServerException(
"CreateTestJobError: $errorMessage",
"400",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be renamed? It mentions server exception but uses 4xx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this needs to be e.statusCode() but for now this can be 5XX.

} catch (e: Exception) {
LOG.error(e) { "Failed to create test generation job" }
// TODO: Not able to emit e.statusCode directly as statusCode is private property
// Cannot access 'statusCode': it is invisible (private in a supertype) in 'ThrottlingException'
Copy link
Contributor

Choose a reason for hiding this comment

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

try statusCode()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tried statusCode(), no luck.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

@@ -173,7 +190,13 @@ class CodeWhispererUTGChatManager(val project: Project, private val cs: Coroutin
}
// update test summary card
} else {
throw Exception(message("testgen.message.failed"))
// If job status is Completed and has no ShortAnswer then there might be some issue in the backend.
throw codeTestServerException(
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you verified that this works? It looks like you are throwing a function which throws an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified in Kibana and its working fine. Attached related links in Slack.


fun codeTestServerException(
errorMessage: String,
statusCode: String?,
Copy link
Contributor

Choose a reason for hiding this comment

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

status code should always be a number

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

@laileni-aws laileni-aws Dec 13, 2024

Choose a reason for hiding this comment

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

statusCode is string in MetricBase in VSC so to align with VSC and JB I initialized this to String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

"TestGenFailedError: " + message("testgen.message.failed"),
"500",
"TestGenFailedError",
message("testgen.error.generic_technical_error_message")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
message("testgen.error.maximum_generations_reach")

e is CodeTestException -> e.uiMessage
e is JsonParseException -> message("testgen.error.generic_technical_error_message")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
override val message: String?,
val statusCode: String? = "400",
val code: String? = "DefaultError",
val uiMessage: String? = message(

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
Comment on lines 26 to 33
internal fun cannotFindValidFile(errorMessage: String): Nothing =
throw CodeTestException(errorMessage, "400", "ProjectZipError")

internal fun cannotFindBuildArtifacts(errorMessage: String): Nothing =
throw CodeTestException(errorMessage, "400", "ProjectZipError")

internal fun invalidSourceZipError(): Nothing =
throw CodeTestException(message("codewhisperer.codescan.invalid_source_zip_telemetry"), "400", "InvalidSourceZipError")
Copy link
Contributor

Choose a reason for hiding this comment

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

i am pretty sure none of these are actually error code 400, please use the actual error code from the service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are client side errors so there wont be any error codes from service

Copy link
Contributor

Choose a reason for hiding this comment

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

if it is client side error then why would we have an http status code

Copy link
Contributor Author

@laileni-aws laileni-aws Dec 14, 2024

Choose a reason for hiding this comment

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

Its a request from manager to see the statusCodes accordingly in the Kibana to differentiate the errors.

Comment on lines 532 to 533
e is CodeTestException && e.statusCode == "400" &&
e.message?.startsWith("CreateTestJobError: Maximum") == true ->
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a better way of doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here Quota exceeded is subset of Throttling exception so opted for checking the error message.

@@ -517,8 +554,9 @@ class CodeWhispererUTGChatManager(val project: Project, private val cs: Coroutin
jobGroup = session.testGenerationJobGroupName,
jobId = session.testGenerationJob,
result = if (e.message == message("testgen.message.cancelled")) MetricResult.Cancelled else MetricResult.Failed,
reason = e.javaClass.name,
reasonDesc = e.message,
reason = (e as CodeTestException).code ?: "DefaultError",
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we just let it be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be but placing this under the bucket of DefaultError to be consistent in Kibana.

@laileni-aws laileni-aws requested review from ctlai95 and rli December 14, 2024 00:00

e is CodeTestException -> e.uiMessage
e is JsonParseException -> message("testgen.error.generic_technical_error_message")
else -> message("testgen.error.generic_error_message")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
Comment on lines 132 to 133
e is ThrottlingException -> e.statusCode()
else -> 500
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
e is ThrottlingException -> e.statusCode()
else -> 500
e is SdkServiceException -> e.statusCode()
else -> ??? // not a service exception, so it would be ???

throw CodeTestException(
"CreateTestJobError: $errorMessage",
"CreateTestJobError",
message("testgen.error.generic_technical_error_message")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
) : RuntimeException()

internal fun noFileOpenError(): Nothing =
throw CodeTestException(message("codewhisperer.codescan.no_file_open"), "ProjectZipError")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
throw CodeTestException(message("codewhisperer.codescan.no_file_open"), "ProjectZipError")

internal fun fileTooLarge(): Nothing =
throw CodeTestException(message("codewhisperer.codescan.file_too_large_telemetry"), "ProjectZipError")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
throw CodeTestException(errorMessage, "ProjectZipError")

internal fun invalidSourceZipError(): Nothing =
throw CodeTestException(message("codewhisperer.codescan.invalid_source_zip_telemetry"), "InvalidSourceZipError")

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
throw CodeTestException(message("codewhisperer.codescan.invalid_source_zip_telemetry"), "InvalidSourceZipError")

fun testGenStoppedError(): Nothing =
throw CodeTestException(message("testgen.message.cancelled"), "TestGenCancelled", message("testgen.message.cancelled"))

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
throw CodeTestException(message("codewhisperer.codescan.invalid_source_zip_telemetry"), "InvalidSourceZipError")

fun testGenStoppedError(): Nothing =
throw CodeTestException(message("testgen.message.cancelled"), "TestGenCancelled", message("testgen.message.cancelled"))

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
@laileni-aws
Copy link
Contributor Author

/retryBuilds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants