From ad98589061947d36c9c7bcda5c247caeae220326 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Thu, 22 Feb 2018 21:48:09 -0500 Subject: [PATCH 1/3] Use real instrumenter in spec --- spec/flipper/cloud_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/flipper/cloud_spec.rb b/spec/flipper/cloud_spec.rb index 3a7d99c89..7cf05a262 100644 --- a/spec/flipper/cloud_spec.rb +++ b/spec/flipper/cloud_spec.rb @@ -1,6 +1,7 @@ require 'helper' require 'flipper/cloud' require 'flipper/adapters/instrumented' +require 'flipper/instrumenters/memory' RSpec.describe Flipper::Cloud do context "initialize with token" do @@ -55,7 +56,7 @@ end it 'can set instrumenter' do - instrumenter = Object.new + instrumenter = Flipper::Instrumenters::Memory.new instance = described_class.new('asdf', instrumenter: instrumenter) expect(instance.instrumenter).to be(instrumenter) end From 76e08c3be07e44542721a28809ea7898ce79a6cc Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Thu, 22 Feb 2018 21:54:39 -0500 Subject: [PATCH 2/3] Allow configuring a local adapter for cloud This improves latency and resiliency. Latency is improved because cloud now uses a sync adapter with reads going to the local. Local reads mean low latency. Resiliency is improved because reads go to local which means if cloud is unavailable most reads will be fine other than a few slow ones when syncs are attempted (because they are not in background thread currently). --- lib/flipper/cloud/configuration.rb | 39 ++++++++++++++++++------ spec/flipper/cloud/configuration_spec.rb | 8 ++++- spec/flipper/cloud_spec.rb | 12 ++++++-- 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/lib/flipper/cloud/configuration.rb b/lib/flipper/cloud/configuration.rb index 4f81ed5e7..db30574e9 100644 --- a/lib/flipper/cloud/configuration.rb +++ b/lib/flipper/cloud/configuration.rb @@ -1,4 +1,6 @@ require "flipper/adapters/http" +require "flipper/adapters/memory" +require "flipper/adapters/sync" require "flipper/instrumenters/noop" module Flipper @@ -36,11 +38,21 @@ class Configuration # configuration.instrumenter = ActiveSupport::Notifications attr_accessor :instrumenter + # Public: Local adapter that all reads should go to in order to ensure + # latency is low and resiliency is high. This adapter is automatically + # kept in sync with cloud. + # + # # for example, to use active record you could do: + # configuration = Flipper::Cloud::Configuration.new + # configuration.local_adapter = Flipper::Adapters::ActiveRecord.new + attr_accessor :local_adapter + def initialize(options = {}) @token = options.fetch(:token) @instrumenter = options.fetch(:instrumenter, Instrumenters::Noop) @read_timeout = options.fetch(:read_timeout, 5) @open_timeout = options.fetch(:open_timeout, 5) + @local_adapter = options.fetch(:local_adapter) { Adapters::Memory.new } @debug_output = options[:debug_output] @adapter_block = ->(adapter) { adapter } @@ -48,7 +60,7 @@ def initialize(options = {}) end # Public: Read or customize the http adapter. Calling without a block will - # perform a read. Calling with a block yields the http_adapter + # perform a read. Calling with a block yields the cloud adapter # for customization. # # # for example, to instrument the http calls, you can wrap the http @@ -62,23 +74,30 @@ def adapter(&block) if block_given? @adapter_block = block else - @adapter_block.call(http_adapter) + @adapter_block.call sync_adapter end end - # Public: Set url and uri for the http adapter. + # Public: Set url for the http adapter. attr_writer :url private + def sync_adapter + Flipper::Adapters::Sync.new(local_adapter, http_adapter, instrumenter: instrumenter) + end + def http_adapter - Flipper::Adapters::Http.new(url: @url, - read_timeout: @read_timeout, - open_timeout: @open_timeout, - debug_output: @debug_output, - headers: { - "Feature-Flipper-Token" => @token, - }) + http_options = { + url: @url, + read_timeout: @read_timeout, + open_timeout: @open_timeout, + debug_output: @debug_output, + headers: { + "Feature-Flipper-Token" => @token, + }, + } + Flipper::Adapters::Http.new(http_options) end end end diff --git a/spec/flipper/cloud/configuration_spec.rb b/spec/flipper/cloud/configuration_spec.rb index 7805d90b0..66abd7f11 100644 --- a/spec/flipper/cloud/configuration_spec.rb +++ b/spec/flipper/cloud/configuration_spec.rb @@ -34,11 +34,17 @@ end it "defaults adapter block" do + # The initial sync of http to local invokes this web request. + stub_request(:get, /featureflipper\.com/).to_return(status: 200, body: "{}") + instance = described_class.new(required_options) - expect(instance.adapter).to be_instance_of(Flipper::Adapters::Http) + expect(instance.adapter).to be_instance_of(Flipper::Adapters::Sync) end it "can override adapter block" do + # The initial sync of http to local invokes this web request. + stub_request(:get, /featureflipper\.com/).to_return(status: 200, body: "{}") + instance = described_class.new(required_options) instance.adapter do |adapter| Flipper::Adapters::Instrumented.new(adapter) diff --git a/spec/flipper/cloud_spec.rb b/spec/flipper/cloud_spec.rb index 7cf05a262..6783bc95e 100644 --- a/spec/flipper/cloud_spec.rb +++ b/spec/flipper/cloud_spec.rb @@ -4,13 +4,18 @@ require 'flipper/instrumenters/memory' RSpec.describe Flipper::Cloud do + before do + stub_request(:get, /featureflipper\.com/).to_return(status: 200, body: "{}") + end + context "initialize with token" do let(:token) { 'asdf' } before do @instance = described_class.new(token) memoized_adapter = @instance.adapter - @http_adapter = memoized_adapter.adapter + sync_adapter = memoized_adapter.adapter + @http_adapter = sync_adapter.instance_variable_get('@remote') @http_client = @http_adapter.instance_variable_get('@client') end @@ -41,9 +46,12 @@ context 'initialize with token and options' do before do + stub_request(:get, /fakeflipper\.com/).to_return(status: 200, body: "{}") + @instance = described_class.new('asdf', url: 'https://www.fakeflipper.com/sadpanda') memoized_adapter = @instance.adapter - @http_adapter = memoized_adapter.adapter + sync_adapter = memoized_adapter.adapter + @http_adapter = sync_adapter.instance_variable_get('@remote') @http_client = @http_adapter.instance_variable_get('@client') end From 2d8cded3b4ede38c521416ee243b24481941e869 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Thu, 22 Feb 2018 22:12:28 -0500 Subject: [PATCH 3/3] Allow passing sync interval all the way down the stack --- lib/flipper/adapters/sync.rb | 3 +++ lib/flipper/adapters/sync/interval_synchronizer.rb | 4 ++++ lib/flipper/cloud/configuration.rb | 11 ++++++++++- spec/flipper/cloud/configuration_spec.rb | 13 +++++++++++++ 4 files changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/flipper/adapters/sync.rb b/lib/flipper/adapters/sync.rb index f3fb04b17..0f794690a 100644 --- a/lib/flipper/adapters/sync.rb +++ b/lib/flipper/adapters/sync.rb @@ -12,6 +12,9 @@ class Sync # Public: The name of the adapter. attr_reader :name + # Public: The synchronizer that will keep the local and remote in sync. + attr_reader :synchronizer + # Public: Build a new sync instance. # # local - The local flipper adapter that should serve reads. diff --git a/lib/flipper/adapters/sync/interval_synchronizer.rb b/lib/flipper/adapters/sync/interval_synchronizer.rb index d4c1b0ae3..f58072f4b 100644 --- a/lib/flipper/adapters/sync/interval_synchronizer.rb +++ b/lib/flipper/adapters/sync/interval_synchronizer.rb @@ -12,6 +12,10 @@ def self.now_ms Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) end + # Public: The number of milliseconds between invocations of the + # wrapped synchronizer. + attr_reader :interval + # Public: Initializes a new interval synchronizer. # # synchronizer - The Synchronizer to call when the interval has passed. diff --git a/lib/flipper/cloud/configuration.rb b/lib/flipper/cloud/configuration.rb index db30574e9..5b9d2554c 100644 --- a/lib/flipper/cloud/configuration.rb +++ b/lib/flipper/cloud/configuration.rb @@ -47,11 +47,16 @@ class Configuration # configuration.local_adapter = Flipper::Adapters::ActiveRecord.new attr_accessor :local_adapter + # Public: Number of milliseconds between attempts to bring the local in + # sync with cloud (default: 10_000 aka 10 seconds). + attr_accessor :sync_interval + def initialize(options = {}) @token = options.fetch(:token) @instrumenter = options.fetch(:instrumenter, Instrumenters::Noop) @read_timeout = options.fetch(:read_timeout, 5) @open_timeout = options.fetch(:open_timeout, 5) + @sync_interval = options.fetch(:sync_interval, 10_000) @local_adapter = options.fetch(:local_adapter) { Adapters::Memory.new } @debug_output = options[:debug_output] @adapter_block = ->(adapter) { adapter } @@ -84,7 +89,11 @@ def adapter(&block) private def sync_adapter - Flipper::Adapters::Sync.new(local_adapter, http_adapter, instrumenter: instrumenter) + sync_options = { + instrumenter: instrumenter, + interval: sync_interval, + } + Flipper::Adapters::Sync.new(local_adapter, http_adapter, sync_options) end def http_adapter diff --git a/spec/flipper/cloud/configuration_spec.rb b/spec/flipper/cloud/configuration_spec.rb index 66abd7f11..96ade11de 100644 --- a/spec/flipper/cloud/configuration_spec.rb +++ b/spec/flipper/cloud/configuration_spec.rb @@ -28,6 +28,19 @@ expect(instance.open_timeout).to eq(5) end + it "can set sync_interval" do + instance = described_class.new(required_options.merge(sync_interval: 1_000)) + expect(instance.sync_interval).to eq(1_000) + end + + it "passes sync_interval into sync adapter" do + # The initial sync of http to local invokes this web request. + stub_request(:get, /featureflipper\.com/).to_return(status: 200, body: "{}") + + instance = described_class.new(required_options.merge(sync_interval: 1_000)) + expect(instance.adapter.synchronizer.interval).to be(1_000) + end + it "can set debug_output" do instance = described_class.new(required_options.merge(debug_output: STDOUT)) expect(instance.debug_output).to eq(STDOUT)