Skip to content

Add method and URL to Event, add method to EventDispatcher #1

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.

Sign up for GitHub

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
delikat merged 7 commits into from
Aug 29, 2016
Merged

Add method and URL to Event, add method to EventDispatcher #1

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc9eab2
Duck type EventDispatcher
Aug 25, 2016
a58a4f2
Add method to Events, rename EventBuilder > EventBuilderV1
Aug 25, 2016
440e9e3
fix tests
Aug 25, 2016
b8a7dac
Add method to NoOpEventDispatcher
Aug 25, 2016
4b299b3
Lint
Aug 25, 2016
a93f803
Respond to CR feedback
Aug 29, 2016
f28c165
Respond to CR feedback
Aug 29, 2016
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
6 changes: 3 additions & 3 deletions lib/optimizely.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def initialize(datafile, event_dispatcher = nil, logger = nil, error_handler = n

@config = ProjectConfig.new(datafile, @logger, @error_handler)
@bucketer = Bucketer.new(@config)
@event_builder = EventBuilder.new(@config, @bucketer)
@event_builder = EventBuilderV1.new(@config, @bucketer)
end

def activate(experiment_key, user_id, attributes = nil)
Expand Down Expand Up @@ -70,7 +70,7 @@ def activate(experiment_key, user_id, attributes = nil)
@logger.log(Logger::INFO,
'Dispatching impression event to URL %s with params %s.' % [impression_event.url,
impression_event.params])
@event_dispatcher.dispatch_event(impression_event.url, impression_event.params)
@event_dispatcher.dispatch_event(impression_event.method, impression_event.url, impression_event.params)
Copy link
Contributor

@mikeproeng37 mikeproeng37 Aug 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

this is more of a design thing, but semantically speaking, when you say dispatch_event, it makes more sense to pass in the event object instead of spreading its props and passing them in individually. As we evolve the SDK I can see this method signature becoming too long/unbearable. Might as well make the refactor here.

Copy link
Contributor Author

@delikat delikat Aug 26, 2016

Choose a reason for hiding this comment

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

Agreed, was going to refactor to pass around Events in a later PR but will do so here.


@config.get_variation_key_from_id(experiment_key, variation_id)
end
Expand Down Expand Up @@ -136,7 +136,7 @@ def track(event_key, user_id, attributes = nil, event_value = nil)
@logger.log(Logger::INFO,
'Dispatching conversion event to URL %s with params %s.' % [conversion_event.url,
conversion_event.params])
@event_dispatcher.dispatch_event(conversion_event.url, conversion_event.params)
@event_dispatcher.dispatch_event(conversion_event.method, conversion_event.url, conversion_event.params)
end

private
Expand Down
35 changes: 14 additions & 21 deletions lib/optimizely/event_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,18 @@ module Optimizely
class Event
# Representation of an event which can be sent to the Optimizely logging endpoint.

# Event API format
OFFLINE_API_PATH = 'https://%{project_id}.log.optimizely.com/event'

# Gets/Sets event params.
attr_accessor :params
attr_reader :method
Copy link
Contributor

@mikeproeng37 mikeproeng37 Aug 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

these all need docs. Method is not a very descriptive name. A developer seeing this might be tempted to pass in a function. Maybe method type or http verb/action is more suitable

Copy link
Contributor Author

@delikat delikat Aug 26, 2016

Choose a reason for hiding this comment

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

Good call, I like http_verb.

attr_reader :params
attr_reader :url

def initialize(params)
def initialize(method, url, params)
@method = method
@url = url
@params = params
end

def url
# URL for sending impression/conversion event.
#
# project_id - ID for the project.
#
# Returns URL for event API.

sprintf(OFFLINE_API_PATH, project_id: @params[Params::PROJECT_ID])
end
end

class EventBuilder
class EventBuilderV1
Copy link
Contributor

@mikeproeng37 mikeproeng37 Aug 26, 2016

Choose a reason for hiding this comment

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

Instead of creating two different event builder classes, why don't we have one event builder that can build V1 events and V2 events. That way you don't have to worry about handling the instantiation of two different builder classes.

Copy link
Contributor Author

@delikat delikat Aug 26, 2016

Choose a reason for hiding this comment

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

Because when we eventually remove V1 support it will be easier to simply remove the EventBuilderV1 class than to rip out the V1 logic from a larger omnibus EventBuilder.

Copy link
Contributor

@mikeproeng37 mikeproeng37 Aug 26, 2016

Choose a reason for hiding this comment

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

I disagree because when we rip out V1 support, we would just be changing the internals of this class and the API references. If we keep two classes around there will also be some forking logic throughout the codebase that we need to kill. Providing the event builder as some sort of facade/factory will make it much easier to deal with.

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi Aug 26, 2016

Choose a reason for hiding this comment

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

I think it is way better to pick the right EventBuilder at object invocation and not have logic all over. The model here allows that.

Copy link
Contributor

@mikeproeng37 mikeproeng37 Aug 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

How about this, since the EventBuilder is already initialized with the project config (which tells us whether we are in V1 or V2), why don't we just have the event builder determine whether to return a V1 Event or V2 event when we call eventBuilder.getConversionEvent and eventBuilder.getImpressionEvent. That event can be passed into event dispatcher, which determines based on the event whether to get or post. This way, the optimizely instance calling the event builder doesn't even have to make the decision to use EventBuilder1 or EventBuilder2

# Class which encapsulates methods to build events for tracking impressions and conversions.

# Attribute mapping format
Expand All @@ -36,8 +26,11 @@ class EventBuilder
# Experiment mapping format
EXPERIMENT_PARAM_FORMAT = '%{experiment_prefix}%{experiment_id}'

attr_accessor :config
attr_accessor :bucketer
# Event endpoint path
OFFLINE_API_PATH = 'https://%{project_id}.log.optimizely.com/event'

attr_reader :config
attr_reader :bucketer
attr_accessor :params

def initialize(config, bucketer)
Expand All @@ -60,7 +53,7 @@ def create_impression_event(experiment_key, variation_id, user_id, attributes)
add_common_params(user_id, attributes)
add_impression_goal(experiment_key)
add_experiment(experiment_key, variation_id)
Event.new(@params)
Event.new(:get, sprintf(OFFLINE_API_PATH, project_id: @params[Params::PROJECT_ID]), @params)
end

def create_conversion_event(event_key, user_id, attributes, event_value, experiment_keys)
Expand All @@ -76,7 +69,7 @@ def create_conversion_event(event_key, user_id, attributes, event_value, experim
add_common_params(user_id, attributes)
add_conversion_goal(event_key, event_value)
add_experiment_variation_params(user_id, experiment_keys)
Event.new(@params)
Event.new(:get, sprintf(OFFLINE_API_PATH, project_id: @params[Params::PROJECT_ID]), @params)
end

private
Expand Down
27 changes: 12 additions & 15 deletions lib/optimizely/event_dispatcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,30 @@
require 'httparty'

module Optimizely
class BaseEventDispatcher
# Class encapsulating event dispatching functionality.
# Override with your own EventDispatcher providing dispatch_event method.

def dispatch_event(_url, _params)
end
end

class NoOpEventDispatcher < BaseEventDispatcher
class NoOpEventDispatcher
# Class providing dispatch_event method which does nothing.

def dispatch_event(_url, _params)
def dispatch_event(_method, _url, _params)
end
end

class EventDispatcher < BaseEventDispatcher
class EventDispatcher
REQUEST_TIMEOUT = 10

def dispatch_event(url, params)
def dispatch_event(method, url, params)
Copy link
Contributor

@aliabbasrizvi aliabbasrizvi Aug 26, 2016

Choose a reason for hiding this comment

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

Lets make this the third argument and optional

# Dispatch the event being represented by the Event object.
#
# method - HTTP verb with which to send the event.
# url - URL to send impression/conversion event to.
# params - Params to be sent to the impression/conversion event.

HTTParty.get(url, query: params, timeout: REQUEST_TIMEOUT)
rescue Timeout::Error => e
return e
if method == :get
begin
HTTParty.get(url, query: params, timeout: REQUEST_TIMEOUT)
rescue Timeout::Error => e
return e
end
end
end
end
end
18 changes: 7 additions & 11 deletions spec/event_builder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,9 @@
}
@event = Optimizely::Event.new(@params)
end

it 'should return URL when url is called' do
expect(@event.url).to eq('https://111001.log.optimizely.com/event')
end
end

describe Optimizely::EventBuilder do
describe Optimizely::EventBuilderV1 do
before(:context) do
@version = Optimizely::VERSION
@config_body = OptimizelySpec::CONFIG_BODY
Expand All @@ -32,7 +28,7 @@
before(:example) do
config = Optimizely::ProjectConfig.new(@config_body_JSON, @logger, @error_handler)
bucketer = Optimizely::Bucketer.new(config)
@event_builder = Optimizely::EventBuilder.new(config, bucketer)
@event_builder = Optimizely::EventBuilderV1.new(config, bucketer)
end

it 'should create Event object with right params when create_impression_event is called' do
Expand All @@ -52,7 +48,7 @@

expect(@event_builder).to receive(:create_impression_event)
.with('test_experiment', 'test_user')
.and_return(Optimizely::Event.new(expected_params))
.and_return(Optimizely::Event.new(:get, '', expected_params))
Copy link
Contributor

@mikeproeng37 mikeproeng37 Aug 26, 2016

Choose a reason for hiding this comment

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

for completeness, we should also have a test case to check that the event is built with the correct method and url

impression_event = @event_builder.create_impression_event('test_experiment', 'test_user')
expect(impression_event.params).to eq(expected_params)
end
Expand All @@ -75,7 +71,7 @@

expect(@event_builder).to receive(:create_impression_event)
.with('test_experiment', 'test_user', {'browser_type' => 'firefox'})
.and_return(Optimizely::Event.new(expected_params))
.and_return(Optimizely::Event.new(:get, '', expected_params))
impression_event = @event_builder.create_impression_event('test_experiment',
'test_user',
{'browser_type' => 'firefox'})
Expand All @@ -99,7 +95,7 @@

expect(@event_builder).to receive(:create_conversion_event)
.with('test_event', 'test_user')
.and_return(Optimizely::Event.new(expected_params))
.and_return(Optimizely::Event.new(:get, '', expected_params))
conversion_event = @event_builder.create_conversion_event('test_event', 'test_user')
expect(conversion_event.params).to eq(expected_params)
end
Expand All @@ -122,7 +118,7 @@

expect(@event_builder).to receive(:create_conversion_event)
.with('test_event', 'test_user', {'browser_type' => 'firefox'})
.and_return(Optimizely::Event.new(expected_params))
.and_return(Optimizely::Event.new(:get, '', expected_params))
conversion_event = @event_builder.create_conversion_event('test_event', 'test_user', {'browser_type' => 'firefox'})
expect(conversion_event.params).to eq(expected_params)
end
Expand All @@ -145,7 +141,7 @@

expect(@event_builder).to receive(:create_conversion_event)
.with('test_event', 'test_user', nil, 42)
.and_return(Optimizely::Event.new(expected_params))
.and_return(Optimizely::Event.new(:get, '', expected_params))
conversion_event = @event_builder.create_conversion_event('test_event', 'test_user', nil, 42)
expect(conversion_event.params).to eq(expected_params)
end
Expand Down
2 changes: 1 addition & 1 deletion spec/event_dispatcher_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

it 'should fire off GET request with provided URL and params' do
stub_request(:get, @url).with(:query => @params)
@event_dispatcher.dispatch_event(@url, @params)
@event_dispatcher.dispatch_event(:get, @url, @params)

expect(a_request(:get, @url).with(:query => @params)).to have_been_made.once
end
Expand Down
24 changes: 12 additions & 12 deletions spec/project_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,15 @@ class InvalidErrorHandler; end
}

