search

open-feature / spec

OpenFeature specification
https://openfeature.dev
Apache License 2.0
597 stars 35 forks source link

Remove context transformers. Add provider hooks #119

Closed justinabrahms closed 1 year ago

justinabrahms commented 1 year ago

refs https://github.com/open-feature/research/pull/22

beeme1mr commented 1 year ago

Hey @justinabrahms, could you please signoff your commit?

justinabrahms commented 1 year ago

Signed off again.

weyert commented 1 year ago

So we are now adding support for provider hooks which appears to call getProviderHooks on the provider implementation. I am wondering what happens when someone would callsOpenFeature.clearHooks()?

Would this also remove these provider hooks or only the user added hooks?

Or is my assumption incorrect that the getProviderHooks will register the defined hooks in OpenFeature-singleton or are two separate things?

My concern if you have these provider hooks e.g. to cherry pick some elements from the evaluation context so it can be used in the provider when resolving a feature flag and someone calls clearHooks this might get lost and the evaluation context might not correctly processed/transformed.

justinabrahms commented 1 year ago

@weyert Great call out. I would expect that this wouldn't remove provider hooks, which may be necessary for a provider to function. Does that make sense to you?

s-sen commented 1 year ago

What is the use case of removing Provider hook? This hook is optionally supplied by the Provider and the app developer has no knowledge of it. Certain default features carried out by provider hook can be overridden by the app developer using hook hints if the provider chooses to document such option. But clear hook operation initiated by the app developer should not remove provider hooks.

toddbaert commented 1 year ago

It seems like we've basically identified application author-facing hooks and provider hooks. The clear hooks method is meant for the application author and probably shouldn't impact the provider hooks which the application author isn't aware of...

I'm not sure the clear hooks method is even in the spec (which IMO is fine). Because it's not spec'd I don't think we need to add this edge case to the spec about not removing provider hooks when it's called.

Please correct me if I'm wrong.

beeme1mr commented 1 year ago

Hey @justinabrahms, we should be good to merge this PR once all conversations have been resolved. Once this is merged, I plan on releasing v0.2.0.

agentgonzo commented 1 year ago

I'm late to the party here (I was off on holiday).

Hooks MUST be evaluated in the following order: - before: API, Client, Invocation, Provider - after: Provider, Invocation,

When talking about context transformation, why are we recommending that it is done via a hook? Why not just recommend having the provider layer do the transformation within the Flag Evaluation stage?

As I understand it, the vender-specific format of the evaluation context is only needed by the vendor's SDK. As such, it (IMO) should be encapsulated within the provider layer.

By transforming the context as a hook, we are introducing encapsulation leakage and passing the transformed context back to the OpenFeature SDK (only for it to be passed back into the provider layer by the flag evaluation stage).

Would it not be simpler (and better encapsulated) to leave the context transformation entirely within the flag evaluation stage of the provider?

WRT to the original messages on Slack about adding a provider hook to allow setting additional parameters to the context so that they can be logged by OpenTelemetry - that makes perfect sense and it's a good idea to add the vendor hooks as stated.

I'm just thinking that it's better to recommend transforming the context as part of the provider's Flag Evaluation stage rather than a hook.

@justinabrahms @beeme1mr , thoughts?

agentgonzo commented 1 year ago

Additionally, I think it would be better to change the MUST for the provider hook to be a MAY. I don't like enforcing behaviour that may end up being a no-op.

If we change it to MAY, then we can say "if you provide a hook, it WILL be called", but if you don't provide a hook, then the OpenFeature SDK will just skip over to the next stage. This is easily provided by the SDK itself as a default no-op call.

@justinabrahms @beeme1mr ?

toddbaert commented 1 year ago

By transforming the context as a hook, we are introducing encapsulation leakage and passing the transformed context back to the OpenFeature SDK (only for it to be passed back into the provider layer by the flag evaluation stage).

This is a good observation, and I think I agree with your conclusion, and discussions yesterday with @beeme1mr and @justinabrahms kinda elucidated the same problem.

I think we can keep the ordering as is, since provider-related side effects should happen as close to the provider as possible, but we should remove the additional recommendation that context transformation be done in hooks.

I'm just thinking that it's better to recommend transforming the context as part of the provider's Flag Evaluation stage rather than a hook.

Sounds like we're on the same page.

justinabrahms commented 1 year ago

Additionally, I think it would be better to change the MUST for the provider hook to be a MAY. I don't like enforcing behaviour that may end up being a no-op.

In java, we're just providing a default noop implementation as part of the interface. I don't feel strongly on this wording.

toddbaert commented 1 year ago

https://github.com/open-feature/spec/pull/129 Here is the wording change, as well as a removal of the entire context transformer concept, which so far has only been adopted in the JS SDK.