Skip to content
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

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
2 changes: 1 addition & 1 deletion app/controllers/api/v2/split_registries_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ class Api::V2::SplitRegistriesController < UnauthenticatedApiController
include CorsSupport

def show
@split_registry = SplitRegistry.instance
@split_registry = SplitRegistry.new(as_of: Time.zone.now)
Copy link
Member

Choose a reason for hiding this comment

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

Hey so given the fact that if I merge and deploy this, the behavior will be on my head I looked a little deeper at the current behavior here. It's my understanding that the closest to zero behavior change for this call would be to pass as_of: nil, because instance used to mean Split.for_presentation(), and Split.for_presentation() would call Split.active() which would apply the condition where(finished_at: nil). As it is this would end up applying the condition where('splits.finished_at is null or splits.finished_at > ?', Time.zone.now), which is sort of spurious since we'd expect there to be no splits finished in the future, but clock error and all that jazz could make that a thing under some circumstances.

Granted this would fail validation for SplitRegistry, if we actually valid?, which we're not doing here. Do we care? I guess not. Ok I'm on board with this. Basically this is behaviorally correct under all normal circumstances and hard to behaviorally regress, and the whole v2 api is soft deprecated at this point anyway so the blast radius of a regression here is low. Ok. I'mma merge this.

end
end
19 changes: 19 additions & 0 deletions app/controllers/api/v3/split_registries_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class Api::V3::SplitRegistriesController < UnauthenticatedApiController
include CorsSupport

def show
split_registry = SplitRegistry.new(as_of: split_registry_params[:build_timestamp])

if split_registry.valid?
@split_registry = split_registry
else
render_errors split_registry
end
end

private

def split_registry_params
params.permit(:build_timestamp)
end
end
20 changes: 18 additions & 2 deletions app/models/split_registry.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
class SplitRegistry
include Singleton
include ActiveModel::Validations

validates :as_of, 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 :as_of, 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(as_of:)
@as_of = as_of
end

def splits
Split.for_presentation
Split.active(as_of: as_of)
end

def experience_sampling_weight
Expand All @@ -11,6 +25,8 @@ def experience_sampling_weight

private

attr_reader :as_of

def _experience_sampling_weight
Integer(ENV.fetch('EXPERIENCE_SAMPLING_WEIGHT', '1')).tap do |weight|
raise <<~TEXT if weight.negative?
Expand Down
10 changes: 10 additions & 0 deletions app/views/api/v3/split_registries/show.json.jbuilder
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
json.splits do
@split_registry.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, :experience_sampling_weight)
6 changes: 6 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@
end
end
end

namespace :v3 do
resources :builds, only: [], param: :timestamp do
resource :split_registry, only: :show
end
end
end

if ENV['SAML_ISSUER'].present?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

describe "#show" do
before do
allow(SplitRegistry.instance).to receive(:experience_sampling_weight).and_return(10)
allow(ENV).to receive(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).and_return(10)
end

it "includes sampling weight" do
Expand Down
71 changes: 71 additions & 0 deletions spec/controllers/api/v3/split_registries_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
require 'rails_helper'

RSpec.describe Api::V3::SplitRegistriesController, type: :controller do
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: { build_timestamp: '2019-11-11T14:35:30Z' }

expect(response).to have_http_status :ok
expect(response_json['experience_sampling_weight']).to eq(10)
end

context "without active split on given timestamp" do
let!(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } }

it "returns empty with no active splits on the timestamp" do
expect(split_1).to be_finished

get :show, params: { build_timestamp: '2019-11-14T14:35:30Z' }

expect(response).to have_http_status :ok
expect(response_json['splits']).to eq({})
end
end

