-
Notifications
You must be signed in to change notification settings - Fork 213
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
Add Nexus failure_reason
metric tag
#1671
Add Nexus failure_reason
metric tag
#1671
Conversation
// NexusTaskFailureTags returns a set of tags for Nexus Operation failures. | ||
func NexusTaskFailureTags(reason string) map[string]string { | ||
return map[string]string{ | ||
FailureReasonTagName: reason, |
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.
Is it possible to, somewhere, clarify all possible enumerate values here? Some spec or something for SDKs to follow. I want to make sure it's a fixed enumerate, that all SDKs share them, and we are ok with the casing of the values.
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.
My plan was to put this in our docs in /sdk-metrics
. Does that work for you?
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.
Sure! I would like to confirm the values before merging this (can add them in here if easiest).
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.
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.
So to confirm, it's handler_error_NOT_IMPLEMENTED
not all consistent cased, correct? I don't mind, just checking.
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, that's correct. Do you think it's worth doing a lower case conversion here?
I updated the docs PR to use upper case letters.
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.
For gRPC errors the status_code
tag is all caps
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.
Let's move on. I think we agree it's fine as long as it's consistent and documented.
On the test failure |
A follow up for #1664 where we decided to add more information on Nexus task failures.