-
Notifications
You must be signed in to change notification settings - Fork 33
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
Introduce Split Registry Endpoint Accepting a Timestamp #133
Changes from 3 commits
a83c1cd
0e45a36
1e7a869
490c4f7
65d0718
c584872
a0f2144
0977312
c3cc06c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
class Api::V2::SplitRegistrySnapshotsController < UnauthenticatedApiController | ||
include CorsSupport | ||
|
||
def show | ||
snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:timestamp]) | ||
|
||
if snapshot.valid? | ||
@split_registry_snapshot = snapshot | ||
else | ||
render_errors snapshot | ||
end | ||
end | ||
|
||
private | ||
|
||
def snapshot_params | ||
params.permit(:timestamp) | ||
end | ||
end |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
class SplitRegistrySnapshot | ||
include ActiveModel::Validations | ||
|
||
validates :timestamp, presence: true | ||
# We want to make sure the client is shipping us high-precision ISO | ||
# timestamps so we choose to allow timestamps at either millisecond or second | ||
# precision based on the W3C interpretation of iso8601 from this SO answer: | ||
# | ||
# https://stackoverflow.com/a/3143231 | ||
validates :timestamp, format: { | ||
with: /\A\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d(.\d+)?([+-][0-2]\d:[0-5]\d|Z)\z/, allow_blank: true | ||
} | ||
|
||
def initialize(timestamp:) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There still needs to be an initializer though, which probably needs this signature, right? .as_of could be added as a sugar class method tho? Or am I misinterpreting? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, my comment wasn't clear, i meant changing the arg name from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh oh, i knew I was misunderstanding. :) |
||
@timestamp = timestamp | ||
end | ||
|
||
def splits | ||
Split.active(as_of: timestamp) | ||
end | ||
|
||
def experience_sampling_weight | ||
@experience_sampling_weight ||= _experience_sampling_weight | ||
end | ||
|
||
private | ||
|
||
attr_reader :timestamp | ||
|
||
def _experience_sampling_weight | ||
Integer(ENV.fetch('EXPERIENCE_SAMPLING_WEIGHT', '1')).tap do |weight| | ||
raise <<~TEXT if weight.negative? | ||
EXPERIENCE_SAMPLING_WEIGHT, if specified, must be greater than or equal to 0. Use 0 to disable experience events. | ||
TEXT | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
json.splits do | ||
@split_registry_snapshot.splits.each do |split| | ||
json.set! split.name do | ||
json.weights split.registry | ||
json.feature_gate split.feature_gate? | ||
end | ||
end | ||
json.merge!({}) | ||
end | ||
json.(@split_registry_snapshot, :experience_sampling_weight) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
require 'rails_helper' | ||
|
||
RSpec.describe Api::V2::SplitRegistrySnapshotsController, type: :controller do | ||
let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } | ||
let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } | ||
let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } | ||
|
||
describe "#show" do | ||
before do | ||
allow(ENV).to receive(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).and_return(10) | ||
end | ||
|
||
it "includes sampling weight" do | ||
get :show, params: { timestamp: '2019-11-11T14:35:30Z' } | ||
|
||
expect(response).to have_http_status :ok | ||
expect(response_json['experience_sampling_weight']).to eq(10) | ||
end | ||
|
||
it "returns empty with no active splits on the timestamp" do | ||
expect(split_1).to be_finished | ||
|
||
get :show, params: { timestamp: '2019-11-14T14:35:30Z' } | ||
|
||
expect(response).to have_http_status :ok | ||
expect(response_json['splits']).to eq({}) | ||
end | ||
|
||
it "returns the full split registry of splits that are active during timestamp" do | ||
expect(split_1).to be_finished | ||
expect(split_2).not_to be_finished | ||
expect(split_3).not_to be_finished | ||
|
||
get :show, params: { timestamp: '2019-11-12T14:35:30Z' } | ||
|
||
expect(response).to have_http_status :ok | ||
expect(response_json['splits']).to eq( | ||
"one" => { | ||
"weights" => { "all" => 100 }, | ||
"feature_gate" => false | ||
}, | ||
"two" => { | ||
"weights" => { "on" => 50, "off" => 50 }, | ||
"feature_gate" => false | ||
}, | ||
"three_enabled" => { | ||
"weights" => { "true" => 99, "false" => 1 }, | ||
"feature_gate" => true | ||
} | ||
) | ||
end | ||
|
||
it "returns unprocessable_entity if the timestamp url param is invalid" do | ||
get :show, params: { timestamp: "2019-04-16 10:38:08 -0400" } | ||
|
||
expect(response).to have_http_status :unprocessable_entity | ||
end | ||
|
||
it "returns unprocessable_entity if the timestamp url param is missing" do | ||
get :show, params: { timestamp: "" } | ||
|
||
expect(response).to have_http_status :unprocessable_entity | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given all the other changes, snapshot no longer makes much sense, let's just change this back to
SplitRegistry
.