feat: support nested props in fractional evaluator by craigpastro · Pull Request #869 · open-feature/flagd

craigpastro · 2023-08-28T17:28:41Z

This PR

This PR adds the ability to use nested properties for the bucket key in fractional evaluation. It is a breaking change from the current behaviour. If the bucket key is not provided it will default to {"cat": [{"var":"$flagd.flagKey"}, {"var":"targetingKey"}]}.

@toddbaert I am not sure that my approach to the missing bucket key is the best. I would have preferred to add {"cat": [{"var":"$flagd.flagKey"}, {"var":"targetingKey"}]} to the targeting object and let JsonLogic handle it, but it would be a bit clumsy and involve string manipulation. I don't think there is a nice way to do it, but I'll play around a bit more.

Related Issues

See #848.
Closes #843.

Notes

Follow-up Tasks

How to test

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

netlify · 2023-08-28T17:28:46Z

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

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

codecov · 2023-08-28T17:33:02Z

Codecov Report

Merging #869 (eafa4f7) into main (e007fcc) will increase coverage by 0.14%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
+ Coverage   72.67%   72.81%   +0.14%     
==========================================
  Files          27       27              
  Lines        2759     2755       -4     
==========================================
+ Hits         2005     2006       +1     
+ Misses        667      664       -3     
+ Partials       87       85       -2

Files Changed	Coverage Δ
core/pkg/eval/json_evaluator.go	`85.61% <ø> (ø)`
core/pkg/eval/fractional_evaluation.go	`72.15% <88.88%> (+4.68%)`	⬆️

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

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

core/pkg/eval/fractional_evaluation.go

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

toddbaert

Tested locally and works as expected. Looks good to me except one small thing which I think is technically a violation of the spec, here.

Otherwise I have left some nits which I would love your input on. To reduce confusion, I've proposed bucketing property instead of target property since the latter has a bit too much overlap with targetinKey and they aren't exactly the same thing. I hope that make sense 😅

Thanks again for your help @craigpastro

core/pkg/eval/fractional_evaluation.go

docs/other_resources/in-process-providers/evaluators/fractional_evaluation.md

core/pkg/eval/fractional_evaluation.go

docs/other_resources/in-process-providers/evaluators/fractional_evaluation.md

Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

…l_evaluation.md Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

core/pkg/eval/fractional_evaluation.go

Kavindu-Dodan

Looks good (left a suggestion for improvement)

And I don't see this as a breaking change, so we don't have to mark the PR as a breaking change 🤔 This introduce a behavior change but that's from an invalid fractional evaluation to targetingKey as a fallback

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

toddbaert · 2023-08-28T19:54:49Z

@craigpastro I'm really bummed but the changes seem to have broken the nice spread on your friends-character emails 😩 😩 😩

toddbaert · 2023-08-28T19:56:03Z

Looks good (left a suggestion for improvement)

And I don't see this as a breaking change, so we don't have to mark the PR as a breaking change 🤔 This introduce a behavior change but that's from an invalid fractional evaluation to targetingKey as a fallback

Ya, using a "hard-coded" string also seems to still work. So I agree with @Kavindu-Dodan unless I'm missing something.

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

craigpastro · 2023-08-28T19:59:58Z

@craigpastro I'm really bummed but the changes seem to have broken the nice spread on your friends-character emails 😩 😩 😩

LOL. No, we got it back by replacing Pheobe with Joey (and RossG with just Ross)... perhaps some deeper meaning here.

craigpastro · 2023-08-28T20:01:20Z

Looks good (left a suggestion for improvement)
And I don't see this as a breaking change, so we don't have to mark the PR as a breaking change 🤔 This introduce a behavior change but that's from an invalid fractional evaluation to targetingKey as a fallback

Ya, using a "hard-coded" string also seems to still work. So I agree with @Kavindu-Dodan unless I'm missing something.

Oh, yes, this is true. It will no longer extract the value of that hardcoded string from the context though.

craigpastro · 2023-08-28T20:02:10Z

Thanks again for your help @craigpastro

It's my pleasure! Will definitely look for other opportunities to contribute in the future.

toddbaert · 2023-08-28T20:08:31Z

I'm going to merge this, but cc'ing @bacherfl . Hoping to put this out in a release soon now that #843 seems complete. Let me know if you have concerns.

🤖 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>

Use nested props in fractional evaluator

8d29989

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

Craig Pastro added 2 commits August 28, 2023 10:45

Update docs slightly

77589bf

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

Handle missing bucket key case

3f97995

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

craigpastro changed the title ~~Use nested props in fractional evaluator~~ feat: use nested props in fractional evaluator Aug 28, 2023

craigpastro marked this pull request as ready for review August 28, 2023 17:54

craigpastro requested a review from a team August 28, 2023 17:54

craigpastro commented Aug 28, 2023

View reviewed changes

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

toddbaert requested review from Kavindu-Dodan and bacherfl August 28, 2023 18:35

doc: update flowchart

8e544f3

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

toddbaert approved these changes Aug 28, 2023

View reviewed changes

Craig Pastro and others added 4 commits August 28, 2023 12:41

Update core/pkg/eval/fractional_evaluation.go

fdbf79a

Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

Update docs/other_resources/in-process-providers/evaluators/fractiona…

0b26fa6

…l_evaluation.md Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

Update docs/other_resources/in-process-providers/evaluators/fractiona…

4e5fe78

…l_evaluation.md Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

Update docs/other_resources/in-process-providers/evaluators/fractiona…

413895f

…l_evaluation.md Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Craig Pastro <pastro.craig@gmail.com>

Kavindu-Dodan reviewed Aug 28, 2023

View reviewed changes

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

Kavindu-Dodan approved these changes Aug 28, 2023

View reviewed changes

feedback and update tests

d11beb8

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

Update test

eafa4f7

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

toddbaert changed the title ~~feat: use nested props in fractional evaluator~~ feat: support nested props in fractional evaluator Aug 28, 2023

toddbaert merged commit 50ff739 into open-feature:main Aug 28, 2023

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

chore: release main #796

Merged

craigpastro deleted the improve-fractional-evaluator branch August 28, 2023 21:52

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

chore: release main #1038

Merged

Conversation

craigpastro commented Aug 28, 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 28, 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!

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

Uh oh!

Codecov Report

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kavindu-Dodan left a comment

Choose a reason for hiding this comment

Uh oh!

toddbaert commented Aug 28, 2023

Uh oh!

toddbaert commented Aug 28, 2023

Uh oh!

craigpastro commented Aug 28, 2023

Uh oh!

craigpastro commented Aug 28, 2023

Uh oh!

craigpastro commented Aug 28, 2023

Uh oh!

toddbaert commented Aug 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

craigpastro commented Aug 28, 2023 •

edited

Loading

netlify bot commented Aug 28, 2023 •

edited

Loading

codecov bot commented Aug 28, 2023 •

edited

Loading

toddbaert left a comment •

edited

Loading