allow(project_instance.bucketer).to receive(:bucket).and_return('111128')
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(:get, log_url, params)
allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
.with('test_experiment')
.and_return([])

stub_request(:get, log_url).with(:query => params)

expect(project_instance.activate('test_experiment', 'test_user')).to eq('control')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(:get, log_url, params).once
expect(project_instance.bucketer).to have_received(:bucket).once
end

Expand All @@ -121,11 +121,11 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}
allow(project_instance.bucketer).to receive(:bucket).and_return('122228')
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(:get, log_url, params)

expect(project_instance.activate('test_experiment_with_audience', 'test_user', 'browser_type' => 'firefox'))
.to eq('control_with_audience')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(:get, log_url, params).once
expect(project_instance.bucketer).to have_received(:bucket).once
end

Expand Down Expand Up @@ -160,7 +160,7 @@ class InvalidErrorHandler; end
}

allow(project_instance.bucketer).to receive(:bucket).and_return('111128')
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(:get, log_url, params)
allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
.with('test_experiment')
.and_return([])
Expand Down Expand Up @@ -192,9 +192,9 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(:get, log_url, params)
project_instance.track('test_event', 'test_user')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(:get, log_url, params).once
end

it 'should properly track an event by calling dispatch_event with right params with revenue provided' do
Expand All @@ -210,9 +210,9 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(:get, log_url, params)
project_instance.track('test_event', 'test_user', nil, 42)
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(:get, log_url, params).once
end

it 'should properly track an event by calling dispatch_event with right params with attributes provided' do
Expand All @@ -228,9 +228,9 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(:get, log_url, params)
project_instance.track('test_event_with_audience', 'test_user', 'browser_type' => 'firefox')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(:get, log_url, params).once
end

it 'should not call dispatch_event when tracking an event for which audience conditions do not match' do
Expand Down Expand Up @@ -258,7 +258,7 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(:get, log_url, params)
project_instance.track('test_event', 'test_user', nil, 42)
expect(spy_logger).to have_received(:log).once.with(Logger::INFO, include("Dispatching conversion event to" \
" URL #{log_url} with params"))
Expand Down