Skip to content

Conversation

@ctidd
Copy link
Contributor

@ctidd ctidd commented Oct 9, 2024

Types of changes

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

Description

Problem: Feature dev instrumentation 1) records all failures as FeatureDevException, losing information about the specific exception type, and 2) lacks annotation of reasonDesc and the operation related to the error. (By operation, although we do capture amazonq_codeGenerationInvoke, we're missing the subprocess name such as StartCodeGeneration.) As a result, we do not have insight into feature dev failures in JetBrains.

Solution: 1) Update exception modeling pattern for feature dev functionality to derive subclasses of FeatureDevException, providing a meaningful class name when inspected as error reason. 2) On FeatureDevException, accept an operation and desc to provide further annotation (merged when logged as reasonDesc).

This change will have conflicts with #4938 and #4949. I'll rebase on top of those and reconcile any drift in a subsequent revision.

Checklist

  • My code follows the code style of this project
  • I have added tests to cover my changes
  • [N/A] A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
  • [N/A] 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.

@ctidd ctidd force-pushed the ctidd/feature-dev-exceptions branch 4 times, most recently from 512f66e to 05b61d7 Compare October 9, 2024 22:46
Comment on lines 23 to 24
desc -> String.format("%s: %s", this.operation, desc)
else -> String.format("%s", this.operation)
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
desc -> String.format("%s: %s", this.operation, desc)
else -> String.format("%s", this.operation)
desc -> "$description: $desc"
else -> operation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated as:

desc -> "$operation | Description: $desc"
else -> operation

* - Each failure is annotated based on className, operation, and a short desc. Use the `reason()` and `reasonDesc()` members for instrumentation.
* - To throw an exception without modeling, throw FeatureDevException directly.
*/
open class FeatureDevException(override val message: String?, open val operation: String, open val desc: String?, override val cause: Throwable? = null)
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
open class FeatureDevException(override val message: String?, open val operation: String, open val desc: String?, override val cause: Throwable? = null)
open class FeatureDevException(override val message: String?, val operation: String, val desc: String?, override val cause: Throwable? = null)

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for cleaning this up

)
}
else -> {
var msg = createUserFacingErrorMessage("$FEATURE_NAME request failed: ${err.message ?: err.cause?.message}")
Copy link
Contributor

Choose a reason for hiding this comment

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

var can be val

@ctidd ctidd force-pushed the ctidd/feature-dev-exceptions branch from 05b61d7 to 8bdc09d Compare October 9, 2024 23:14
@ctidd ctidd marked this pull request as ready for review October 9, 2024 23:15
@ctidd ctidd requested review from a team as code owners October 9, 2024 23:15
@ctidd ctidd force-pushed the ctidd/feature-dev-exceptions branch 2 times, most recently from 372c006 to ac495d8 Compare October 10, 2024 00:00
@ctidd ctidd force-pushed the ctidd/feature-dev-exceptions branch from ac495d8 to 1ec073f Compare October 10, 2024 00:01
Copy link
Contributor

@chengoramazon chengoramazon left a comment

Choose a reason for hiding this comment

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

Just FYI: feels like metadata.metricName already play the role of operation, so have operation in the description maybe redundant. Sharing some metrics links offline.

@rli rli merged commit aa3ba5e into aws:main Oct 10, 2024
8 of 10 checks passed
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.

4 participants