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.

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

Stop initialising the IsUnexpected flag to false for new errors #54

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

mattcottam-monzo
Copy link
Contributor

The IsUnexpected flag has predominately been designed to support the case that you have an "expected" error (e.g. validation errors that would lead to a 400-like error code) that should actually be seen as "unexpected" (e.g. because the params being validated come from code and not user input), and so fail more noisily.

However there is also a sort-of-opposite scenario, where an "unexpected" error wants to be perceived as "expected". An example might be a downstream error from Service B bubbling up to Service A, where Service A handling this error doesn't need to also fail noisily in certain circumstances (e.g. a different team owning each service, where the error will just be noise to the owners of Service A).

This flag has the potential to support this second scenario too, as it has 3 values: nil, true or false. However, we currently initialise all new errors with false, meaning in reality this flag will never be nil, and it's impossible to differentiate between "expected" errors and "normal" errors (ones where we've not set this flag either way, and the normal logic should apply).

This PR changes the default for new errors to not initialise this flag (i.e. it will be nil). This should be a safe change, as long as any calling logic handles the potential nil pointer correctly already - you should check this before pulling in this change.

@@ -218,7 +218,8 @@ func (p *Error) Retryable() bool {
}

// Unexpected states whether an error is not expected to occur. In many cases this will be due to a bug, e.g. due to a
// defensive check failing
// defensive check failing.
// Note that if the IsUnexpected flag has not been set at all, this will still return 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.

Felt it was worth adding a note here. Not changing this means this change should have no practical impact for someone relying on this function (we defaulted to false -> false, now we default to nil -> false).

Longer term I question if we really want this function, or perhaps we should have an equivalent Expected() function (that returns true only if the flag is set to false). But that risks making this more of a breaking change.

@tompreston tompreston merged commit f93c635 into master Sep 3, 2024
2 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