refactor(in-process): flagd-evaluator PoC#94
refactor(in-process): flagd-evaluator PoC#94aepfli wants to merge 5 commits intoopen-feature:mainfrom
Conversation
…icate logic Replaces custom evaluation implementation with direct dependency on flagd-evaluator crate, eliminating ~1000 lines of duplicate code. Changes: - Remove custom operators (fractional, semver, datalogic-rs integration) - Remove custom flag parsing and model conversion logic - Remove custom FlagStore, use evaluator's storage directly - Refactor InProcessResolver and FileResolver to call evaluator functions - Add proper reason/error code mapping to OpenFeature types - Add flag metadata conversion (primitives only) - Set permissive validation mode for spec compliance - Add initial sync validation for both gRPC and file connectors Benefits: - Single source of truth across all providers (Python, Java, Rust) - Zero FFI overhead (direct Rust-to-Rust calls) - Perfect spec alignment with flagd evaluation engine - Simpler codebase (-70% code in in-process module) All 36 tests passing. Refs open-feature/flagd#1842 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary of ChangesHello @aepfli, 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 significantly refactors the in-process evaluation module by integrating the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring. Replacing the custom evaluation logic with the flagd-evaluator crate is a great move that reduces the codebase size, eliminates duplicated logic from other flagd providers, and improves maintainability. The changes are well-contained within the in-process resolver module.
I've identified a few areas for improvement, primarily concerning code duplication between the new FileResolver and InProcessResolver, and a correctness issue in how the evaluation context is constructed. Addressing these will make the new implementation even more robust and maintainable.
…sion Addresses a critical correctness issue where nested structs in evaluation context were converted to debug strings, breaking targeting rules that need to access object fields. Changes: - Return empty JSON objects for EvaluationContextFieldValue::Struct instead of debug strings - This preserves object type expectations in targeting rules - Add detailed comment explaining OpenFeature Rust SDK limitation with Arc<dyn Any> struct storage - Remove unused value_to_json helper functions - Remove unused Mutex imports Note: The OpenFeature Rust SDK stores structs as Arc<dyn Any> which cannot be introspected or serialized. This is a known limitation documented in the SDK source. Until fixed upstream, nested struct fields in evaluation context cannot be accessed by targeting rules, but this change ensures targeting rules don't fail when they encounter struct types. See: https://github.com/open-feature/rust-sdk/blob/main/open-feature/src/evaluation/context_field_value.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Eliminates significant code duplication between InProcessResolver and FileResolver by extracting shared helper functions to a common module. Changes: - Create common.rs with shared functions: - empty_flag(): Helper to create empty FeatureFlag - build_context_json(): Convert EvaluationContext to JSON - context_field_to_json(): Recursive struct handling - map_reason/map_error_code(): OpenFeature type mapping - result_to_details(): Result conversion - json_to_value/json_to_metadata_value(): JSON conversions - get_flag_and_metadata(): Shared storage access helper - Optimize resolve_value() signature: - Change evaluator_fn from Fn(&JsonValue, &Map) to Fn(&Map) - Eliminate unnecessary context map cloning - Single reference passed instead of value + reference - Update both resolvers to use common module: - Remove ~300-400 lines of duplicated code - Consistent implementation across resolvers - Single source of truth for helper logic Benefits: - Easier maintenance (fix bugs once, not twice) - Consistent behavior guaranteed - Performance improvement (no context map clone) - Cleaner, more focused resolver code All tests passing (36/36). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds comprehensive documentation about the thread-local nature of the validation mode setting in both InProcessResolver and FileResolver. The issue: - flagd_evaluator::storage::set_validation_mode() sets thread-local state - If multiple resolver instances are created in the same thread with different validation requirements, they will share the same mode - This is a design limitation of the evaluator's current API Current status: - Both InProcessResolver and FileResolver use Permissive mode - No practical issues in current usage - Thread-local (not global) means different threads are isolated Future improvements suggested: 1. Make validation mode configurable via FlagdOptions/CacheSettings 2. Pass validation mode directly to evaluator functions 3. Support per-instance configuration in flagd-evaluator This documents a known limitation without breaking changes, allowing users to understand the behavior and potential future improvements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…esolvers Replace global thread-local state with per-instance FlagEvaluator for both InProcessResolver and FileResolver, eliminating global state issues and enabling multiple independent resolver instances with different configurations. - Update InProcessResolver to use Arc<tokio::sync::RwLock<FlagEvaluator>> - Update FileResolver to use Arc<tokio::sync::RwLock<FlagEvaluator>> - Each resolver instance creates its own FlagEvaluator with Permissive mode - Remove unused get_flag_and_metadata() and empty_flag() from common.rs - Use async RwLock for Send-safe access across await points - Remove calls to thread-local update_flag_state_with_mode() - Simplify evaluation by using evaluator instance methods directly This fixes issues where multiple resolver instances could interfere with each other via global state, and aligns the Rust implementation with Python's instance-based architecture. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1d5f4f8 to
5443f89
Compare
…icate logic
Replaces custom evaluation implementation with direct dependency on flagd-evaluator crate, eliminating ~1000 lines of duplicate code.
Changes:
Benefits:
All 36 tests passing.
Refs open-feature/flagd#1842
🤖 Generated with Claude Code
This PR
Related Issues
Fixes #1234523
Notes
Follow-up Tasks
How to test