open-feature / ofep

A focal point for OpenFeature research, proposals and requests for comments
https://openfeature.dev
20 stars 15 forks source link

Add proposal for Provider Hook #22

Closed s-sen closed 2 years ago

s-sen commented 2 years ago

Signed-off-by: ssen mailssen@gmail.com

toddbaert commented 2 years ago

Just to clarify: is there any reason that we couldn't use our existing hooks abstraction for this? and just attach them at the provider level as specified. My main concern is adding new abstractions... if provider hooks use the same hook abstraction as our existing hooks, I'm more comfortable with this proposal.

I also feel that provider hooks should be "innermost" hooks - they should run last in before, after the application author has run their hooks, and first in error, after, and finally.

toddbaert commented 2 years ago

@s-sen:

Just to clarify: is there any reason that we couldn't use our existing hooks abstraction for this? and just attach them at the provider level as specified? My main concern is adding new abstractions... if provider hooks use the same hook abstraction as our existing hooks, I'm more comfortable with this proposal.

Can you add any clarification on this? To be clear, I'm just making sure that a provider hook would use the same hook interface that already exists... it seems like that's the case.

toddbaert commented 2 years ago

I'd also like to point out I think this makes the contextTransformer concept redundant. Since before hooks can transform context, a provider before hook is a great place to do such transformation. I think that if we add provider hooks, we should completely remove the contextTransformer concept.

cc @thomaspoignant @beeme1mr @weyert @justinabrahms

s-sen commented 2 years ago

Can you add any clarification on this? To be clear, I'm just making sure that a provider hook would use the same hook interface that already exists... it seems like that's the case

Yes, there is no change proposed in the current hook interface. The Provider Hook implementation must use the same interface that is currently defined.

But the application developer should not have to register/add the hook. During running the hooks, the SDK should query the Provider implementation for any provider hooks and if it is provided run the Provider hook in the order that the standard mandates.

thomaspoignant commented 2 years ago

'd also like to point out I think this makes the contextTransformer concept redundant. Since before hooks can transform context, a provider before hook is a great place to do such transformation. I think that if we add provider hooks, we should completely remove the contextTransformer concept.

100% agree, keeping both will be confusing.

justinabrahms commented 2 years ago

I really like that this gets rid of context transformers. Thanks, @s-sen!!

toddbaert commented 2 years ago

@s-sen I'm going to merge this if you agree.

beeme1mr commented 2 years ago

@s-sen, it looks like we have consensus. Please update the OFEP status to APPROVED and I'll merge it in.

Once the OFEP has been merged, feel free to open a PR in the spec repo with the required changes and reference this PR.

Thanks for driving this spec enhancement.

s-sen commented 2 years ago

Sorry - I could not do this earlier. I do not need to do any additional changes in the document I assume?

The next step is to open a PR in the spec repo?

beeme1mr commented 2 years ago

@s-sen, nothing else is required here. The next step would be to open a PR in the spec repo and reference this PR.

Thanks for driving this topic. I think it's a great enhancement.