feat: Add hook-data concept for hooks. by kinyoklion · Pull Request #273 · open-feature/spec

kinyoklion · 2024-09-30T15:54:14Z

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 <4955475+kinyoklion@users.noreply.github.com>

beeme1mr

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.

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

toddbaert

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 <4955475+kinyoklion@users.noreply.github.com>

kinyoklion · 2024-10-25T22:50:33Z

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.

Create an array the same size as the number of hooks being invoked.
As you iterate the hooks pass the data at the same index in the array as the hook is in its array.
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.

lukas-reining

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.

Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion · 2024-12-19T18:55:38Z

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.

guidobrei

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

toddbaert

👍 This has been open for a while. @kinyoklion feel free to merge whenever, and we can do a release.

I'd like to propose additional hardening of the specification soon, and possibly moving this section out of experimental (probably after we've implemented what's outlined in this PR in a few SDKs). When this is merged I'll create a parent issue in the spec and sub issues in each SDK.

## This PR Adds support for hook data. open-feature/spec#273 ### Related Issues  ### Notes  I realized that the 4.6.1 section of the spec wasn't consistent with the expected usage. Basically it over-specifies the typing of the hook data matching that of the evaluation context. That is one possible approach, it would just mean a bit more work on the part of the hook implementers. In the earlier example in the spec I put a `Span` in the hook data: ``` public Optional<EvaluationContext> before(HookContext context, HookHints hints) { SpanBuilder builder = tracer.spanBuilder('sample') .setParent(Context.current().with(Span.current())); Span span = builder.startSpan() context.hookData.set("span", span); } public void after(HookContext context, FlagEvaluationDetails details, HookHints hints) { // Only accessible by this hook for this specific evaluation. Object value = context.hookData.get("span"); if (value instanceof Span) { Span span = (Span) value; span.end(); } } ``` This is only possible if the hook data allows specification of any `object` instead of being limited to the immutable types of a context. For hooks hook data this is safe because only the hook mutating the data will have access to that data. Additionally the execution of the hook will be in sequence with the evaluation (likely in a single thread). The alternative would be to store data in the hook, and use the hook data to know when to remove it. Something like this: ``` public Optional<EvaluationContext> before(HookContext context, HookHints hints) { SpanBuilder builder = tracer.spanBuilder('sample') .setParent(Context.current().with(Span.current())); Span span = builder.startSpan() String storageId = Uuid(); this.tmpData.set(storageId, span); context.hookData.set("span", storageId); } public void after(HookContext context, FlagEvaluationDetails details, HookHints hints) { // Only accessible by this hook for this specific evaluation. Object value = context.hookData.get("span"); if (value) { String id = value.AsString(); Span span= this.tmpData.get(id); span.end(); } } ``` ### Follow-up Tasks   ### How to test  --------- Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Co-authored-by: chrfwow <christian.lutnik@dynatrace.com>

## This PR Adds support for hook data. open-feature/spec#273 ### Related Issues  ### Notes  I realized that the 4.6.1 section of the spec wasn't consistent with the expected usage. Basically it over-specifies the typing of the hook data matching that of the evaluation context. That is one possible approach, it would just mean a bit more work on the part of the hook implementers. In the earlier example in the spec I put a `Span` in the hook data: ``` public Optional<EvaluationContext> before(HookContext context, HookHints hints) { SpanBuilder builder = tracer.spanBuilder('sample') .setParent(Context.current().with(Span.current())); Span span = builder.startSpan() context.hookData.set("span", span); } public void after(HookContext context, FlagEvaluationDetails details, HookHints hints) { // Only accessible by this hook for this specific evaluation. Object value = context.hookData.get("span"); if (value instanceof Span) { Span span = (Span) value; span.end(); } } ``` This is only possible if the hook data allows specification of any `object` instead of being limited to the immutable types of a context. For hooks hook data this is safe because only the hook mutating the data will have access to that data. Additionally the execution of the hook will be in sequence with the evaluation (likely in a single thread). The alternative would be to store data in the hook, and use the hook data to know when to remove it. Something like this: ``` public Optional<EvaluationContext> before(HookContext context, HookHints hints) { SpanBuilder builder = tracer.spanBuilder('sample') .setParent(Context.current().with(Span.current())); Span span = builder.startSpan() String storageId = Uuid(); this.tmpData.set(storageId, span); context.hookData.set("span", storageId); } public void after(HookContext context, FlagEvaluationDetails details, HookHints hints) { // Only accessible by this hook for this specific evaluation. Object value = context.hookData.get("span"); if (value) { String id = value.AsString(); Span span= this.tmpData.get(id); span.end(); } } ``` ### Follow-up Tasks   ### How to test  --------- Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Co-authored-by: chrfwow <christian.lutnik@dynatrace.com> Signed-off-by: Weihan Li <weihanli@outlook.com>

