feat!: fractional bucketing improvements#373
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the fractional evaluation mechanism within the flagd provider. It focuses on improving the precision and reliability of fractional flag assignments by modernizing the underlying hashing and bucketing algorithms. The changes aim to enhance the accuracy and robustness of fractional flag evaluations, resolving a previously identified issue. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors and improves the fractional evaluation logic to support high-resolution bucketing. The changes include extracting logic into a helper function, using unsigned 32-bit hashes for better distribution, and adopting a more precise integer-based calculation for bucketing. The test configurations are also updated to reflect these changes. My review identifies a minor opportunity for improving maintainability by replacing a magic number with a named constant.
...eature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/custom_ops.py
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #373 +/- ##
==========================================
- Coverage 98.72% 95.91% -2.82%
==========================================
Files 3 30 +27
Lines 157 1517 +1360
==========================================
+ Hits 155 1455 +1300
- Misses 2 62 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There's still 1 failing test, but I'm not sure how to fix it because I don't know what exactly it should be testing in the first place. It's the |
| "~metadata", | ||
| "~deprecated", | ||
| "~fractional-v1", | ||
| "~fractional-v2", |
There was a problem hiding this comment.
[q] shouldnt we also support this via rpc mode?
There was a problem hiding this comment.
yes, we will, however it's not yet released in flagd, but @toddbaert is working on it. i will remove the fractional-v2 and fractional-nested exclusions once flagd is ready.
3beeac1 to
e3163ad
Compare
|
@leakonvalinka I fixed some conflicts, squashed and rebased on main. Only changes I made from yours was minor lockfile stuff, a small test update, and updating to the latest e2e suite. |
toddbaert
left a comment
There was a problem hiding this comment.
Implementation is great. I will coordinate the release/merge.
406114a to
c2a03d9
Compare
- support nested properties in fractional targeting rules - support non-string variant keys in fractional operator - bump grpcio and grpcio-health-checking to >=1.80.0 - update test harness and add comprehensive fractional tests - adjust test assertions for invalid-fractional-weights behavior change Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
6844506 to
6d76956
Compare
## High-precision and nested fractional This is a coordinated feature across flagd and all flagd providers. - `fractional` now supports up to max-int32 total weight and 1/max-int32 resolution as described in the [high-precision fractional bucketing ADR](https://flagd.dev/architecture-decisions/high-precision-fractional-bucketing) - `fractional` now supports computed (nested JSONLogic) variants and weights as described in the [fractional ADR](https://flagd.dev/architecture-decisions/fractional) - Includes e2e tests from the [flagd-testbed v3.1.0...v3.4.0](open-feature/flagd-testbed@v3.1.0...v3.4.0) to validate consistent fractional bucketing across all provider implementations -⚠️ Pseudorandom hash assignments will change with this release due to changes in the hashing algorithm; update flagd and your providers to ensure fractional assignment is consistent across all workloads ### Related PRs | Component | PR | |---|---| | flagd | #1909 | | java-sdk-contrib | open-feature/java-sdk-contrib#1740 | | js-sdk-contrib | open-feature/js-sdk-contrib#1501 | | python-sdk-contrib | open-feature/python-sdk-contrib#373 | | dotnet-sdk-contrib | open-feature/dotnet-sdk-contrib#622 | | flagd-schemas | open-feature/flagd-schemas#207 | > **Warning** > @toddbaert will coordinate the release across flagd and all providers. Do not merge without coordination. > @toddbaert will update the RPC mode tests accordingly once the associated flagd PR is merged and an up-to-date flagd-testbed image is available. --------- Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
High-precision and nested fractional
This is a coordinated feature across flagd and all flagd providers.
fractionalnow supports up to max-int32 total weight and 1/max-int32 resolution as described in the high-precision fractional bucketing ADRfractionalnow supports computed (nested JSONLogic) variants and weights as described in the fractional ADRRelated PRs
Original PR description
Related Issues
#360 and #359
Follow-up Tasks
Remove
fractional-v2andfractional-nestedtags from RPC tests