feat: add flag key to hash in fractional evaluation by craigpastro · Pull Request #847 · open-feature/flagd

craigpastro · 2023-08-17T21:53:46Z

This PR

This PR adds the flag key as input to the hash in fractional evaluation. Since the evaluation function only has access to user-supplied logic and context, which almost certainly will not contain the flag key, in order to get this data to the evaluation function I add a flagdProperties struct to the context via a namespaced key $flagd. The flagdProperties struct contains the flag key, which is then used as input to the hash.

Related Issues

Part 2 of #843.

Notes

Follow-up Tasks

How to test

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

netlify · 2023-08-17T21:53:52Z

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	`8c59388`
🔍 Latest deploy log	https://app.netlify.com/sites/polite-licorice-3db33c/deploys/64e601bcf53da90008d653b2

core/pkg/eval/fractional_evaluation_test.go

core/pkg/eval/json_evaluator.go

toddbaert · 2023-08-21T14:44:50Z

@craigpastro

Could we add another test that just directly references $flagd.flag_key in a rule? Something that returns a value if flagd.flag_key === {some-known-value}. I know nobody would often write a rule like this in real life, but it will assure that we are actually populating the value as expected. See json logic do here.

toddbaert · 2023-08-21T15:18:28Z

Related: #851

…rov-ii

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

craigpastro · 2023-08-21T21:55:59Z

Could we add another test that just directly references $flagd.flag_key in a rule? Something that returns a value if flagd.flag_key === {some-known-value}. I know nobody would often write a rule like this in real life, but it will assure that we are actually populating the value as expected. See json logic do here.

I've added a test. Is it what you had in mind?

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

core/pkg/eval/json_evaluator_test.go

core/pkg/eval/json_evaluator.go

core/pkg/eval/fractional_evaluation.go

toddbaert

Once this and this are resolved, I think I can approve.

toddbaert · 2023-08-22T17:17:17Z

Could we add another test that just directly references $flagd.flag_key in a rule? Something that returns a value if flagd.flag_key === {some-known-value}. I know nobody would often write a rule like this in real life, but it will assure that we are actually populating the value as expected. See json logic do here.

I've added a test. Is it what you had in mind?

yes!

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

codecov · 2023-08-22T17:30:41Z

Codecov Report

Merging #847 (8c59388) into main (88d832a) will decrease coverage by 0.14%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
- Coverage   72.70%   72.56%   -0.14%     
==========================================
  Files          27       27              
  Lines        2729     2759      +30     
==========================================
+ Hits         1984     2002      +18     
- Misses        661      669       +8     
- Partials       84       88       +4

