feat: go context in hook calls

[Kunde21]

⚠️ BREAKING CHANGE ⚠️

This PR is a breaking change of the unstable Hooks API.
Structures implementing the Hook interface will need to be modified.

This PR

Include the go context in hook methods, to maintain the request context through the hooks and enabling request-scoped actions.

Related Issues

Fixes #159

Notes

There's no good way to pass the context from one hook call to the next in the chain. If the goal is to use hooks like HTTP middleware or gRPC interceptors, then this breaks the pattern.

[skyerus]

Thank you for the contribution!
Please sign your commit to pass the DCO check.

[codecov]

Codecov Report

Merging #163 (85ba23d) into main (96d3169) will not change coverage.
The diff coverage is 93.75%.

@@           Coverage Diff           @@
##             main     #163   +/-   ##
=======================================
  Coverage   71.25%   71.25%           
=======================================
  Files           8        8           
  Lines         668      668           
=======================================
  Hits          476      476           
  Misses        174      174           
  Partials       18       18           

Impacted Files	Coverage Δ
pkg/openfeature/hooks.go	29.03% <75.00%> (ø)
pkg/openfeature/client.go	71.86% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[toddbaert]

There's no good way to pass the context from one hook call to the next in the chain. If the goal is to use hooks like HTTP middleware or gRPC interceptors, then this breaks the pattern.

I don't believe that we generally want to encourage interdependence between hooks, so my initial reaction is that this is OK... but I'm wondering what other use-cases we would be preventing here.

Particularly interested in @skyerus and @james-milligan 's opinions on this.

[skyerus]

There's no good way to pass the context from one hook call to the next in the chain. If the goal is to use hooks like HTTP middleware or gRPC interceptors, then this breaks the pattern.

I don't believe that we generally want to encourage interdependence between hooks, so my initial reaction is that this is OK... but I'm wondering what other use-cases we would be preventing here.

Particularly interested in @skyerus and @james-milligan 's opinions on this.

Do other SDKs behave in this manor? An example of a limitation would be that a user couldn't use a Before hook to start a timer and an After hook to end the timer and log the transaction time. It does seem somewhat limiting.

[toddbaert]

There's no good way to pass the context from one hook call to the next in the chain. If the goal is to use hooks like HTTP middleware or gRPC interceptors, then this breaks the pattern.

I don't believe that we generally want to encourage interdependence between hooks, so my initial reaction is that this is OK... but I'm wondering what other use-cases we would be preventing here.
Particularly interested in @skyerus and @james-milligan 's opinions on this.

Do other SDKs behave in this manor? An example of a limitation would be that a user couldn't use a Before hook to start a timer and an After hook to end the timer and log the transaction time. It does seem somewhat limiting.

Other SDKs don't use a go context.Context, obviously, but they do pass the HookContext, which also contains the EvaluationContext (wow "context" overload).

Personally, I think using the EvaluationContext as a vehicle for communicating between hooks is a bad idea™️. IMHO it can promote dependence between hooks which would be troublesome and difficult to debug. A better strategy instead, I think, is to use weak-maps, with the hookContext being a weak-map key to some value which stores whatever shared state you want for this hook execution. However, I'd still only recommend that for a single hook that has multiple stages it wants to communicate between.

I guess the Go equivalent to what I'm saying above would be a hook that has a before and after, in which you want to start/stop a timer. If the before/after stages get the same context, I think that would be sufficient for the use-cases we want to encourage. That seems less important than hook1 and hook2 sharing the same context, but I'm not sure.

[thisthat]

There's no good way to pass the context from one hook call to the next in the chain. If the goal is to use hooks like HTTP middleware or gRPC interceptors, then this breaks the pattern.

I don't believe that we generally want to encourage interdependence between hooks, so my initial reaction is that this is OK... but I'm wondering what other use-cases we would be preventing here.
Particularly interested in @skyerus and @james-milligan 's opinions on this.

Do other SDKs behave in this manor? An example of a limitation would be that a user couldn't use a Before hook to start a timer and an After hook to end the timer and log the transaction time. It does seem somewhat limiting.

Other SDKs don't use a go context.Context, obviously, but they do pass the HookContext, which also contains the EvaluationContext (wow "context" overload).

