feat: Log and Handle Error for Event Dispatcher response #221
Conversation
| end | ||
|
|
||
| error_msg = "Event failed to dispatch with response code: #{response.code}" |
There was a problem hiding this comment.
Shouldn't it be in constants file?
There was a problem hiding this comment.
We don't follow this as a practice in the SDK. And I would prefer to keep this here as I don't expect it to be reused.
| case response.code | ||
| when 400...500 | ||
| @logger.log(Logger::ERROR, error_msg) | ||
| @error_handler.handle_error(HTTPCallError.new("HTTP Client Error: #{response.code}")) |
There was a problem hiding this comment.
As a consumer, i would be super frustrated that I have to parse the error message to determine the actual http response code... I would request that the code be added as a property of the error type.
Relatedly, I would recommend having two separate error types for server/client. With a single error type, error handlers will need to parse the message instead of using instance matching.
Indeed, these error types don't need to be created yourself. Ruby already provides them in the standard library: https://ruby-doc.org/stdlib-2.6.5/libdoc/net/http/rdoc/Net/HTTPExceptions.html
And as a stylistic point, I'll just note that instead of handling the error here, all this code could be a single rescue => e. the 400/500 responses could be raised, and the timeout/standard error rescues be combined to a single one that logs the error message and invokes the handler. I think that would simplify all this quite a bit.
| @@ -28,26 +30,46 @@ class EventDispatcher | |||
| # @api constants | |||
| REQUEST_TIMEOUT = 10 | |||
|
|
|||
| def initialize(logger: nil, error_handler: nil) | |||
| @logger = logger || NoOpLogger.new | |||
| @error_handler = error_handler || NoOpErrorHandler.new | |||
There was a problem hiding this comment.
I'm confused why the keyword params are defaulted to nil, only to have that default overridden?
I think the idiomatic way to do this would be:
def initialize(logger: NoOpLogger.new, error_handler: NoOpErrorHandler.new)
@logger = logger
@error_handler = error_handler
end
| batch_size: DEFAULT_BATCH_SIZE, | ||
| flush_interval: DEFAULT_BATCH_INTERVAL, | ||
| logger: NoOpLogger.new, | ||
| notification_center: nil | ||
| ) | ||
| @event_queue = event_queue | ||
| @logger = logger | ||
| @event_dispatcher = event_dispatcher | ||
| @event_dispatcher = event_dispatcher || EventDispatcher.new(logger: @logger) |
There was a problem hiding this comment.
keyword args can be used within the method signature directly, there's no need to do all the manual defaulting within the body.
e.g.
def initialize(
logger: NoOpLogger.new,
event_dispatcher: EventDispatcher.new(logger: logger)
)
Summary
Test plan
Issues