kinyoklion commented Sep 30, 2024

View reviewed changes

Comment thread specification/sections/04-hooks.md

kinyoklion commented Sep 30, 2024

View reviewed changes

Comment thread specification/sections/04-hooks.md Outdated

kinyoklion commented Sep 30, 2024

View reviewed changes

Comment thread specification/sections/04-hooks.md

feat: Add hook-data concept for hooks.

d391718

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion force-pushed the rlamb/hook-data branch from c42ef76 to d391718 Compare September 30, 2024 15:59

Generate spec json.

2110b92

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion force-pushed the rlamb/hook-data branch from cb5cabb to 2110b92 Compare September 30, 2024 16:00

kinyoklion marked this pull request as ready for review September 30, 2024 16:01

kinyoklion requested a review from toddbaert as a code owner September 30, 2024 16:01

kinyoklion requested a review from beeme1mr September 30, 2024 16:02

beeme1mr reviewed Sep 30, 2024

View reviewed changes

Comment thread specification/sections/04-hooks.md Outdated

Update specification/sections/04-hooks.md

e7a7bf4

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

toddbaert requested review from aepfli, andrewdmaclean, askpt, beeme1mr, benjiro, jonathannorris, justinabrahms and maxveldink October 3, 2024 12:00

toddbaert reviewed Oct 4, 2024

View reviewed changes

Comment thread specification/sections/04-hooks.md Outdated

toddbaert reviewed Oct 4, 2024

View reviewed changes

Comment thread specification/sections/04-hooks.md Outdated

toddbaert reviewed Oct 4, 2024

View reviewed changes

PR feedback.

29bc48c

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion force-pushed the rlamb/hook-data branch from c84eebd to 29bc48c Compare October 25, 2024 22:23

kinyoklion added 4 commits October 25, 2024 15:24

Generate spec.

0ef7524

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Update hook context section.

8d732ac

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Make spec.

7591a03

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Re-fix definitions.

8157ad9

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion mentioned this pull request Dec 2, 2024

feat!: add evaluation details to finally hook #280

Merged

beeme1mr approved these changes Dec 2, 2024

View reviewed changes

lukas-reining approved these changes Dec 4, 2024

View reviewed changes

Comment thread specification/sections/04-hooks.md Outdated

kinyoklion and others added 2 commits December 19, 2024 10:51

Update specification/sections/04-hooks.md

d869ae4

Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Merge branch 'main' into rlamb/hook-data

032c1da

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion requested a review from toddbaert December 19, 2024 18:53

Regenerate spec JSON.

2b3acfd

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

guidobrei approved these changes Dec 20, 2024

View reviewed changes

toddbaert approved these changes Jan 13, 2025

View reviewed changes

Merge branch 'main' into rlamb/hook-data

affaf78

toddbaert merged commit c287b58 into main Jan 15, 2025

toddbaert deleted the rlamb/hook-data branch January 15, 2025 14:29

kinyoklion mentioned this pull request Feb 24, 2025

feat: Add support for hook data. open-feature/dotnet-sdk#387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

feat: Add hook-data concept for hooks.#273
toddbaert merged 12 commits intomainfrom
rlamb/hook-data

kinyoklion commented Sep 30, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beeme1mr left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert left a comment

Uh oh!

kinyoklion commented Oct 25, 2024

Uh oh!

lukas-reining left a comment

Uh oh!

Uh oh!

kinyoklion commented Dec 19, 2024

Uh oh!

guidobrei left a comment

Uh oh!

toddbaert left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

kinyoklion commented Sep 30, 2024

This PR

Related Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beeme1mr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert left a comment

Choose a reason for hiding this comment

Uh oh!

kinyoklion commented Oct 25, 2024

Uh oh!

lukas-reining left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kinyoklion commented Dec 19, 2024

Uh oh!

guidobrei left a comment

Choose a reason for hiding this comment

Uh oh!

toddbaert left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

toddbaert left a comment •

edited

Loading