Global instrumenter configuration #636
Conversation
| Flipper.instrumenter.subscribe( | ||
| Flipper::Feature::InstrumentationName, | ||
| Flipper::Instrumentation::CloudSubscriber.new(brow) | ||
| ) |
There was a problem hiding this comment.
Just a heads up that I rewrote this to use a subscriber instead of wrapping the instrumenter. I think it's fair to say that anyone that wants to use this should be using AS::Notifications (default in Rails) or some other instrumentation framework.
Doesn't seem like there were any tests for this, and I have not tested it manually yet.
There was a problem hiding this comment.
The new telemetry stuff needs to work as a wrapper so that and this will conflict.
The main reason is that AS notifications are super slow. I don't want people (including me) to have to use those in order to get analytics.
There was a problem hiding this comment.
The main reason is that AS notifications are super slow. I don't want people (including me) to have to use those in order to get analytics.
FWIW, the engine configures AS Notifications by default in all Rails apps, so every app you manage right now is probably already using it.
There was a problem hiding this comment.
K. I thought we turned that off for some reason but I can see it is not. I'm sure my apps aren't sensitive to it but I've helped a handful of people debug perf issues and AS::Notification was the easiest win each time.
| @@ -1,8 +1,9 @@ | |||
| module Flipper | |||
| class Configuration | |||
| def initialize(options = {}) | |||
| @default = -> { Flipper.new(adapter) } | |||
| @adapter = -> { Flipper::Adapters::Memory.new } | |||
| adapter { Flipper::Adapters::Memory.new } | |||
There was a problem hiding this comment.
Wow! That looks nice. I know its a little thing, but sweet.
| @@ -84,21 +84,19 @@ | |||
| context 'when primary is instrumented and fails' do | |||
| before do | |||
| allow(memory_adapter).to receive(:get).and_raise(Net::ReadTimeout) | |||
| Flipper.instrumenter = instrumenter | |||
There was a problem hiding this comment.
This is ok because it's actually updating config and config is reset for every spec, right? Just did a double take because global changes without resetting always make me shiver. :)
| @@ -6,16 +6,16 @@ | |||
| let(:statsd_client) { Statsd.new } | |||
| let(:socket) { FakeUDPSocket.new } | |||
| let(:adapter) do | |||
| memory = Flipper::Adapters::Memory.new | |||
| Flipper::Adapters::Instrumented.new(memory, instrumenter: ActiveSupport::Notifications) | |||
| Flipper::Adapters::Instrumented.new(Flipper::Adapters::Memory.new) | |||
There was a problem hiding this comment.
Now stuff like this can use use right?
While working on some changes for a different PR, I was running into some friction with having to pass the instrumenter option around everywhere. This doesn't seem like something that needs configured per object. This PR adds a global
Flipper.instrumenterconfig and aFlipper.instrumentmethod.