-
Notifications
You must be signed in to change notification settings - Fork 127
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
Record new error type "invalid state" to accurately describe the… #230
Conversation
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.
I, in general, like this idea, but share the same concerns.
Also, we would have diverging behavior between glean-ac and glean-core, which we also may not want...
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.
I like the idea and the implementation. As @badboy said, we should merge this after we drop glean-ac. It's fine to change things for existing metrics, I think, in this case: we're specifying a new error class to better catch problems.
I'm holding back r+ on this to make sure this lands after we release glean-core.
CAUTION: This might change error reporting for existing metrics.
CAUTION: This might change error reporting for existing metrics.
We should decide if we want that.
That will only apply once we drop Glean AC (or we would need to also backport this)