Files Changed	Coverage Δ
core/pkg/eval/json_evaluator.go	`84.58% <67.85%> (-2.92%)`	⬇️
core/pkg/eval/fractional_evaluation.go	`67.46% <100.00%> (+0.80%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

craigpastro · 2023-08-22T17:52:14Z

The length of evaluateVariant is failing the linter. I don't see any nice way to restructure the function. I can make it pass by removing whitespace but that doesn't seem like a good idea, and if more properties are added to flagdProperties it will just fail again in the future. Added a // nolint:funlen is an option.

core/pkg/eval/json_evaluator.go

toddbaert

LGTM. I think we can differ documenting this feature until we decide to add other properties (such as current_timestamp).

We can tackle the changes described here in a separate PR.

toddbaert · 2023-08-22T18:36:22Z

The length of evaluateVariant is failing the linter. I don't see any nice way to restructure the function. I can make it pass by removing whitespace but that doesn't seem like a good idea, and if more properties are added to flagdProperties it will just fail again in the future. Added a // nolint:funlen is an option.

I've added the nolint. A lot of length in this func is coming from debug messages and helpful comments. It doesn't read as overly complex to me.

There's also a very small negative delta in codecov, but I'm fine with merging as is.

🤖 I have created a release *beep* *boop* --- <details><summary>flagd: 0.6.4</summary> ## [0.6.4](flagd/v0.6.3...flagd/v0.6.4) (2023-08-30) ### 🐛 Bug Fixes * **deps:** update module github.com/cucumber/godog to v0.13.0 ([#855](#855)) ([5b42486](5b42486)) * **deps:** update module github.com/open-feature/flagd/core to v0.6.3 ([#794](#794)) ([9671964](9671964)) ### 🧹 Chore * **deps:** update golang docker tag to v1.21 ([#822](#822)) ([effe29d](effe29d)) </details> <details><summary>flagd-proxy: 0.2.9</summary> ## [0.2.9](flagd-proxy/v0.2.8...flagd-proxy/v0.2.9) (2023-08-30) ### 🐛 Bug Fixes * **deps:** update module github.com/open-feature/flagd/core to v0.6.3 ([#794](#794)) ([9671964](9671964)) ### 🧹 Chore * **deps:** update golang docker tag to v1.21 ([#822](#822)) ([effe29d](effe29d)) </details> <details><summary>core: 0.6.4</summary> ## [0.6.4](core/v0.6.3...core/v0.6.4) (2023-08-30) ### 🐛 Bug Fixes * **deps:** update kubernetes packages to v0.28.0 ([#841](#841)) ([cc195e1](cc195e1)) * **deps:** update kubernetes packages to v0.28.1 ([#860](#860)) ([f3237c2](f3237c2)) * **deps:** update module github.com/open-feature/open-feature-operator to v0.2.36 ([#799](#799)) ([fa4da4b](fa4da4b)) * **deps:** update module golang.org/x/crypto to v0.12.0 ([#797](#797)) ([edae3fd](edae3fd)) * **deps:** update module golang.org/x/net to v0.14.0 ([#798](#798)) ([92c2f26](92c2f26)) * **deps:** update module sigs.k8s.io/controller-runtime to v0.15.1 ([#795](#795)) ([13d62fd](13d62fd)) * **deps:** update module sigs.k8s.io/controller-runtime to v0.16.0 ([#856](#856)) ([88d832a](88d832a)) ### ✨ New Features * add flag key to hash in fractional evaluation ([#847](#847)) ([ca6a35f](ca6a35f)) * add gRPC healthchecks ([#863](#863)) ([da30b7b](da30b7b)) * support nested props in fractional evaluator ([#869](#869)) ([50ff739](50ff739)) ### 🧹 Chore * deprecate fractionalEvaluation for fractional ([#873](#873)) ([243fef9](243fef9)) * replace xxh3 with murmur3 in bucket algorithm ([#846](#846)) ([c3c9e4e](c3c9e4e)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

feat: add flag key to hash in fractional evaluation

1ac0674

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

craigpastro commented Aug 17, 2023

View reviewed changes

core/pkg/eval/fractional_evaluation_test.go Show resolved Hide resolved

toddbaert reviewed Aug 21, 2023

View reviewed changes

core/pkg/eval/json_evaluator.go Outdated Show resolved Hide resolved

toddbaert reviewed Aug 21, 2023

View reviewed changes

core/pkg/eval/json_evaluator.go Outdated Show resolved Hide resolved

toddbaert requested review from Kavindu-Dodan, bacherfl and thisthat August 21, 2023 14:59

toddbaert mentioned this pull request Aug 21, 2023

feature: add standard set of "ambient" props to context #851

Closed

Craig Pastro added 2 commits August 21, 2023 12:55

Merge remote-tracking branch 'upstream/main' into fractional-eval-imp…

7254dd1

…rov-ii

Update based on feedback

2dce26e

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

craigpastro marked this pull request as ready for review August 21, 2023 21:56

craigpastro requested a review from a team August 21, 2023 21:56

Arrange a bit better

41d9a1f

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

bacherfl reviewed Aug 22, 2023

View reviewed changes

core/pkg/eval/json_evaluator_test.go Outdated Show resolved Hide resolved

bacherfl reviewed Aug 22, 2023

View reviewed changes

core/pkg/eval/json_evaluator.go Outdated Show resolved Hide resolved

Merge branch 'main' into fractional-eval-improv-ii

9ab6acd

toddbaert reviewed Aug 22, 2023

View reviewed changes

core/pkg/eval/fractional_evaluation.go Outdated Show resolved Hide resolved

toddbaert requested changes Aug 22, 2023

View reviewed changes

toddbaert mentioned this pull request Aug 22, 2023

docs: add specification for in-process flagd provider implementations #848

Merged

Update based on feedback

1eaa3af

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

fixup: override funlen lint (mostly comments, debug)

da4fc4d

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

toddbaert reviewed Aug 22, 2023

View reviewed changes

core/pkg/eval/json_evaluator.go Show resolved Hide resolved

toddbaert self-requested a review August 22, 2023 17:55

toddbaert approved these changes Aug 22, 2023

View reviewed changes

toddbaert requested review from bacherfl and beeme1mr August 22, 2023 17:58

toddbaert mentioned this pull request Aug 22, 2023

Improvements to fractional custom evalutor #843

Closed

3 tasks

beeme1mr approved these changes Aug 22, 2023

View reviewed changes

Merge branch 'main' into fractional-eval-improv-ii

8c59388

toddbaert merged commit ca6a35f into open-feature:main Aug 23, 2023

github-actions bot mentioned this pull request Aug 22, 2023

chore: release main #796

Merged

craigpastro deleted the fractional-eval-improv-ii branch August 23, 2023 15:36

github-actions bot mentioned this pull request Dec 2, 2023

chore: release main #1038

Merged

Conversation

craigpastro commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR

Related Issues

Notes

Follow-up Tasks

How to test

Uh oh!

netlify bot commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for polite-licorice-3db33c canceled.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toddbaert commented Aug 21, 2023

Uh oh!

craigpastro commented Aug 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert left a comment

Choose a reason for hiding this comment

Uh oh!

toddbaert commented Aug 22, 2023

Uh oh!

codecov bot commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

craigpastro commented Aug 22, 2023

Uh oh!

Uh oh!

toddbaert left a comment

Choose a reason for hiding this comment

Uh oh!

toddbaert commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

craigpastro commented Aug 17, 2023 •

edited

Loading

netlify bot commented Aug 17, 2023 •

edited

Loading

toddbaert commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

toddbaert commented Aug 22, 2023 •

edited

Loading