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.

Sign up for GitHub

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

Jump to bottom

Global instrumenter configuration #636

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Global instrumenter configuration #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0809e6
Configure instrumenter globally, add Flipper.instrument
bkeepers Jun 8, 2022
f605c86
Refactor cloud to use subscriber instead of custom instrumenter
bkeepers Jun 8, 2022
b995c12
Properly deprecate instrumenter option
bkeepers Jun 8, 2022
29452a7
Update changelog for instrumenter change
bkeepers Jun 8, 2022
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
11 changes: 11 additions & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,17 @@
### Additions/Changes

* Added failsafe adapter (https://github.com/jnunemaker/flipper/pull/626)
* Deprecate `instrumenter:` option everywhere in favor of global config. If you are manually configuring the instrumenter, you will need to update your configuration:
```diff
# config/initializers/flipper.rb
Flipper.configure do |config|
config.adapter do
- Flipper::Adapters::Instrumented.new(my_adapter, instrumenter: MyInstrumenter)
+ Flipper::Adapters::Instrumented.new(my_adapter)
end
+ config.instrumenter MyInstrumenter
end
```

## 0.24.1

Expand Down
15 changes: 7 additions & 8 deletions examples/api/custom_memoized.ru
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@
#

require 'bundler/setup'
require "active_support/notifications"
require 'active_support'
require 'active_support/notifications'
require "flipper/api"
require "flipper/adapters/pstore"

adapter = Flipper::Adapters::Instrumented.new(
Flipper::Adapters::PStore.new,
instrumenter: ActiveSupport::Notifications,
)
flipper = Flipper.new(adapter)
Flipper.configure do |config|
config.instrumenter ActiveSupport::Notifications
config.adapter { Flipper::Adapters::Instrumented.new(Flipper::Adapters::PStore.new) }
end

ActiveSupport::Notifications.subscribe(/.*/, ->(*args) {
name, start, finish, id, data = args
Expand All @@ -31,7 +31,6 @@ ActiveSupport::Notifications.subscribe(/.*/, ->(*args) {
# You can uncomment this to get some default data:
# flipper[:logging].enable_percentage_of_time 5

run Flipper::Api.app(flipper) { |builder|
builder.use Flipper::Middleware::SetupEnv, flipper
run Flipper::Api.app { |builder|
builder.use Flipper::Middleware::Memoizer, preload: true
}
7 changes: 3 additions & 4 deletions examples/api/memoized.ru
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,15 @@
#

require 'bundler/setup'
require 'active_support'
require "active_support/notifications"
require "flipper/api"
require "flipper/adapters/pstore"

Flipper.configure do |config|
config.instrumenter ActiveSupport::Notifications
config.adapter {
Flipper::Adapters::Instrumented.new(
Flipper::Adapters::PStore.new,
instrumenter: ActiveSupport::Notifications,
)
Flipper::Adapters::Instrumented.new(Flipper::Adapters::PStore.new)
}
end

Expand Down
15 changes: 6 additions & 9 deletions examples/instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'bundler/setup'
require 'securerandom'
require 'active_support'
require 'active_support/notifications'

class FlipperSubscriber
Expand All @@ -14,17 +15,13 @@ def call(*args)
require 'flipper'
require 'flipper/adapters/instrumented'

# pick an adapter
adapter = Flipper::Adapters::Memory.new

# instrument it if you want, if not you still get the feature instrumentation
instrumented = Flipper::Adapters::Instrumented.new(adapter, :instrumenter => ActiveSupport::Notifications)

# get a handy dsl instance
flipper = Flipper.new(instrumented, :instrumenter => ActiveSupport::Notifications)
Flipper.configure do |config|
config.instrumenter ActiveSupport::Notifications
config.adapter { Flipper::Adapters::Instrumented.new(Flipper::Adapters::Memory.new) }
end

# grab a feature
search = flipper[:search]
search = Flipper[:search]

perform = lambda do
# check if that feature is enabled
Expand Down
5 changes: 2 additions & 3 deletions examples/instrumentation_last_accessed_at.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Quick example of how to keep track of when a feature was last checked.
require 'bundler/setup'
require 'securerandom'
require 'active_support'
require 'active_support/notifications'
require 'flipper'

Expand All @@ -20,9 +21,7 @@ def call(name, start, finish, id, payload)
end

Flipper.configure do |config|
config.default {
Flipper.new(config.adapter, instrumenter: ActiveSupport::Notifications)
}
config.instrumenter ActiveSupport::Notifications
end

Flipper.enabled?(:search)
Expand Down
12 changes: 12 additions & 0 deletions lib/flipper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ def instance=(flipper)
:memoize=, :memoizing?,
:sync, :sync_secret # For Flipper::Cloud. Will error for OSS Flipper.

# Public: Get or set the configured instrumenter.
def_delegators :configuration, :instrumenter, :instrumenter=

# Public: Instrument a call using the configured instrumenter.
#
# Flipper.instrument(name, payload) do |payload|
# payload[:result] = something
# end
#
def_delegators :instrumenter, :instrument

# Public: Use this to register a group by name.
#
# name - The Symbol name of the group.
Expand Down Expand Up @@ -140,6 +151,7 @@ def groups_registry=(registry)
end
end

require 'flipper/deprecated_instrumenter'
require 'flipper/actor'
require 'flipper/adapter'
require 'flipper/adapters/memoizable'
Expand Down
27 changes: 11 additions & 16 deletions lib/flipper/adapters/instrumented.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,23 @@ module Adapters
# operations.
class Instrumented < SimpleDelegator
include ::Flipper::Adapter
include DeprecatedInstrumenter

# Private: The name of instrumentation events.
InstrumentationName = "adapter_operation.#{InstrumentationNamespace}".freeze

# Private: What is used to instrument all the things.
attr_reader :instrumenter

# Public: The name of the adapter.
attr_reader :name

# Internal: Initializes a new adapter instance.
#
# adapter - Vanilla adapter instance to wrap.
#
# options - The Hash of options.
# :instrumenter - What to use to instrument all the things.
#
def initialize(adapter, options = {})
super(adapter)
deprecated_instrumenter_option options
@adapter = adapter
@name = :instrumented
@instrumenter = options.fetch(:instrumenter, Instrumenters::Noop)
end

# Public
Expand All @@ -37,7 +32,7 @@ def features
adapter_name: @adapter.name,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.features
end
end
Expand All @@ -50,7 +45,7 @@ def add(feature)
feature_name: feature.name,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.add(feature)
end
end
Expand All @@ -63,7 +58,7 @@ def remove(feature)
feature_name: feature.name,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.remove(feature)
end
end
Expand All @@ -76,7 +71,7 @@ def clear(feature)
feature_name: feature.name,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.clear(feature)
end
end
Expand All @@ -89,7 +84,7 @@ def get(feature)
feature_name: feature.name,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.get(feature)
end
end
Expand All @@ -101,7 +96,7 @@ def get_multi(features)
feature_names: features.map(&:name),
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.get_multi(features)
end
end
Expand All @@ -112,7 +107,7 @@ def get_all
adapter_name: @adapter.name,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.get_all
end
end
Expand All @@ -127,7 +122,7 @@ def enable(feature, gate, thing)
thing_value: thing.value,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.enable(feature, gate, thing)
end
end
Expand All @@ -142,7 +137,7 @@ def disable(feature, gate, thing)
thing_value: thing.value,
}

@instrumenter.instrument(InstrumentationName, default_payload) do |payload|
Flipper.instrument(InstrumentationName, default_payload) do |payload|
payload[:result] = @adapter.disable(feature, gate, thing)
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/flipper/adapters/sync.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module Adapters
# rather than in the main thread only when reads happen.
class Sync
include ::Flipper::Adapter
include DeprecatedInstrumenter

# Public: The name of the adapter.
attr_reader :name
Expand All @@ -22,15 +23,14 @@ class Sync
# interval - The Float or Integer number of seconds between syncs from
# remote to local. Default value is set in IntervalSynchronizer.
def initialize(local, remote, options = {})
deprecated_instrumenter_option options
@name = :sync
@local = local
@remote = remote
@synchronizer = options.fetch(:synchronizer) do
sync_options = {
raise: false,
}
instrumenter = options[:instrumenter]
sync_options[:instrumenter] = instrumenter if instrumenter
synchronizer = Synchronizer.new(@local, @remote, sync_options)
IntervalSynchronizer.new(synchronizer, interval: options[:interval])
end
Expand Down
9 changes: 5 additions & 4 deletions lib/flipper/adapters/sync/synchronizer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,25 @@ class Sync
# Public: Given a local and remote adapter, it can update the local to
# match the remote doing only the necessary enable/disable operations.
class Synchronizer
include DeprecatedInstrumenter

# Public: Initializes a new synchronizer.
#
# local - The Flipper adapter to get in sync with the remote.
# remote - The Flipper adapter that is source of truth that the local
# adapter should be brought in line with.
# options - The Hash of options.
# :instrumenter - The instrumenter used to instrument.
# :raise - Should errors be raised (default: true).
def initialize(local, remote, options = {})
deprecated_instrumenter_option options
@local = local
@remote = remote
@instrumenter = options.fetch(:instrumenter, Instrumenters::Noop)
@raise = options.fetch(:raise, true)
end

# Public: Forces a sync.
def call
@instrumenter.instrument("synchronizer_call.flipper") { sync }
Flipper.instrument("synchronizer_call.flipper") { sync }
end

private
Expand Down Expand Up @@ -54,7 +55,7 @@ def sync

nil
rescue => exception
@instrumenter.instrument("synchronizer_exception.flipper", exception: exception)
Flipper.instrument("synchronizer_exception.flipper", exception: exception)
raise if @raise
end
end
Expand Down
27 changes: 10 additions & 17 deletions lib/flipper/cloud/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
require "flipper/adapters/memory"
require "flipper/adapters/dual_write"
require "flipper/adapters/sync"
require "flipper/cloud/instrumenter"
require "flipper/cloud/registry"
require "brow"

module Flipper
module Cloud
class Configuration
include DeprecatedInstrumenter

# The set of valid ways that syncing can happpen.
VALID_SYNC_METHODS = Set[
:poll,
Expand Down Expand Up @@ -41,14 +42,6 @@ class Configuration
# configuration.debug_output = STDOUT
attr_accessor :debug_output

# Public: Instrumenter to use for the Flipper instance returned by
# Flipper::Cloud.new (default: Flipper::Instrumenters::Noop).
#
# # for example, to use active support notifications you could do:
# configuration = Flipper::Cloud::Configuration.new
# 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.
Expand All @@ -67,6 +60,7 @@ class Configuration
attr_accessor :sync_secret

def initialize(options = {})
deprecated_instrumenter_option options
@token = options.fetch(:token) { ENV["FLIPPER_CLOUD_TOKEN"] }

if @token.nil?
Expand All @@ -88,14 +82,15 @@ def initialize(options = {})
@adapter_block = ->(adapter) { adapter }
self.url = options.fetch(:url) { ENV.fetch("FLIPPER_CLOUD_URL", DEFAULT_URL) }

instrumenter = options.fetch(:instrumenter, Instrumenters::Noop)

# This is alpha. Don't use this unless you are me. And you are not me.
cloud_instrument = options.fetch(:cloud_instrument) { ENV["FLIPPER_CLOUD_INSTRUMENT"] == "1" }
@instrumenter = if cloud_instrument
Instrumenter.new(brow: brow, instrumenter: instrumenter)
else
instrumenter
if cloud_instrument
require 'flipper/instrumentation/cloud_subscriber'

Flipper.instrumenter.subscribe(
Flipper::Feature::InstrumentationName,
Flipper::Instrumentation::CloudSubscriber.new(brow)
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@jnunemaker jnunemaker Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

end
end

Expand Down Expand Up @@ -123,7 +118,6 @@ def adapter(&block)

def sync
Flipper::Adapters::Sync::Synchronizer.new(local_adapter, http_adapter, {
instrumenter: instrumenter,
interval: sync_interval,
}).call
end
Expand Down Expand Up @@ -168,7 +162,6 @@ def dual_write_adapter

def sync_adapter
Flipper::Adapters::Sync.new(local_adapter, http_adapter, {
instrumenter: instrumenter,
interval: sync_interval,
})
end
Expand Down
Loading