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.

Sign up for GitHub

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(gumby): modify metric to track errors #677

Closed
wants to merge 2 commits into from
Closed

fix(gumby): modify metric to track errors #677

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4dd9958
fix(gumby): modify metric to track errors
Jan 23, 2024
081d870
fix: add back result/reason
Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions telemetry/definitions/commonDefinitions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3538,8 +3538,18 @@
"type": "codeTransformSessionId",
"required": true
},
{ "type": "result", "required": true },
{ "type": "reason", "required": true }
Copy link
Contributor Author

@dhasani23 dhasani23 Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EDIT: Added them back.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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": "codeTransformRuntimeError",
"required": false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dhasani23 dhasani23 Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note that codeTransformRuntimeError is an existing field we have (already defined as a string)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

},
{
"type": "result",
"required": true
},
{
"type": "reason",
"required": true
}
]
},
{
Expand Down Expand Up @@ -3890,8 +3900,16 @@
"required": true
},
{
"type": "reason",
"required": false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See first comment at very top.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are trying to keep result and reason as part of our new standard. What is the goal here from removing 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.

Added them back.

"type": "codeTransformRuntimeError",
"required": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required here because when mvn command fails we should always have an error message to emit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my other comment above

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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": "result",
"required": true
},
{
"type": "reason",
"required": true
}
]
}
Expand Down