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

feat: Add hook-data concept for hooks. #273

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

kinyoklion
Copy link
Member

This PR

Add support for the hook-data concept for hooks. Hook-data allows for per-evaluation data to be propagated between hooks.

This is especially useful for analytics purposes where you may want to measure things that happen between stages, or you want to do something like create a span in one stage and close it in another.

This concept is similar to the series data concept for LaunchDarkly hooks. https://github.com/launchdarkly/open-sdk-specs/tree/main/specs/HOOK-hooks#evaluationseriesdata

Unlike series data the data in this approach is mutable. This is because the before stage already has a return value. We could workaround this by specifying a return structure, but it maybe seems more complex. The data is only passed to a specific hook instance, so mutability is not of great concern.

Some functional languages may still need to use an immutable with return values approach.

I can create an OFEP if we think this merits discussion prior to proposal.

Related Issues

Related discussion in a PR comment.
open-feature/java-sdk#1049 (comment)

Signed-off-by: Ryan Lamb <[email protected]>
@kinyoklion kinyoklion marked this pull request as ready for review September 30, 2024 16:01
@kinyoklion kinyoklion requested a review from beeme1mr September 30, 2024 16:02
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.

I like this proposal, but I would like to see how it could be implemented in an SDK. Properly scoping hook data may be complicated.

specification/sections/04-hooks.md Outdated Show resolved Hide resolved
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Ryan Lamb <[email protected]>
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 definitely support this idea. I left a few comments. My biggest question is how we can define this so it's non-breaking for existing hooks.

Signed-off-by: Ryan Lamb <[email protected]>
Signed-off-by: Ryan Lamb <[email protected]>
Signed-off-by: Ryan Lamb <[email protected]>
Signed-off-by: Ryan Lamb <[email protected]>
@kinyoklion
Copy link
Member Author

I like this proposal, but I would like to see how it could be implemented in an SDK. Properly scoping hook data may be complicated.

I should be able to find time to update a OF SDK to demonstrate, but it may take me a bit.

It will be a little different now that we added it to hook context, but you can see what we did in the LD SDKs as its own argument:
https://github.com/launchdarkly/js-core/blob/ae2b5bbed9ec32d9640f1c5a168badaad28174ec/packages/shared/sdk-server/src/hooks/HookRunner.ts#L109

In our case we create the data when we call the hook, and the return is the new data. Which means we can just map it to an array.

But, if that was not the case, as it will be in the OF example you instead do this.

  1. Create an array the same size as the number of hooks being invoked.
  2. As you iterate the hooks pass the data at the same index in the array as the hook is in its array.
  3. Maintain the array for the execution of that series of stages.

Some pseudocode:

const hookData = new Array(hooksForInvocation.length);
hookData.fill({})

...
hooksForInvocation.forEach((hook, index) => executeBeforeEvaluation(hookContext.withData(hookData[index])))

The hooks are stable for a specific invocation of stages, so you just need an equal sized list of hook data.

Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

I like the change and as far as I can see, adding the hook data does not break the old hooks when it is implemented.

specification/sections/04-hooks.md Outdated Show resolved Hide resolved
kinyoklion and others added 2 commits December 19, 2024 10:51
@kinyoklion kinyoklion requested a review from toddbaert December 19, 2024 18:53
Signed-off-by: Ryan Lamb <[email protected]>
@kinyoklion
Copy link
Member Author

I like this proposal, but I would like to see how it could be implemented in an SDK. Properly scoping hook data may be complicated.

I think this was resolved via the pseudocode.

Copy link
Member

@guidobrei guidobrei left a comment

Choose a reason for hiding this comment

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

Not that my approval changes anything, but this proposal looks really promising.

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.

5 participants