feat: Add hooks implementation #95

MaximFischuk · 2024-12-16T15:47:37Z

This PR

This pull request introduces the implementation of Hooks in the OpenFeature SDK following the OpenFeature standard. The changes include the addition of hooks to various stages in the feature flag evaluation process, such as before, after, error, and finally. The implementation provides capabilities to add hooks at the global, client, provider, and invocation levels, which allows for extensive customizability and control over the feature flag lifecycle.

Adds hooks to support the OpenFeature specification for flag evaluation.
Implements the capability to register hooks at various levels including global, client, provider, and invocation.
Introduces logging for lifecycle events using hooks.
Provides test cases for the new hooks capability to ensure compliance with the OpenFeature specification.

Related Issues

Fixes #5

Notes

Make sure logging is appropriately configured in the consuming application to take advantage of the logging hooks.
This implementation supports extensibility for additional hooks in the future by following the standardized hook interface.

Follow-up Tasks

Consider implementing example hooks that demonstrate different use cases, like metrics collection or error reporting.
Evaluate if default hooks should be provided in the SDK for common use cases.

How to test

Build and run the example included in examples/hooks.rs to see a working implementation of the hooks in action.
Review test cases provided in src/hooks/mod.rs to ensure they cover expected behaviors and edge cases.
Ensure that the hooks are called in the correct order and can handle errors gracefully.

Signed-off-by: Maxim Fischuk <[email protected]> Signed-off-by: MaximFischuk <[email protected]>

Signed-off-by: MaximFischuk <[email protected]>

beeme1mr · 2024-12-18T13:40:53Z

src/hooks/logging.rs

+    ) {
+        log::error!("Error hook for flag {}: {:?}", context.flag_key, error);
+    }
+    async fn finally<'a>(&self, context: &HookContext<'a>, _: Option<&'a HookHints>) {


Suggested change

async fn finally<'a>(&self, context: &HookContext<'a>, _: Option<&'a HookHints>) {

async fn finally<'a>(&self, context: &HookContext<'a>, _: &EvaluationDetails<Value>, _: Option<&'a HookHints>) {

Hey @MaximFischuk, the finally stage is hooks changed slightly in a recent spec update. Thanks!

Thank you for the information! Seems like I've missed it, I'll update it soon :)

…ment 4.3.8` of OpenFeature specification Signed-off-by: MaximFischuk <[email protected]>

beeme1mr · 2024-12-25T21:41:51Z

Hey @MaximFischuk, reviews may be slower than usual around the holidays. Hopefully, someone can take a look soon.

I'll do my best to review, but my Rust skills are basically non-existent 😄

gruebel

overall great job 🍻

gruebel · 2024-12-26T20:31:36Z

Cargo.toml

@@ -2,7 +2,7 @@
 name = "open-feature"
 version = "0.2.4"
 edition = "2021"
-rust-version = "1.71.1" # MSRV
+rust-version = "1.71.1"                                 # MSRV


I think something went wrong here 😅

Yep, TOML formatter did weird thing :)

gruebel · 2024-12-26T20:37:36Z

src/api/api.rs

@@ -54,6 +56,12 @@ impl OpenFeature {
        self.provider_registry.set_named(name, provider).await;
    }

+    /// Add a new hook to the global list of hooks.
+    pub async fn add_hook<T: Hook>(&mut self, hook: T) {
+        let mut lock = self.hooks.get_mut().await; // TODO: Remove async context to avoid deadlock and enforce initialization before usage.


not sure about the TODO here. didn't you RwLock guard hooks anyway?

Some of my notes were accidentally published to PR, sorry, I'll remove them

gruebel · 2024-12-26T20:42:32Z

src/evaluation/context_field_value.rs

@@ -11,7 +11,7 @@ pub enum EvaluationContextFieldValue {
    Float(f64),
    String(String),
    DateTime(OffsetDateTime),
-    Struct(Arc<dyn Any + Send + Sync>),
+    Struct(Arc<dyn Any + Send + Sync>), // TODO: better to make structure in similar way as serde_json::Value


you mean to create a nested HashMap struct?

Exactly! Type information is completely cleared here, so the client cannot restore the initial type without workarounds. My suggestion is to add a recursion Map like it was implemented in serde_json.

gruebel · 2024-12-26T20:52:45Z

src/api/client.rs

@@ -23,20 +25,26 @@ pub struct Client {
    provider_registry: ProviderRegistry,
    evaluation_context: EvaluationContext,
    global_evaluation_context: GlobalEvaluationContext,
+    global_hooks: GlobalHooks,
+
+    hooks: Vec<HookWrapper>,


could you rename it to client_hooks?

gruebel · 2024-12-26T20:53:26Z

src/api/client.rs

+            .await;
+        hook_context.evaluation_context = &context;
+
+        // INFO: Result of the resolution or error reson with default value


Suggested change

// INFO: Result of the resolution or error reson with default value

// INFO: Result of the resolution or error reason with default value

gruebel · 2024-12-26T21:05:56Z

src/hooks/logging.rs

+        context: &HookContext<'a>,
+        _: Option<&'a HookHints>,
+    ) -> Result<Option<EvaluationContext>, EvaluationError> {
+        log::debug!("Before hook for flag {}", context.flag_key);


just thinking, wouldn't it be better to raise the log levels, debug -> info and the trace -> debug?

It's debatable, enterprise clients usually hide debug level of logs in production. Flag evaluating could generate extremely many logs in large applications so it may force a client to disable logs specifically for the OpenFeature library because just setting log level info is not enough.
But, I don't insist :)

gruebel · 2024-12-26T21:07:26Z

src/serde_json.rs

@@ -20,7 +20,7 @@ fn json_value_to_value(value: &serde_json::Value) -> EvaluationResult<Value> {
    match value {
        serde_json::Value::Bool(value) => Ok(Value::Bool(*value)),
        serde_json::Value::Number(value) if value.is_i64() => {
-            Ok(Value::Int(value.as_i64().unwrap()))
+            Ok(Value::Int(value.as_i64().unwrap())) //TODO: remove unwrap or use expect


can you explain the TODO? how do you wan to remove the unwrap()?

In general, it is better to avoid any potential panic in libraries, so in my opinion, the error could be returned to the caller as EvaluationResult::Err(...), or when any error is impossible then expect will make it clearer to anyone who gets an unexpected panic.

…ncy, remove accidentally committed todos Signed-off-by: MaximFischuk <[email protected]>

Signed-off-by: MaximFischuk <[email protected]>

jbovet · 2024-12-30T13:08:01Z

src/hooks/logging.rs

+use super::{Hook, HookContext, HookHints};
+
+/// A hook that logs the evaluation lifecycle of a flag.
+pub struct LoggingHook;


@MaximFischuk please consider adding a print option to the LoggingHook struct for evaluation context; it's important to print the context in a readable JSON format if it is set according to the spec

jbovet · 2024-12-30T13:17:11Z

src/hooks/logging.rs

+        _: &EvaluationDetails<Value>,
+        _: Option<&'a HookHints>,
+    ) -> Result<(), EvaluationError> {
+        log::debug!("After hook for flag {}", context.flag_key);


@MaximFischuk please consider adding fields (Logged data) to the log messages (serialized, opt-in) according to the specification in each Stage.

MaximFischuk changed the title ~~Feature/hooks~~ feat: Add hooks implementation Dec 16, 2024

MaximFischuk force-pushed the feature/hooks branch 4 times, most recently from 1d86d02 to fafe675 Compare December 17, 2024 09:53

MaximFischuk added 4 commits December 18, 2024 15:26

feat: Add implementation of Hooks

b87c512

Signed-off-by: Maxim Fischuk <[email protected]> Signed-off-by: MaximFischuk <[email protected]>

refactor: Remove unused submodules

435f0c0

Signed-off-by: MaximFischuk <[email protected]>

fix: Evaluation context passing on before hook

dfff03a

Signed-off-by: MaximFischuk <[email protected]>

test: Fix tests to do not use global OpenFeature singletone

60ca477

Signed-off-by: MaximFischuk <[email protected]>

MaximFischuk force-pushed the feature/hooks branch from aa8b329 to 60ca477 Compare December 18, 2024 13:29

MaximFischuk marked this pull request as ready for review December 18, 2024 13:35

beeme1mr requested review from sheepduke and gruebel December 18, 2024 13:37

beeme1mr reviewed Dec 18, 2024

View reviewed changes

beeme1mr requested a review from justinabrahms December 18, 2024 16:26

refactor: Update finally hook signature to be compliant to `Require…

