8000 Add method and URL to Event, add method to EventDispatcher by delikat · Pull Request #1 · optimizely/ruby-sdk · GitHub
[go: up one dir, main page]
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
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 1 commit
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
Prev Previous commit
Next Next commit
Add method to Events, rename EventBuilder > EventBuilderV1
  • Loading branch information
Andrew Delikat committed Aug 25, 2016
commit a58a4f2595ce56f307e422aed9b91870d2ae0a81
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

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
34 changes: 14 additions & 20 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,19 @@ 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

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

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

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

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

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 +27,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 +54,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 +70,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
13 changes: 9 additions & 4 deletions lib/optimizely/event_dispatcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,20 @@ def dispatch_event(_url, _params)
class EventDispatcher
REQUEST_TIMEOUT = 10

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

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 by 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
0