Skip to content
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

Remove context transformers. Add provider hooks #119

Merged
merged 3 commits into from
Aug 5, 2022
Merged

Conversation

justinabrahms
Copy link
Member

specification/sections/02-providers.md Outdated Show resolved Hide resolved
specification/sections/02-providers.md Outdated Show resolved Hide resolved
specification/sections/02-providers.md Outdated Show resolved Hide resolved
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@beeme1mr
Copy link
Member

beeme1mr commented Aug 3, 2022

Hey @justinabrahms, could you please signoff your commit?

@beeme1mr beeme1mr linked an issue Aug 3, 2022 that may be closed by this pull request
@justinabrahms
Copy link
Member Author

Signed off again.

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to modify 4.4.1 to include provider hooks... (Also, I'm not sure what it's trying to say when it mentions flag evaluation options)

Added a few other comments elsewhere.

specification/sections/02-providers.md Outdated Show resolved Hide resolved
Provider interface must add hook mechanism.
Make wording clearer.
Ensure Provider is listed alongside other hook registrars.

Signed-off-by: Justin Abrahms <[email protected]>
@weyert
Copy link
Contributor

weyert commented Aug 4, 2022

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
Copy link
Member Author

@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
Copy link

s-sen commented Aug 4, 2022

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
Copy link
Member

toddbaert commented Aug 5, 2022

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.

@toddbaert toddbaert self-requested a review August 5, 2022 12:31
@beeme1mr
Copy link
Member

beeme1mr commented Aug 5, 2022

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.

Co-authored-by: Todd Baert <[email protected]>
Signed-off-by: Justin Abrahms <[email protected]>
@agentgonzo
Copy link
Member

agentgonzo commented Aug 9, 2022

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provider Hook Spec
8 participants