Skip to content

feat: Log and Handle Error for Event Dispatcher response #221

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

Merged
merged 3 commits into from
Dec 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/optimizely.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def initialize(
)
@logger = logger || NoOpLogger.new
@error_handler = error_handler || NoOpErrorHandler.new
@event_dispatcher = event_dispatcher || EventDispatcher.new
@event_dispatcher = event_dispatcher || EventDispatcher.new(logger: @logger, error_handler: @error_handler)
@user_profile_service = user_profile_service

begin
Expand Down Expand Up @@ -701,7 +701,7 @@ def validate_instantiation_options

return if Helpers::Validator.event_dispatcher_valid?(@event_dispatcher)

@event_dispatcher = EventDispatcher.new
@event_dispatcher = EventDispatcher.new(logger: @logger, error_handler: @error_handler)
raise InvalidInputError, 'event_dispatcher'
end

Expand Down
4 changes: 2 additions & 2 deletions lib/optimizely/event/batch_event_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,15 @@ class BatchEventProcessor < EventProcessor

def initialize(
event_queue: SizedQueue.new(DEFAULT_QUEUE_CAPACITY),
event_dispatcher: Optimizely::EventDispatcher.new,
event_dispatcher: nil,
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)
Copy link
Contributor

@jasonkarns jasonkarns Dec 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
)

@batch_size = if (batch_size.is_a? Integer) && positive_number?(batch_size)
batch_size
else
Expand Down
50 changes: 36 additions & 14 deletions lib/optimizely/event_dispatcher.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2016-2017, Optimizely and contributors
# Copyright 2016-2017, 2019, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand All @@ -15,6 +15,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
require_relative 'exceptions'

require 'httparty'

module Optimizely
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

# Dispatch the event being represented by the Event object.
#
# @param event - Event object
def dispatch_event(event)
if event.http_verb == :get
begin
HTTParty.get(event.url, headers: event.headers, query: event.params, timeout: REQUEST_TIMEOUT)
rescue Timeout::Error => e
return e
end
response = HTTParty.get(event.url, headers: event.headers, query: event.params, timeout: REQUEST_TIMEOUT)

elsif event.http_verb == :post
begin
HTTParty.post(event.url,
body: event.params.to_json,
headers: event.headers,
timeout: REQUEST_TIMEOUT)
rescue Timeout::Error => e
return e
end
response = HTTParty.post(event.url,
body: event.params.to_json,
headers: event.headers,
timeout: REQUEST_TIMEOUT)
end

error_msg = "Event failed to dispatch with response code: #{response.code}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be in constants file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"))
Copy link
Contributor

@jasonkarns jasonkarns Dec 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


when 500...600
@logger.log(Logger::ERROR, error_msg)
@error_handler.handle_error(HTTPCallError.new("HTTP Server Error: #{response.code}"))
end
rescue Timeout::Error => e
@logger.log(Logger::ERROR, "Request Timed out. Error: #{e}")
@error_handler.handle_error(e)

# Returning Timeout error to retain existing behavior.
e
rescue StandardError => e
@logger.log(Logger::ERROR, "Event failed to dispatch. Error: #{e}")
@error_handler.handle_error(e)
nil
end
end
end
7 changes: 7 additions & 0 deletions lib/optimizely/exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
module Optimizely
class Error < StandardError; end

class HTTPCallError < Error
# Raised when a 4xx or 5xx response code is recieved.
def initialize(msg = 'HTTP call resulted in a response with an error code.')
super
end
end

class InvalidAudienceError < Error
# Raised when an invalid audience is provided

Expand Down
95 changes: 91 additions & 4 deletions spec/event_dispatcher_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2016-2017, Optimizely and contributors
# Copyright 2016-2017, 2019, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand All @@ -16,11 +16,13 @@
# limitations under the License.
#
require 'spec_helper'
require 'webmock'
require 'optimizely/event_builder'
require 'optimizely/event_dispatcher'
require 'optimizely/exceptions'

describe Optimizely::EventDispatcher do
let(:error_handler) { spy(Optimizely::NoOpErrorHandler.new) }
let(:spy_logger) { spy('logger') }

