feat: add failure reason to events #4203
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4203 +/- ##
=======================================
Coverage 78.49% 78.50%
=======================================
Files 380 380
Lines 27143 27172 +29
=======================================
+ Hits 21306 21331 +25
- Misses 4216 4218 +2
- Partials 1621 1623 +2 ☔ View full report in Codecov by Sentry. |
x/events/events.go
Outdated
| func reasonForError(err error) string { | ||
| if ve := new(schema.ValidationError); errors.As(err, &ve) { | ||
| return ve.Message | ||
| } | ||
| if r := *new(herodot.ReasonCarrier); errors.As(err, &r) { | ||
| return r.Reason() | ||
| } | ||
| return fmt.Sprintf("reason could not be determined for error: %T", err) | ||
| } |
There was a problem hiding this comment.
People will ask about "reason IDs" to find e.g. all failed logins for reason X over time. Don't our validation errors have some type of ID or error code that we could emit?
There was a problem hiding this comment.
I want to avoid people relying on these errors and then we change the casing or error message and we get complaints about backwards incompatible changes.
There was a problem hiding this comment.
I agree here. We might be able to reuse the text package message IDs, although we might need to add a few more there as not all errors have a message associated.
Should we then split it into two attributes, ReasonID and Reason?
There was a problem hiding this comment.
We can also set some default like "contact_support" or something for the reason id to make it clear that you should request a reason id if it doesn't exist (opposed to reading the reason string field)
Not sure if this covers all the error types we can get, but it should be the majority. We can still add more once we stumble upon them.