Personally, I think using the EvaluationContext as a vehicle for communicating between hooks is a _bad idea_™️. IMHO it can promote dependence between hooks which would be troublesome and difficult to debug. A better strategy instead, I think, is to use weak-maps, with the hookContext being a weak-map key to some value which stores whatever shared state you want for this hook execution. However, I'd still only recommend that for a single hook that has multiple stages it wants to communicate between.

I guess the Go equivalent to what I'm saying above would be a hook that has a before and after, in which you want to start/stop a timer. If the before/after stages get the same context, I think that would be sufficient for the use-cases we want to encourage. That seems less important than hook1 and hook2 sharing the same context, but I'm not sure.

IMHO hooks should be seen as events and be functional and don't rely on any carry-over state. They should rely only on the properties of the HookContext to do their own work.
Furthermore, I consider this a security problem since it means basically leaking the SDK context to external actors on purpose.

[toddbaert]

IMHO hooks should be seen as events and be functional and don't rely on any carry-over state. They should rely only on the properties of the HookContext to do their own work.
Furthermore, I consider this a security problem since it means basically leaking the SDK context to external actors on purpose.

Yep, I pretty much agree. I think maintaining state between STAGES in the same hook is valid, but that state should not be maintained in the evaluation context (or even the hook context IMHO). That's why weakmaps are great. The go context.Context may also be valid for this purpose, but that's what I'm not 100% about. @thisthat

[thisthat]

IMHO hooks should be seen as events and be functional and don't rely on any carry-over state. They should rely only on the properties of the HookContext to do their own work.
Furthermore, I consider this a security problem since it means basically leaking the SDK context to external actors on purpose.

Yep, I pretty much agree. I think maintaining state between STAGES in the same hook is valid, but that state should not be maintained in the evaluation context (or even the hook context IMHO). That's why weakmaps are great. The go context.Context may also be valid for this purpose, but that's what I'm not 100% about. @thisthat

@toddbaert a weakmap would be perfect for this use case, but Go doesn't have them :( Creating a new context.Context and passing it through the hooks of a single request would be the best approach for devexp IMHO.

[toddbaert]

@toddbaert a weakmap would be perfect for this use case, but Go doesn't have them :( Creating a new context.Context and passing it through the hooks of a single request would be the best approach for devexp IMHO.

I this PR, we are using the context passed in to the evaluation. When you say "Creating a new context.Context" are you meaning something different?

[skyerus]

To state the problem declaratively (to avoid confusion, I got myself confused by the problem statement initially).

Given that each of the hook's functions (Before/After/Finally/Error) are applied on the same structure, there is already the ability to pass data between the hook functions. The concern is when it comes to chaining data between independent hooks e.g. no way to pass data from hook1.Before() to hook2.Before().
It seems that we have reached consensus that sharing data between hook structures is not a good idea, I am also of this mind.

In which case, the next steps for this PR are to fix the linting and DCO check. @Kunde21 are you happy to do this? If not, I'm happy to takeover.

[toddbaert]

To state the problem declaratively (to avoid confusion, I got myself confused by the problem statement initially).

Given that each of the hook's functions (Before/After/Finally/Error) are applied on the same structure, there is already the ability to pass data between the hook functions. The concern is when it comes to chaining data between independent hooks e.g. no way to pass data from hook1.Before() to hook2.Before(). It seems that we have reached consensus that sharing data between hook structures is not a good idea, I am also of this mind.

In which case, the next steps for this PR are to fix the linting and DCO check. @Kunde21 are you happy to do this? If not, I'm happy to takeover.

Agreed on all counts.

[Kunde21]

@skyerus - Fixed up the commit. Should cover the issues in the workflow checks.

[skyerus]

@Kunde21 thank you!

As the PR currently stands, this will be a breaking change. As Hooks are experimental, we could argue that a breaking change is ok without bumping the major version.

To make this a non-breaking change we'd be forced to convert places that currently take a Hook as a parameter to be type interface{} and cast to the existing Hook interface or HookWithContext interface, then taking the appropriate action. Imo, losing the type safety & the additional code bloat is not worth it. I'd prefer to allow the change as it is.

What are your thoughts? @toddbaert @beeme1mr @Kavindu-Dodan @james-milligan

[beeme1mr]

Hey @skyerus, this is OTel handled a breaking change to an unstable API. I think we could do something similar here.

[toddbaert]

Hey @skyerus, this is OTel handled a breaking change to an unstable API. I think we could do something similar here.

Ya, I'm in favor of highlighting it loudly in a release doc and keeping it as a minor increment.