Hook ordering & state

[justinabrahms]

We need to have a concept of hook ordering. In our case, we have a client hook EmitMetrics. Sometimes, devs don't want to emit metrics, so we want to selectively overwrite it.

client.registerHook(EmitMetrics())

client.getBooleanValues('key', defaultValue) # This emits metrics

client.getBooleanValues('key', defaultValue, FlagOptions(DONT_EMIT_METRICS)) # This doesnt emit metrics.

We're thinking that we'll build this like:

class EmitMetrics:
  def after(ctx, details):
    if not ctx.state['dont-emit-metrics']:
      emitMetrics()

class DONT_EMIT_METRICS:
  def before(ctx):
    ctx.state['dont-emit-metrics'] = True

This means we'll need precise control around when hooks run (with associated APIs to insert at specific ordering) as well as a change to HookContext to hold some state.

[toddbaert]

I think well-defined ordering is certainly a good idea, likely a necessity. I created a stub markdown file for hooks in the spec: specification/flag-evaluation/hooks.md. We should mention that topic when we flesh it out.

I'm less sure about storing arbitrary state on the HookContext. It might be the right approach, but I also am wary of encouraging dependencies between hooks that way. It could be a footgun.

In the sample OTel Hook, we used a private WeakMap member (Java has this concept too: https://docs.oracle.com/javase/7/docs/api/java/util/WeakHashMap.html) to store state, using the Hook Context object reference itself as the key (since the reference doesnt change across flag evaluation, the before, after, etc stages can access the same state). This means that only the OTel hook can access this map, and so only it can modify state stored in this map having to do with it's OTel related logic, maintaining good separation of concerns. Another nice aspect of this pattern is that when the HookContext is garbage collected the values it indexes in the WeakMap are as well.

Do you think this pattern would satisfy your use-case @justinabrahms ?

[justinabrahms]

Without passing state, I'm not clear how we could represent:

I want this hook to do the thing it does on every invocation... except this one in particular.

The other thing we've talked about is having users put some sentinel values within the evaluation context, which feels super gross.

Maybe we have some immutable state that's included on FlagEvaluationOptions when the user kicks off the request?

[toddbaert]

The other thing we've talked about is having users put some sentinel values within the evaluation context, which feels super gross.

Very gross, I couldn't agree more on that. It actually even caused bugs in some vendor SDKs in my JS experimentation, because of circular references that broke JSON parsing (presumably to build an HTTP POST body).

I want this hook to do the thing it does on every invocation... except this one in particular.

Ya, I think that's certainly valid, I'm just not sure about doing it with "side-effects" between hooks.

Maybe we have some immutable state that's included on FlagEvaluationOptions when the user kicks off the request?

I think this is a really good solution, and a perfect use of FlagEvaluationOptions, which is pretty bare at the moment.

I should add, I'm not 100% opposed to the mutable state between hooks, I just suspect it's something we should avoid if possible. I could be wrong though :shrug: .

[toddbaert]

Addressed by https://github.com/open-feature/spec/pull/45. Once that's merged we can likely close this.

[beeme1mr]

Addressed by hook hints