feat: get_optimizely_config API to get static experiments and features data #220
Conversation
1 similar comment
lib/optimizely.rb
Outdated
| # OptimizelyConfig Object Schema | ||
| # { | ||
| # 'experimentsMap' => { | ||
| # '111111' => { |
There was a problem hiding this comment.
The examples are misleading as "id"-based map. Change them all to "key"-based map.
There was a problem hiding this comment.
Great catch. Fixed!. I copied them over from javascript-sdk. will fix it there too.
| # 'revision' => '13', | ||
| # } | ||
| # | ||
| optimizely_config = OptimizelyConfig.new(project_config) |
There was a problem hiding this comment.
Add "unless is_valid" guard like other API calls, returning null when not ready.
| feature_variables_map = @project_config.feature_flags.reduce({}) do |result_map, feature| | ||
| result_map.update(feature['id'] => feature['variables']) | ||
| end | ||
| @project_config.experiments.reduce({}) do |experiments_map, experiment| |
There was a problem hiding this comment.
experiments should include experiments dedicated to "groups" as well. It looks like this list does not include them. Check it out.
There was a problem hiding this comment.
It does. Project Config already merged grouped experiments in @experments array. It contains all experiments except rollouts
lib/optimizely.rb
Outdated
| @@ -523,6 +524,52 @@ def close | |||
| @event_processor.stop! if @event_processor.respond_to?(:stop!) | |||
| end | |||
|
|
|||
| def get_optimizely_config # rubocop:disable Naming/AccessorMethodName | |||
There was a problem hiding this comment.
Instead of disabling it inline here. Please put in rubocop_todo.yml. Makes it easier to see all rule exceptions at one place.
You can even use this utility to auto generate todo file for you.
rubocop --auto-gen-config
There was a problem hiding this comment.
this particular rule can only be disabled for whole file using rubocop_todo which i think is not a good idea. What do you suggest ?
There was a problem hiding this comment.
I see. I would choose to keep in the todo file. But you may keep it this way.
There was a problem hiding this comment.
We shouldn't depend our code files on rubocop. Please add the rule in rubocop_todo.yml.
| # | ||
|
|
||
| module Optimizely | ||
| class OptimizelyConfig |
There was a problem hiding this comment.
This is not a blocker as discussed offline, I would suggest that we should persist OptimizelyConfig response in this class. And each time config is called, it should expect a project config and we only generate new maps if the project ID or revision has changed, otherwise we can return the same response.
Datafile is unlikely to change so often. And we can save time from unnecessarily creating and merging so many maps to generate the same response again and again. Especially when the datafile is huge with many features/ experiments/ variables.
There was a problem hiding this comment.
Its a great suggestion and i remember we discussed it offline as well. I did it this way because its explicitly mentioned in the documentation that a new object will be generated every time. But i agree, this makes more sense to me as well.
@jaeopt @mjc1283 .. What do you guys think about storing optimizelyconfig and creating a new one only if project config has a newer revision?
There was a problem hiding this comment.
@zashraf1985 @oakbani both Jae and Matt agree that we should add caching fo the OptimizelyConfig. Can you update the PR?
There was a problem hiding this comment.
@thomaszurkan-optimizely You are abolutely right. But its also decided to do it in two separate PRs and merge them one by one in to master. So this should be considered as the first PR without caching. Once this is merged, we will create another one for caching.
Summary
Added a
get_optimizely_configAPI which returns static experiments and features data.Test plan