-
Notifications
You must be signed in to change notification settings - Fork 55
feat: Add initial support for hooks #256
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
Changes from all commits
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 |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
require "ldclient-rb/impl/flag_tracker" | ||
require "ldclient-rb/impl/store_client_wrapper" | ||
require "ldclient-rb/impl/migrations/tracker" | ||
require "concurrent" | ||
require "concurrent/atomics" | ||
require "digest/sha1" | ||
require "forwardable" | ||
|
@@ -54,6 +55,7 @@ def initialize(sdk_key, config = Config.default, wait_for_sec = 5) | |
end | ||
|
||
@sdk_key = sdk_key | ||
@hooks = Concurrent::Array.new(config.hooks) | ||
|
||
@shared_executor = Concurrent::SingleThreadExecutor.new | ||
|
||
|
@@ -131,6 +133,23 @@ def initialize(sdk_key, config = Config.default, wait_for_sec = 5) | |
end | ||
end | ||
|
||
# | ||
# Add a hook to the client. In order to register a hook before the client starts, please use the `hooks` property of | ||
# {#LDConfig}. | ||
# | ||
# Hooks provide entrypoints which allow for observation of SDK functions. | ||
# | ||
# @param hook [Interfaces::Hooks::Hook] | ||
# | ||
def add_hook(hook) | ||
unless hook.is_a?(Interfaces::Hooks::Hook) | ||
@config.logger.error { "[LDClient] Attempted to add a hook that does not include the LaunchDarkly::Intefaces::Hooks:Hook mixin. Ignoring." } | ||
return | ||
end | ||
|
||
@hooks.push(hook) | ||
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. I don't remember, does ruby have threading concerns? 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. Like your node implementation, I make a copy of the hooks before I begin processing, so I don't think there is a thread issue here. 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. Ok. I wasn't sure if you actually had to lock the collection. In node there aren't threads, so it doesn't have to worry about any memory synchronization stuff. 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. Actually in thinking about it more, I do need to do some locking. This would work fine in MRI, but jRuby would probably have some sync issues since it doesn't use a GIL. Updated to use the concurrent package we have used elsewhere. |
||
end | ||
|
||
# | ||
# Tells the client that all pending analytics events should be delivered as soon as possible. | ||
# | ||
|
@@ -226,10 +245,116 @@ def variation(key, context, default) | |
# @return [EvaluationDetail] an object describing the result | ||
# | ||
def variation_detail(key, context, default) | ||
detail, _, _ = evaluate_internal(key, context, default, true) | ||
detail, _, _ = evaluate_with_hooks(key, context, default, :variation_detail) do | ||
evaluate_internal(key, context, default, true) | ||
end | ||
|
||
detail | ||
end | ||
|
||
# | ||
# evaluate_with_hook will run the provided block, wrapping it with evaluation hook support. | ||
# | ||
# Example: | ||
# | ||
# ```ruby | ||
# evaluate_with_hooks(key, context, default, method) do | ||
# puts 'This is being wrapped with evaluation hooks' | ||
# end | ||
# ``` | ||
# | ||
# @param key [String] | ||
# @param context [String] | ||
# @param default [String] | ||
# @param method [Symbol] | ||
# @param &block [#call] Implicit passed block | ||
# | ||
private def evaluate_with_hooks(key, context, default, method) | ||
return yield if @hooks.empty? | ||
|
||
hooks, evaluation_series_context = prepare_hooks(key, context, default, method) | ||
hook_data = execute_before_evaluation(hooks, evaluation_series_context) | ||
evaluation_detail, flag, error = yield | ||
execute_after_evaluation(hooks, evaluation_series_context, hook_data, evaluation_detail) | ||
|
||
[evaluation_detail, flag, error] | ||
end | ||
|
||
# | ||
# Execute the :before_evaluation stage of the evaluation series. | ||
# | ||
# This method will return the results of each hook, indexed into an array in the same order as the hooks. If a hook | ||
# raised an uncaught exception, the value will be nil. | ||
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. Would this nil propagate to the after? 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. Yes. The results of this method get returned as an array. We then use that array in the after_evaluation .zip call to pair it with the appropriate hook. |
||
# | ||
# @param hooks [Array<Interfaces::Hooks::Hook>] | ||
# @param evaluation_series_context [EvaluationSeriesContext] | ||
# | ||
# @return [Array<any>] | ||
# | ||
private def execute_before_evaluation(hooks, evaluation_series_context) | ||
hooks.map do |hook| | ||
try_execute_stage(:before_evaluation, hook.metadata.name) do | ||
hook.before_evaluation(evaluation_series_context, {}) | ||
end | ||
end | ||
end | ||
|
||
# | ||
# Execute the :after_evaluation stage of the evaluation series. | ||
# | ||
# This method will return the results of each hook, indexed into an array in the same order as the hooks. If a hook | ||
# raised an uncaught exception, the value will be nil. | ||
# | ||
# @param hooks [Array<Interfaces::Hooks::Hook>] | ||
# @param evaluation_series_context [EvaluationSeriesContext] | ||
# @param hook_data [Array<any>] | ||
# @param evaluation_detail [EvaluationDetail] | ||
# | ||
# @return [Array<any>] | ||
# | ||
private def execute_after_evaluation(hooks, evaluation_series_context, hook_data, evaluation_detail) | ||
hooks.zip(hook_data).reverse.map do |(hook, data)| | ||
try_execute_stage(:after_evaluation, hook.metadata.name) do | ||
hook.after_evaluation(evaluation_series_context, data, evaluation_detail) | ||
end | ||
end | ||
end | ||
|
||
# | ||
# Try to execute the provided block. If execution raises an exception, catch and log it, then move on with | ||
# execution. | ||
# | ||
# @return [any] | ||
# | ||
private def try_execute_stage(method, hook_name) | ||
begin | ||
yield | ||
rescue => e | ||
@config.logger.error { "[LDClient] An error occurred in #{method} of the hook #{hook_name}: #{e}" } | ||
nil | ||
end | ||
end | ||
|
||
# | ||
# Return a copy of the existing hooks and a few instance of the EvaluationSeriesContext used for the evaluation series. | ||
# | ||
# @param key [String] | ||
# @param context [LDContext] | ||
# @param default [any] | ||
# @param method [Symbol] | ||
# @return [Array[Array<Interfaces::Hooks::Hook>, Interfaces::Hooks::EvaluationSeriesContext]] | ||
# | ||
private def prepare_hooks(key, context, default, method) | ||
# Copy the hooks to use a consistent set during the evaluation series. | ||
# | ||
# Hooks can be added and we want to ensure all correct stages for a given hook execute. For example, we do not | ||
# want to trigger the after_evaluation method without also triggering the before_evaluation method. | ||
hooks = @hooks.dup | ||
evaluation_series_context = Interfaces::Hooks::EvaluationSeriesContext.new(key, context, default, method) | ||
|
||
[hooks, evaluation_series_context] | ||
end | ||
|
||
# | ||
# This method returns the migration stage of the migration feature flag for the given evaluation context. | ||
# | ||
|
@@ -508,7 +633,9 @@ def create_default_data_source(sdk_key, config, diagnostic_accumulator) | |
# @return [Array<EvaluationDetail, [LaunchDarkly::Impl::Model::FeatureFlag, nil], [String, nil]>] | ||
# | ||
def variation_with_flag(key, context, default) | ||
evaluate_internal(key, context, default, false) | ||
evaluate_with_hooks(key, context, default, :variation_detail) do | ||
evaluate_internal(key, context, default, false) | ||
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.
I was thinking if it would be possible to force implementation of metadata. But it would probably just end up like the JS implementation. Where ultimately we end up falling back anyway.
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.
I could raise an exception. That would force people to implement it. Want me to make that change?