before(:context) do
@url = 'https://www.optimizely.com'
@params = {
Expand All @@ -34,6 +36,9 @@

before(:example) do
@event_dispatcher = Optimizely::EventDispatcher.new
@customized_event_dispatcher = Optimizely::EventDispatcher.new(
logger: spy_logger, error_handler: error_handler
)
end

it 'should properly dispatch V2 (POST) events' do
Expand Down Expand Up @@ -64,7 +69,7 @@
expect(a_request(:get, get_url)).to have_been_made.once
end

it 'should properly dispatch V2 (GET) events' do
it 'should properly dispatch V2 (GET) events with timeout exception' do
get_url = @url + '?a=111001&g=111028&n=test_event&u=test_user'
stub_request(:get, get_url)
event = Optimizely::Event.new(:get, get_url, @params, @post_headers)
Expand All @@ -74,4 +79,86 @@

expect(result).to eq(timeout_error)
end

it 'should log and handle Timeout error' do
get_url = @url + '?a=111001&g=111028&n=test_event&u=test_user'
stub_request(:post, get_url)
event = Optimizely::Event.new(:post, get_url, @params, @post_headers)
timeout_error = Timeout::Error.new
allow(HTTParty).to receive(:post).with(any_args).and_raise(timeout_error)
result = @customized_event_dispatcher.dispatch_event(event)

expect(result).to eq(timeout_error)
expect(spy_logger).to have_received(:log).with(
Logger::ERROR, 'Request Timed out. Error: Timeout::Error'
).once

expect(error_handler).to have_received(:handle_error).once.with(Timeout::Error)
end

it 'should log and handle any standard error' do
get_url = @url + '?a=111001&g=111028&n=test_event&u=test_user'
stub_request(:post, get_url)
event = Optimizely::Event.new(:post, get_url, @params, @post_headers)
error = ArgumentError
allow(HTTParty).to receive(:post).with(any_args).and_raise(error)
result = @customized_event_dispatcher.dispatch_event(event)

expect(result).to eq(nil)
expect(spy_logger).to have_received(:log).with(
Logger::ERROR, 'Event failed to dispatch. Error: ArgumentError'
).once

expect(error_handler).to have_received(:handle_error).once.with(ArgumentError)
end

it 'should log and handle any response with status code 4xx' do
stub_request(:post, @url).to_return(status: 499)
event = Optimizely::Event.new(:post, @url, @params, @post_headers)

@customized_event_dispatcher.dispatch_event(event)

expect(spy_logger).to have_received(:log).with(
Logger::ERROR, 'Event failed to dispatch with response code: 499'
).once

error = Optimizely::HTTPCallError.new('HTTP Client Error: 499')
expect(error_handler).to have_received(:handle_error).once.with(error)
end

it 'should log and handle any response with status code 5xx' do
stub_request(:post, @url).to_return(status: 500)
event = Optimizely::Event.new(:post, @url, @params, @post_headers)

@customized_event_dispatcher.dispatch_event(event)

expect(spy_logger).to have_received(:log).with(
Logger::ERROR, 'Event failed to dispatch with response code: 500'
).once

error = Optimizely::HTTPCallError.new('HTTP Server Error: 500')
expect(error_handler).to have_received(:handle_error).once.with(error)
end

it 'should do nothing on response with status code 3xx' do
stub_request(:post, @url).to_return(status: 399)
event = Optimizely::Event.new(:post, @url, @params, @post_headers)

response = @customized_event_dispatcher.dispatch_event(event)

expect(response).to be_nil
expect(spy_logger).not_to have_received(:log)
expect(error_handler).not_to have_received(:handle_error)
end

it 'should do nothing on response with status code 600' do
stub_request(:post, @url).to_return(status: 600)
event = Optimizely::Event.new(:post, @url, @params, @post_headers)

response = @customized_event_dispatcher.dispatch_event(event)

expect(response).to be_nil
expect(spy_logger).not_to have_received(:log)
expect(error_handler).not_to have_received(:handle_error)
end
end