Fix scenarios where error messages aren't propagated correctly #424
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hspedro
pushed a commit
that referenced
this pull request
Dec 16, 2024
* Improve tests for agent and service/remote to expose bad behaviors * Breakdown test into multiple ones to make them more clear * Fix scenarios where error messages aren't propagated correctly --------- Co-authored-by: Felippe Durán <felippe.duran@wildlifestudios.com>
hspedro
added a commit
that referenced
this pull request
Dec 18, 2024
…#425) * Fix scenarios where error messages aren't propagated correctly (#424) * Improve tests for agent and service/remote to expose bad behaviors * Breakdown test into multiple ones to make them more clear * Fix scenarios where error messages aren't propagated correctly --------- Co-authored-by: Felippe Durán <felippe.duran@wildlifestudios.com> * fix(groups): add cancelFunc and Close callback When running tests we need to ensure a graceful cleanup by terminating/closing the context by calling the Close() method, which should call the cancelFunc of the group service context. This should graceful handle goroutines like groupTTLCleanup with a sane context cancellation --------- Co-authored-by: Felippe Durán <felippe.duran@gmail.com> Co-authored-by: Felippe Durán <felippe.duran@wildlifestudios.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, there are three scenarios where pitaya errors aren't propagated correctly:
errors.NewErrordoesn't make proper use oferrors.As, triggering the issue.utils.GetErrorFromPayloadlooks for explicit implementations, breaking polymorphism and inducing the bug.utils.GetErrorPayload, that would convert any error to the protobuf version of a pitaya error, which would then fail to get parsed byutils.GetErrorFromPayloadFor my specific case, I encountered these issues when trying to return pitaya errors in the routing function. The first one would convert it to a generic pitaya error
Internalbefore returning to clients, and the other two would make error messages not propagate correctly to metrics, having errors asUnknownin Prometheus.This MR fixes all three scenarios, with the following changes:
utils.GetErrorFromPayloadnow always assumes error payloads are in protobuf schema. This is aligned with theutils.GetErrorPayload, which always serializes with the same schema.errors.NewErrornow useserrors.Aswhen trying to merge pitaya errors.I also took the time to improve some test cases, implement a few more and test things against pitaya-cli.
Notes:
An alternative would be to make the serialization methods use a non-protobuf schema for JSON. However, the C#
PitayaClientalways assumes errors to be in the protobuf schema, withcodeandmsg(see here). I don't know if that was intentional, or an accidental behavior due to the previously asymmetricutils.GetErrorFromPayloadandutils.GetErrorPayload.