fix(gumby): modify metric to track errors #677
Conversation
| @@ -3538,8 +3538,10 @@ | |||
| "type": "codeTransformSessionId", | |||
| "required": true | |||
| }, | |||
| { "type": "result", "required": true }, | |||
| { "type": "reason", "required": true } | |||
There was a problem hiding this comment.
We do emit result and reason for this metric, and I have no plan to change this, but I removed these here because I noticed that they are missing in many other metrics, so I want to be consistent. Let me know if this is not best practice.
There was a problem hiding this comment.
EDIT: Added them back.
There was a problem hiding this comment.
Moving forward we will want to have reason/result in all of our metrics, but since we are already working on revamping the metric schema and the majority of these metrics will be replaced, I think it's fine to not back-fill the fields at the moment.
There was a problem hiding this comment.
Not just relate to this case
if any field is required true, that means it needs to be populated. if you are concerned about consistency, required:true is nice way to force it.
| "type": "reason", | ||
| "required": false | ||
| "type": "codeTransformRuntimeError", | ||
| "required": true |
There was a problem hiding this comment.
Required here because when mvn command fails we should always have an error message to emit.
There was a problem hiding this comment.
Same as my other comment above
There was a problem hiding this comment.
If IntelliJ uses the codeTransformRuntimeError field already then we should use this field. If not, we should move the error into the reason field if possible.
There was a problem hiding this comment.
What kind of error message is expected here? Do you have any sample format.
Telemetry metrics are for overall analysis and we don't want to have long texts. maybe a gist in reason is good practice.
if you need long error messages, we can dump it to cloudwatch logs
| { "type": "reason", "required": true } | ||
| { | ||
| "type": "codeTransformRuntimeError", | ||
| "required": false |
There was a problem hiding this comment.
Not required here because we use this codeTransform_isDoubleClickedToTriggerInvalidProject in many spots, in some of which it is not necessary to include any type of error message.
There was a problem hiding this comment.
Note that codeTransformRuntimeError is an existing field we have (already defined as a string)
There was a problem hiding this comment.
The reason field should be sufficient to tell you why it failed. Is there a reason you specifically need this field? Can the value that would go in to this field be put in to reason instead?
There was a problem hiding this comment.
If IntelliJ uses the codeTransformRuntimeError field already then we should use this field. If not, we should move the error into the reason field if possible.
| @@ -3890,8 +3892,8 @@ | |||
| "required": true | |||
| }, | |||
| { | |||
| "type": "reason", | |||
| "required": false | |||
There was a problem hiding this comment.
See first comment at very top.
There was a problem hiding this comment.
I think we are trying to keep result and reason as part of our new standard. What is the goal here from removing this?
|
Just discussed with AWS Toolkit team on metric improvement, and moving forward we want to avoid using code transform specific fields, but since codeTransfromRuntimeError is an existing field, to align with other current error metrics, we can keep using that fields. Discussed offline and we will add result/reason fields back. |
|
Looks like we should be logging to CW Logs for longer error messages, so will close this PR |
Problem
We run a
mvnandjavapshell command and want to track the error message that results when these commands fail.Solution
Add a field to the relevant metrics to keep track of the error message.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.