context "with splits active on given during timestamp" 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 }

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: { build_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
end

it "returns unprocessable_entity if the timestamp url param is invalid" do
get :show, params: { build_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: { build_timestamp: "" }

expect(response).to have_http_status :unprocessable_entity
end
end
end
82 changes: 53 additions & 29 deletions spec/models/split_registry_spec.rb
Original file line number Diff line number Diff line change
@@ -1,62 +1,86 @@
require 'rails_helper'

RSpec.describe SplitRegistry do
subject { described_class.instance }
subject { described_class.new(as_of: Time.zone.now) }

describe 'validations' do
it "is valid with valid args" do
expect(described_class.new(as_of: "2019-04-16T14:35:30Z")).to be_valid
end

it "is invalid with no timestamp" do
expect(described_class.new(as_of: "")).to be_invalid
end

it "is invalid with a non-ISO date" do
expect(described_class.new(as_of: "2019-04-16 10:38:08 -0400")).to be_invalid
end

it "is valid with an ISO date with millis" do
expect(described_class.new(as_of: "2019-04-16T14:35:30.123Z")).to be_valid
end

it "is invalid with an ISO date without seconds" do
expect(described_class.new(as_of: "2019-04-16T14:35Z")).to be_invalid
end
end

describe "#splits" do
it "doesn't cache the instance" do
expect(subject.splits).to eq(subject.splits)
expect(subject.splits).not_to eql(subject.splits)
end

it "returns active splits" do
it "returns active splits as of provided timestamp" do
split = FactoryBot.create(:split)

expect(subject.splits.all).to include(split)
end

it "doesn't return inactive splits" do
split = FactoryBot.create(:split, finished_at: Time.zone.now)
it "doesn't return inactive splits as of given timestamp" do
split = FactoryBot.create(:split, finished_at: 1.day.ago)

expect(subject.splits.all).not_to include(split)
end

it "returns splits that were retired after the given timestamp" do
split = FactoryBot.create(:split, finished_at: Time.zone.now)

expect(described_class.new(as_of: 1.day.ago).splits.all).to include(split)
end
end

describe "#experience_sampling_weight" do
context "bypassing singleton memoization" do
subject { described_class.send(:new) }

it "memoizes the env var fetch" do
allow(ENV).to receive(:fetch).and_call_original
it "memoizes the env var fetch" do
allow(ENV).to receive(:fetch).and_call_original

subject.experience_sampling_weight
subject.experience_sampling_weight
subject.experience_sampling_weight
subject.experience_sampling_weight

expect(ENV).to have_received(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).exactly(:once)
end
expect(ENV).to have_received(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).exactly(:once)
end

it "returns 1 with no env var" do
with_env EXPERIENCE_SAMPLING_WEIGHT: nil do
expect(subject.experience_sampling_weight).to eq 1
end
it "returns 1 with no env var" do
with_env EXPERIENCE_SAMPLING_WEIGHT: nil do
expect(subject.experience_sampling_weight).to eq 1
end
end

it "returns an positive value specified" do
with_env EXPERIENCE_SAMPLING_WEIGHT: '10' do
expect(subject.experience_sampling_weight).to eq 10
end
it "returns an positive value specified" do
with_env EXPERIENCE_SAMPLING_WEIGHT: '10' do
expect(subject.experience_sampling_weight).to eq 10
end
end

it "returns zero when specified" do
with_env EXPERIENCE_SAMPLING_WEIGHT: '0' do
expect(subject.experience_sampling_weight).to eq 0
end
it "returns zero when specified" do
with_env EXPERIENCE_SAMPLING_WEIGHT: '0' do
expect(subject.experience_sampling_weight).to eq 0
end
end

it "blows up on negative" do
with_env EXPERIENCE_SAMPLING_WEIGHT: '-1' do
expect { subject.experience_sampling_weight }.to raise_error(/greater than or equal/)
end
it "blows up on negative" do
with_env EXPERIENCE_SAMPLING_WEIGHT: '-1' do
expect { subject.experience_sampling_weight }.to raise_error(/greater than or equal/)
end
end
end
Expand Down