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

Hook evaluation specification clarity #164

Open
tcarrio opened this issue Nov 27, 2022 · 14 comments
Open

Hook evaluation specification clarity #164

tcarrio opened this issue Nov 27, 2022 · 14 comments

Comments

@tcarrio
Copy link
Member

tcarrio commented Nov 27, 2022

This is in reference to the hooks evaluation order in Requirement 4.4.2. This reads:

Hooks MUST be evaluated in the following order:

  • before: API, Client, Invocation, Provider
  • after: Provider, Invocation, Client, API
  • error (if applicable): Provider, Invocation, Client, API
  • finally: Provider, Invocation, Client, API

Currently this is implemented differently between the Python and JavaScript SDKs, but neither is technically wrong based on the loose specification.

The JS SDK generates the before list of hooks, then reverses it for the after, error, and finally. This appeases the overall ordering of hook executions as listed above. Hence, the before is [...API, ...Client, ...Invocation, ...Provider]. The after is [...API, ...Client, ...Invocation, ...Provider].reverse(). So the after hooks will have all of the Provider hooks execute before the Invocation hooks, those before the Client hooks, and those before the API hooks.

The Python SDK generates the before list of hooks and the after/error/finally list of hooks by spreading them in their own declarations. So, before is API + Client + Invocation + Provider and the after is Provider + Invocation + Client + API. So the after hooks satisfy the same as mentioned above for the JS SDK.

The issue here is that the overall hook execution order would vary if any of these hooks have more than one entry. For example:

API = [A, B]
Client = [C, D]
Invocation = [E, F]
Provider = [G, H]

# `before` list
before_hooks = API + Client + Invocation + Provider

# spreading directly (like Python)
list1 = Provider + Invocation + Client + API
# results in [G, H, E, F, C, D, A, B]

# reversing the before list (like JavaScript)
list2 = before_hooks[:]
list2.reverse()
# results in [H, G, F, E, D, C, B, A]

This detail should be clear from the specification so there is not any discrepancy between implementations of the API.

@toddbaert
Copy link
Member

toddbaert commented Nov 29, 2022

I think this was a conscious choice, not an oversight. Every specified behavior means tests and maintenance so there's a cost associated with specifying in such detail... But beyond this, we felt specifying ordering of hooks within stage was a bad idea. It would take some time to dig up the conversation, but I'll leave my recollection:

The main reason is it implies that hooks might have (side)-effects that impact the current stage (for example, Hook1.before performs some activity that Hook2.before depends on). We believed this was a bad pattern, and encouraged poor separation of concerns. Due to the fact that we did not want to encourage this, the ordering of hooks within a particular stage was intentionally unspecified. As it stands, the ordering guarantees of hooks currently are limited to stages, which are quite explicit.

I'm open to changing this, but I'd like a fairly wide consensus and some solid reasoning if we do.

cc @justinabrahms @beeme1mr @moredip because if memory serves, you guys were part of the conversation I'm trying to recall.

@beeme1mr
Copy link
Member

My initial reaction is that we should specify order explicitly in the spec. I seem to recall that hooks were expected to run first-in-last-out.

@toddbaert
Copy link
Member

My initial reaction is that we should specify order explicitly in the spec. I seem to recall that hooks were expected to run first-in-last-out.

In your opinion, what value does this buy us if we don't want to establish dependencies between hooks in the same stage? Or has the opinion on that changed?

@tcarrio
Copy link
Member Author

tcarrio commented Jan 2, 2023

I think the issue is primarily in the expected behavior of an OpenFeature SDK pertaining to hook execution. If I have Python, PHP, and JavaScript services, I would expect the execution of these to be the same in each of these.

Furthermore, there is already a dependency between hooks in the current behavior defined in the specification. If a before hook throws an error, subsequent before hooks will not execute. Similarly this is true for after hooks. The fact that the behavior of one can impact further hooks is a dependency.

Referencing 4.4.2: Suppose I had a logging hook registered for each SDK, but Python and PHP follow hook ordering as the original hook arrays of each component instead of the before hook array entirely reversed. Now, it's possible that the after log hook does not occur for Python and PHP, but it does for JavaScript, solely due to the opinion of the SDK implementation.

Do I now have to register my hooks differently for different languages just to provide reproducible behavior? And even then, it's still not possible to entirely avoid this scenario if you're using these at the API layer- it will inevitably vary between languages.

@toddbaert
Copy link
Member

Furthermore, there is already a dependency between hooks in the current behavior defined in the specification. If a before hook throws an error, subsequent before hooks will not execute.

I wouldn't consider this a dependency between hooks. It's just describing short circuit behavior in cases of abnormal execution. That seems different than hook2 requiring hook1 to have performed some side effect to me.

Do I now have to register my hooks differently for different languages just to provide reproducible behavior?

Ya, I can see how this is undesirable. I'm starting to come around to the idea that it might be worth specifying this. I would like to have other opinions though.

@skyerus
Copy link
Contributor

skyerus commented Jan 3, 2023

I think the issue is primarily in the expected behavior of an OpenFeature SDK pertaining to hook execution. If I have Python, PHP, and JavaScript services, I would expect the execution of these to be the same in each of these.

This has sold me on the idea. I can't see any harm in having the execution order behaviour defined, the ordering can be disregarded if not important to the developer.

@toddbaert
Copy link
Member

OK @tcarrio I'm sold on this change then, if @skyerus also agrees.

@beeme1mr
Copy link
Member

beeme1mr commented Jan 3, 2023

@toddbaert can do quickly audit which SDKs would be affected by this change? I think it's worth doing but I would like to understand the impact.

@tcarrio
Copy link
Member Author

tcarrio commented Jan 3, 2023

An audit would be good. We similarly updated the PHP SDK to follow the reverse pattern, and I believe I updated Python to do the same. But an official review of all of them would be a good idea.

@beeme1mr
Copy link
Member

beeme1mr commented Jan 3, 2023

@tcarrio can you confirm PHP and Python behave this way already? I can double-check JS but I'm nearly positive it's using the reverse pattern as well.

@beeme1mr
Copy link
Member

beeme1mr commented Jan 3, 2023

As Tom stated above, the JS SDK does reverse the order in the after, error, and finally stages.

https://github.com/open-feature/js-sdk/blob/main/src/client.ts#L192

@tcarrio
Copy link
Member Author

tcarrio commented Jan 4, 2023

PHP SDK uses the reversal approach

Reference

@toddbaert
Copy link
Member

We should perhaps roll this into whatever release includes the new client changes.

@beeme1mr
Copy link
Member

beeme1mr commented Mar 7, 2023

Hey @tcarrio, would you be willing to add this to the spec? It sounds like we have consensus that it should be explicitly defined and the reverse order is preferred approach.

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

No branches or pull requests

4 participants