-
Notifications
You must be signed in to change notification settings - Fork 919
GODRIVER-3690 Add ErrorCodesFrom to the mongo package #2241
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
base: master
Are you sure you want to change the base?
GODRIVER-3690 Add ErrorCodesFrom to the mongo package #2241
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.
Pull Request Overview
This PR extends the CommandFailedEvent structure to include error codes, addressing GODRIVER-3690. The change enables users to access error codes directly from command failure events without parsing the error themselves.
Key changes:
- Added a
Codesfield toCommandFailedEventto expose error codes - Implemented
errorCodes()helper function to extract codes from driver errors - Added test coverage for the new field
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| event/monitoring.go | Added Codes field to CommandFailedEvent structure |
| x/mongo/driver/operation.go | Populates the new Codes field when publishing failed events; removed blank line |
| x/mongo/driver/errors.go | Implemented errorCodes() function to extract error codes from driver errors |
| mongo/errors.go | Refactored variable declarations to use var() block (style change) |
| internal/integration/clam_prose_test.go | Added test case for CommandFailedEvent.Codes; removed blank line |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🧪 Performance ResultsCommit SHA: 7ef025aThere were no significant changes to the performance to report for version 69407bc9b48fa90007cf2de1. For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change Report./v2/mongocompatible changesErrorCodes: added |
3b7f589 to
63d4781
Compare
63d4781 to
58198cf
Compare
RafaelCenzano
left a comment
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.
LGTM
mongo/errors.go
Outdated
| var ec interface{ ErrorCodes() []int } | ||
| if errors.As(wrapErrors(err), &ec) { | ||
| return ec.ErrorCodes() |
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.
Can users use errors.As to convert the error to a mongo.ServerError and get the same information?
E.g.
var se mongo.ServerError
if errors.As(err, &se) {
se.ErrorCodes()
}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.
This would only work if mongo.ServerError is in the error tree for the arbitrary error which isn't the case for errors propagated by event monitoring. Here's an example: https://github.com/prestonvasquez/go-playground/blob/01e0e77437bdd9ba0a23d912121e152880dc19e9/mgd_errors_test.go#L17
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.
Ah, I didn't notice that the func calls wrapErrors. I think we can actually get the same behavior by adding wrapErrors to the command failed monitor like this:
if clientOpts.Monitor != nil {
orig := clientOpts.Monitor.Failed
clientOpts.Monitor.Failed = func(ctx context.Context, cfe *event.CommandFailedEvent) {
cfe.Failure = wrapErrors(cfe.Failure)
orig(ctx, cfe)
}
client.monitor = clientOpts.Monitor
}We'd add that to these lines. Would that accomplish the same thing?
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.
@matthewdale The monitoring objects that are passed to clients are mutable, so users could easily (albeit probably accidentally) undo any of our own mutation. I agree that we should improve ergonomics for event consumers to use errors.Is and errors.As, but this should be done at emission rather than mutating user-provided monitors.
mongo.ErrorCodes is clear and resolves the immediate problem at hand. If you'd like, I can a ticket to decouple wrapErrors into its own utility. If ever implemented, we can deprecate mongo.ErrorCodes in favor of the more idiomatic approach.
mongo/errors.go
Outdated
| } | ||
|
|
||
| // ErrorCodesFrom returns the list of server error codes contained in err. | ||
| func ErrorCodesFrom(err error) []int { |
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.
Optional: Consider naming this ErrorCodes, which matches the method name on the error types.
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.
Good idea 👍
| var ( | ||
| _ ServerError = CommandError{} | ||
| _ ServerError = WriteError{} | ||
| _ ServerError = WriteException{} | ||
| _ ServerError = BulkWriteException{} | ||
| ) |
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.
Do we need to define interface{ ErrorCodes() []int } here and add additional checks that these structs also satisfies the interface? It may prevent ErrorCodes() in ServerError from being modified.
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.
Nice, will update
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
qingyang-hu
left a comment
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.
LGTM!
…s-with-error-codes
matthewdale
left a comment
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.
Looks good! 👍
I'll create a separate ticket to wrap errors in the command monitor API.
| } | ||
|
|
||
| var ec errorCoder | ||
| if errors.As(wrapErrors(err), &ec) { |
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.
Do you know if wrapErrors is idempotent? I.e. if we double-wrap, does it hurt the correctness of ErrorCodes?
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.
wrapErrors is not idempotent in that calling it multiple times creates nested wrappers, but ErrorCodes is functionally idempotent because errors.As walks the entire error chain and finds the correct error code regardless of nesting depth. However, there is an inefficiency when ErrorCodes calls wrapErrors on already wrapped errors, creating unnecessary allocations. I'll add a commit to fix that 👍
…codes' of github.com:prestonvasquez/mongo-go-driver into feature/godriver-3690-extend-event-failures-with-error-codes
7ef025a
GODRIVER-3690
Summary
Add an
ErrorCodesfunction that can be used to parse server error codes from an arbitrary Go error.Usage:
Background
Would be convenient to get server status codes without caller taking a dependency on the experimental API. For example, OpenTelemetry needs to include
db.response.status.codefor otelmongo: open-telemetry/opentelemetry-go-contrib#7983 (comment)An alternative to this design would be to put
Codesonevent.CommandFailedEvent. Suggest we forgo this solution for two reasons:event.CommandFailedEvent