Open tcarrio opened 1 year ago
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.
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.
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?
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.
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.
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.
OK @tcarrio I'm sold on this change then, if @skyerus also agrees.
@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.
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.
@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.
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
We should perhaps roll this into whatever release includes the new client changes.
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.
This is in reference to the hooks evaluation order in Requirement 4.4.2. This reads:
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 theafter
,error
, andfinally
. This appeases the overall ordering of hook executions as listed above. Hence, thebefore
is[...API, ...Client, ...Invocation, ...Provider]
. Theafter
is[...API, ...Client, ...Invocation, ...Provider].reverse()
. So theafter
hooks will have all of theProvider
hooks execute before theInvocation
hooks, those before theClient
hooks, and those before theAPI
hooks.The Python SDK generates the
before
list of hooks and theafter/error/finally
list of hooks by spreading them in their own declarations. So,before
isAPI + Client + Invocation + Provider
and theafter
isProvider + Invocation + Client + API
. So theafter
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:
This detail should be clear from the specification so there is not any discrepancy between implementations of the API.