feat: Add support for hook data.

[kinyoklion]

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

[codecov]

Codecov Report

Attention: Patch coverage is 98.72611% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.47%. Comparing base (568722a) to head (90374f4).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/OpenFeature/OpenFeatureClient.cs	92.59%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
+ Coverage   85.69%   86.47%   +0.77%     
==========================================
  Files          39       42       +3     
  Lines        1601     1671      +70     
  Branches      171      177       +6     
==========================================
+ Hits         1372     1445      +73     
+ Misses        191      187       -4     
- Partials       38       39       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[beeme1mr]

Hey @kinyoklion, thanks for kicking this off. I'll cut some GitHub issues for other languages once this PR is ready so we can use this as a reference.

[kinyoklion]

@toddbaert I would like to get your thoughts on the spec divergence in the description. If we are in agreement I can make a minor change to the spec wording.

[toddbaert]

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).

@kinyoklion I agree with your assertion that allowing any object is relatively safe in this case, and certainly more ergonomic. I think we should adjust the spec accordingly.