a2b6497

…ment 4.3.8` of OpenFeature specification Signed-off-by: MaximFischuk <[email protected]>

MaximFischuk force-pushed the feature/hooks branch from 031f9c5 to a2b6497 Compare December 19, 2024 11:30

MaximFischuk requested a review from beeme1mr December 19, 2024 11:34

gruebel reviewed Dec 26, 2024

View reviewed changes

MaximFischuk added 2 commits December 29, 2024 13:49

refactor: Rename hooks field to client_hooks for clarity and consiste…

af0d637

…ncy, remove accidentally committed todos Signed-off-by: MaximFischuk <[email protected]>

chore: Correct typo in comment regarding resolution result in client.rs

cd0df01

Signed-off-by: MaximFischuk <[email protected]>

jbovet reviewed Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add hooks implementation #95

feat: Add hooks implementation #95

MaximFischuk commented Dec 16, 2024 •

edited

Loading

beeme1mr Dec 18, 2024

MaximFischuk Dec 18, 2024

beeme1mr commented Dec 25, 2024

gruebel left a comment

gruebel Dec 26, 2024

MaximFischuk Dec 29, 2024

gruebel Dec 26, 2024

MaximFischuk Dec 29, 2024

gruebel Dec 26, 2024

MaximFischuk Dec 29, 2024

gruebel Dec 26, 2024

MaximFischuk Dec 29, 2024

gruebel Dec 26, 2024

gruebel Dec 26, 2024

MaximFischuk Dec 29, 2024

gruebel Dec 26, 2024

MaximFischuk Dec 29, 2024

jbovet Dec 30, 2024

jbovet Dec 30, 2024 •

edited

Loading

	async fn finally<'a>(&self, context: &HookContext<'a>, _: Option<&'a HookHints>) {
	async fn finally<'a>(&self, context: &HookContext<'a>, _: &EvaluationDetails<Value>, _: Option<&'a HookHints>) {

	// INFO: Result of the resolution or error reson with default value
	// INFO: Result of the resolution or error reason with default value

feat: Add hooks implementation #95

Are you sure you want to change the base?

feat: Add hooks implementation #95

Conversation

MaximFischuk commented Dec 16, 2024 • edited Loading

This PR

Related Issues

Notes

Follow-up Tasks

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeme1mr commented Dec 25, 2024

gruebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbovet Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

MaximFischuk commented Dec 16, 2024 •

edited

Loading

jbovet Dec 30, 2024 •

edited

Loading