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 evaluation details to finally hook #328

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

bbland1
Copy link
Contributor

@bbland1 bbland1 commented Feb 26, 2025

This PR

  • adds evaluation details to finally hook stage
  • utilized InterfaceEvaluationDetails similar to the After method of the hook

Related Issues

Fixes #327

Notes

This breaks the signature of the finally stages based on this spec enhancement. It is not considered a breaking change to the SDK because hooks are marked as experimental in the spec, and the change has no impact on known hooks.

- Finally(ctx context.Context, hookContext HookContext, hookHints HookHints)
+ Finally(ctx context.Context, hookContext HookContext, flagEvaluationDetails InterfaceEvaluationDetails, hookHints HookHints)

Follow-up Tasks

Update the go-sdk-contrib repo, with a quick search the few places finally is being used that I saw.

How to test

  • Running local unit tests passed again with change to interface.

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.13%. Comparing base (3c70dc2) to head (dca32b5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #328   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files          14       14           
  Lines        1408     1408           
=======================================
  Hits         1241     1241           
  Misses        143      143           
  Partials       24       24           
Flag Coverage Δ
e2e 88.13% <100.00%> (ø)
unit 88.13% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@bbland1 bbland1 marked this pull request as ready for review February 26, 2025 07:24
@bbland1 bbland1 requested a review from a team as a code owner February 26, 2025 07:24
toddbaert added a commit to open-feature/community that referenced this pull request Feb 27, 2025
@bbland1 has contributed 2 recent changes to the Go SDK:

- open-feature/go-sdk#325
- open-feature/go-sdk#328

@bbland1 when merged, this PR will add you to the org. It comes with no obligation but it's the first step on the https://github.com/open-feature/community/blob/main/CONTRIBUTOR_LADDER.md. Please approve or 👍 this PR to signal your interest!

Signed-off-by: Todd Baert <[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.

Looks good to me. This is more straightforward in Go because there's no try/catch causing scope difficulties with the evalDetails as in other SDKs.

Thanks again for your 2nd PR (I think) @bbland1 !

Before we release this we'll have to make this (small) breaking change clear in the release notes.... just saying it now so I don't forget it.

@bbland1 consider joining the org: open-feature/community#433

@beeme1mr
Copy link
Member

Let's make sure to coordinate with @thomaspoignant. This breaking change was easier in other languages since it was unlikely to affect people. However, this change will definitely have at least some impact and we should do what we can to minimize it.

@bbland1
Copy link
Contributor Author

bbland1 commented Feb 27, 2025

@toddbaert thank you, I enjoyed getting to work on the PRs!

@beeme1mr & @thomaspoignant if I remember correctly for go-feature-flag the Finally didn't have specific logic containing just implementing to satisfy the interface so 🤞🏽 impact isn't too bad. I think the open-telemetry maybe had some logic in it's implementation.

@thomaspoignant
Copy link
Member

Sorry for the late reply.
I have open a PR for GO Feature Flag to support the finally open-feature/go-sdk-contrib#641.

@beeme1mr
Copy link
Member

beeme1mr commented Mar 4, 2025

Okay, I'll merged tomorrow unless someone objects. I've also created an issue to upgrade the OTel hook. We'll want to make sure this is released around the same time as the Go SDK.

@beeme1mr beeme1mr merged commit 63de64a into open-feature:main Mar 5, 2025
8 checks passed
@bbland1 bbland1 deleted the update-finally-hook branch March 5, 2025 17:06
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.

Add evaluation details to finally hook stage
5 participants