Gracefully degrade application failure error when deserializing #103
Conversation
drewhoskins-stripe
force-pushed
the
drewhoskins_serialization_fallback
branch
from
3dbd0a0 to
aa531aa
Compare
drewhoskins-stripe
force-pushed
the
drewhoskins_serialization_fallback
branch
from
aa531aa to
56558ca
Compare
| @@ -0,0 +1,12 @@ | |||
| # Simulates Temporal::Connection::Serializer::Failure | |||
| Fabricator(:application_failure, from: Temporal::Api::Failure::V1::Failure) do | |||
There was a problem hiding this comment.
nit: Can you please move this one over to fabricators/grpc and prefix with api_? Helps with distinguishing between domain and edge entities
| application_failure_info do |attrs| | ||
| Temporal::Api::Failure::V1::ApplicationFailureInfo.new( | ||
| type: attrs[:error_class], | ||
| details: TestDeserializer.new.to_details_payloads(attrs[:message]), |
There was a problem hiding this comment.
This will fail when used in any other spec since TestDeserializer is only defined in a your new specs
There was a problem hiding this comment.
Also since the fabricator is for a proto message it probably should not be concerned with error serialisation, but passed details for the error directly
There was a problem hiding this comment.
Good flag. As for the second suggestion, feels like reusing production methods rather than reimplementing them trumps strict dependencies here, to make tests more understandable and maintainable.
| 'Could not instantiate original error. Defaulting to StandardError.', | ||
| { | ||
| :original_error=>"ErrorWithTwoArgs", | ||
| :instantiation_error_message=> "wrong number of arguments (given 1, expected 2)", |
There was a problem hiding this comment.
Any specific reason for not using the new Hash syntax here (key: 'value')?
There was a problem hiding this comment.
Looks good, thank you for the PR, Drew. I've noticed that it's missing a test case that uses an explicitly provided default error class.
It might be really useful to provide one of our error classes based on the context instead of the generic StandardError (which might be difficult to distinguish from other failures).
Let's add this in a follow up PR, but we should do this before bumping the version
…oner/idle-metrics Thread pool and poller metrics
Description
As discussed in slack, our error handler raises when confronted with a user error that takes multiple arguments to instantiate.
This improves it by at least raising an error that links back to the original error (and still keeping the original backtrace)
Also log an error when this happens.
Test Plan
Unit:
rspec ./spec/unit/lib/temporal/workflow/errors_spec.rbIntegration, to check for regressions: