Skip to content

Remove context transformers. Add provider hooks#119

Merged
justinabrahms merged 3 commits into
mainfrom
provider-hooks
Aug 5, 2022
Merged

Remove context transformers. Add provider hooks#119
justinabrahms merged 3 commits into
mainfrom
provider-hooks

Conversation

@justinabrahms

Copy link
Copy Markdown
Member

@justinabrahms justinabrahms force-pushed the provider-hooks branch 2 times, most recently from 64a00cc to 8fcc213 Compare August 2, 2022 19:06
Comment thread specification/sections/02-providers.md Outdated
Comment thread specification/sections/02-providers.md Outdated
Comment thread specification/sections/02-providers.md Outdated

@beeme1mr beeme1mr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@beeme1mr

beeme1mr commented Aug 3, 2022

Copy link
Copy Markdown
Member

Hey @justinabrahms, could you please signoff your commit?

@beeme1mr beeme1mr linked an issue Aug 3, 2022 that may be closed by this pull request
Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
@justinabrahms

Copy link
Copy Markdown
Member Author

Signed off again.

@toddbaert toddbaert left a comment

Copy link
Copy Markdown
Member

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.

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

Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
@weyert

weyert commented Aug 4, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

s-sen commented Aug 4, 2022

Copy link
Copy Markdown

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

toddbaert commented Aug 5, 2022

Copy link
Copy Markdown
Member

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.

Comment thread specification/sections/02-providers.md Outdated
@toddbaert toddbaert self-requested a review August 5, 2022 12:31
@beeme1mr beeme1mr requested a review from james-milligan August 5, 2022 14:54
@beeme1mr

beeme1mr commented Aug 5, 2022

Copy link
Copy Markdown
Member

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 <toddbaert@gmail.com>
Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
@agentgonzo

agentgonzo commented Aug 9, 2022

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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