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
qe: Identify which request in a batch caused error #3384
Conversation
245caac
to
689eb59
Compare
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Adds `batch_request_idx` property to user facing errors. On the client, that would allow us to build correct error message for `$transaction` errors. Ref: prisma/prisma#15433, prisma/prisma#14373
689eb59
to
26e00ab
Compare
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
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.
Code is clear, well done! Would it make sense to write a test exercising that the new property is being populated correctly? Other than that changes look good!
.await; | ||
|
||
histogram!(PRISMA_CLIENT_QUERIES_HISTOGRAM_MS, operation_timer.elapsed()); |
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 might be an unintentional change, but previously this metric wouldn't have been logged if there was an error, right? The new behaviour sounds more correct to me, but we might want to check what happens in other places and if it's consistent.
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.
Yep, in execute_single_query
we write this histogram regardless of whether the result is Ok
or Err
. Looks like another unrelated bug fixed here at the same time 😄
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 catch.
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.
Even amount of errors = correct result 😆
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 is nice and clean. I agree with @miguelff could you add a test cause for this so we know it works as expected.
.await; | ||
|
||
histogram!(PRISMA_CLIENT_QUERIES_HISTOGRAM_MS, operation_timer.elapsed()); |
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 catch.
@miguelff @garrensmith now second attempt: this time a proper test, not the snapshot one |
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
…6240) Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
…6240) Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
…6240) Engine PR: prisma/prisma-engines#3384 Uses newly added `batch_request_idx` property of an errors to identify and correctly report error location within a batch. Fix #15433 Fix #14373
Adds
batch_request_idx
property to user facing errors. On the client,that would allow us to build correct error message for
$transaction
errors.
Ref: prisma/prisma#15433, prisma/prisma#14373