Skip to content

Conversation

@sameerank
Copy link
Contributor

@sameerank sameerank commented Nov 5, 2025

What does this PR do?

Implements an feature flag evaluator in Ruby that aligns as closely as possible with with the libdatadog FFE (Feature Flag Evaluation) interface contract, according to what's in DataDog/libdatadog#1282. This is one layer of stacked PRs:

main
^- #4998 (Add OpenFeature component)
^- #5024 (Add feature flags events exposure)
^- #5022 (Add InternalEvaluator) <-- you are here!

Motivation:

Eventually, we want to use a C binding with datadog-ffe-ffi for flag evaluations. In this PR, I aimed to implement an evaluator in Ruby that can be easily swapped out to use the binding when it is ready.

Change log entry

Additional Notes:

Ran tests with

docker run --rm -v $PWD:/app -w /app ruby:3.3 bash -c "
export BUNDLE_GEMFILE=gemfiles/ruby_3.3_openfeature_latest.gemfile &&
bundle install &&
bundle exec rake spec:open_feature
"

And I see one failure that is not from the internal evaluator

rspec ./spec/datadog/open_feature/component_spec.rb:42 # Datadog::OpenFeature::Component.build when open_feature is enabled when remote configuration is disabled logs warning and returns nil

How to test the change?

@pr-commenter
Copy link

pr-commenter bot commented Nov 5, 2025

Benchmarks

Benchmark execution time: 2025-11-14 03:33:42

Comparing candidate commit 2bae8a0 in PR branch FFL-1361-Evaluation-in-binding-in-ruby with baseline commit 49cee89 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 44 metrics, 2 unstable metrics.

@sameerank sameerank force-pushed the FFL-1361-Evaluation-in-binding-in-ruby branch 2 times, most recently from da05c91 to 1857c1f Compare November 6, 2025 16:20
@github-actions github-actions bot added core Involves Datadog core libraries appsec Application Security monitoring product labels Nov 6, 2025
@sameerank sameerank changed the base branch from add-openfeature-component to ffl-1319-add-agent-communication-for-openfeature November 6, 2025 16:21
@github-actions
Copy link

github-actions bot commented Nov 6, 2025

👋 Hey @DataDog/ruby-guild, please fill "Change log entry" section in the pull request description.

If changes need to be present in CHANGELOG.md you can state it this way

**Change log entry**

Yes. A brief summary to be placed into the CHANGELOG.md

(possible answers Yes/Yep/Yeah)

Or you can opt out like that

**Change log entry**

None.

(possible answers No/Nope/None)

Visited at: 2025-11-14 03:04:18 UTC

require_relative 'binding/configuration'

# Define alias for backward compatibility after InternalEvaluator is loaded
Datadog::OpenFeature::Binding::Evaluator = Datadog::OpenFeature::Binding::InternalEvaluator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aspiration is that in #5007 when it is ready, we can just switch this to

Datadog::OpenFeature::Binding::Evaluator = Datadog::OpenFeature::Binding::NativeEvaluator

And then we're using the libdatadog FFE powered evaluator

@sameerank sameerank marked this pull request as ready for review November 7, 2025 09:14
@sameerank sameerank requested a review from a team as a code owner November 7, 2025 09:14
@Strech Strech force-pushed the ffl-1319-add-agent-communication-for-openfeature branch from 0a25c37 to d5bbcd5 Compare November 7, 2025 09:27
@Strech Strech requested a review from a team as a code owner November 7, 2025 09:27
@Strech Strech force-pushed the ffl-1319-add-agent-communication-for-openfeature branch from e7a0762 to 47383e2 Compare November 7, 2025 14:22
@sameerank sameerank force-pushed the FFL-1361-Evaluation-in-binding-in-ruby branch from 6f4c1fd to 94c24c2 Compare November 10, 2025 02:42
@sameerank sameerank requested a review from a team as a code owner November 10, 2025 03:38
@sameerank sameerank marked this pull request as draft November 10, 2025 03:39
@sameerank sameerank force-pushed the FFL-1361-Evaluation-in-binding-in-ruby branch from 4e749c5 to bf1fbb4 Compare November 10, 2025 04:11
@sameerank sameerank marked this pull request as ready for review November 10, 2025 07:16
@Strech Strech force-pushed the ffl-1319-add-agent-communication-for-openfeature branch from b1a978f to 9e7efb0 Compare November 10, 2025 10:02
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been summoned by you asking a review of profiling-rb... although I'm not sure why? Were you looking for an extra review?
I've given it a pass, but I stopped short at some of the more deeper domain classes.

Ok so this is a lot code!. The PR description states

Eventually, we want to use a C binding with datadog-ffe-ffi for flag evaluations. In this PR, I aimed to implement an evaluator in Ruby that can be easily swapped out to use the binding when it is ready.

and then you reply at some point with

Much of this is temporary. It will be replaced by a C binding to an evaluator in libdatadog and then the Ruby evaluation code can be deleted.

So this again gets me thinking: What's the plan here again? Are we actually planning on ever shipping the pure-Ruby version to customers? Or is this just for internal testing?

Because, again, this being a lot of code, the answers to those questions are very important in determining how much time we should spend on making sure this PR is solid. Do my doubts here make sense?

"DD_ENV" => {version: ["A"]},
"DD_ERROR_TRACKING_HANDLED_ERRORS" => {version: ["A"]},
"DD_ERROR_TRACKING_HANDLED_ERRORS_INCLUDE" => {version: ["A"]},
"DD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLED" => {version: ["A"]},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually the EXPERIMENTAL part comes after the feature name -- consider maybe DD_FLAGGING_PROVIDER_EXPERIMENTAL_ENABLED or something like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't pick the name, it's in align with other libs 😥

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're still on time to change that, may be worth it; if not, well, it's experimental anyway 😭

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of copy-paste to reuse the current transport code... I know there's already a lot going on in this PR, but I would gently suggest considering just writing a new transport from scratch rather than copying multiple really confusingly-written files.

Copy link
Member

@Strech Strech Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋🏼 I'm who wrote copy/paste transport. To be honest I wasn't feeling comfortable to go with self-written due to the limitations of my time on this project. At the same time I don't see much a different approach in most components, all of them use Core to write the transport, as did I.

I don't mind a better transport if I could, but that's not the case. Do you think it's a blocker?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(We've chatted privately in more detail, but just recording it here that the answer is not at all ;D)

@Strech
Copy link
Member

Strech commented Nov 10, 2025

@ivoanjo @y9v I had a plan to split the work to component and then transport and binding as a separate PRs, but by the accident you got pinged with a full blast PR.

I would apply certain suggestions to my dummy binding PR and open it for review, in addition I will extract certain pieces already suggested.

Sorry that you get though this and thanks for lots of suggestions! ❤️

@Strech Strech force-pushed the ffl-1319-add-agent-communication-for-openfeature branch from d7424f9 to 2221c34 Compare November 10, 2025 15:05
@sameerank
Copy link
Contributor Author

Thanks for the reviews @ivoanjo @y9v ! I didn't know opening a PR onto another stacked branch to consolidate our work would trigger a wider request for reviews. I will convert this PR back to draft so I can rebase this work on the base branch again

@sameerank sameerank marked this pull request as draft November 10, 2025 17:25
* Fix configuration_spec.rb path issues and method calls
  - Correct fixture path from ../../../../ to ../../../
  - Replace undefined from_json with from_hash method
  - Replace be_in matcher with include matcher for RSpec compatibility

* Fix allocation matching test method signatures and expectations
  - Remove extra parameter from get_assignment calls (4 params → 3 params)
  - Update error expectations from :Ok to nil for successful evaluations
  - Fix flag_metadata access to use hash keys instead of method calls
  - Update test descriptions to reflect internal evaluator behavior

* Fix flag lookup logic in Configuration#get_flag
  - Search by flag.key property instead of hash key for proper UFC compatibility
  - Enables correct flag resolution where hash key != flag.key

* Update assignment reason expectations in provider tests
  - Change TARGETING_MATCH to STATIC for simple allocations without rules
  - Aligns with libdatadog assignment reason logic (no rules + single empty shard = STATIC)

All binding tests now pass (54 examples, 0 failures).
Remaining provider/engine integration issues need separate investigation.
* Fix OpenFeature.engine returning nil by enabling remote configuration
  - Component requires both open_feature.enabled and remote.enabled
  - Update test to configure both settings for proper initialization

* Fix provider test expected values to match flag configurations
  - Update integer expectation: 42 → 21 to match actual config value
  - Update number expectation: 9000 → 1000 to match actual config value
  - Update float expectation: 36.6 → 12.5 to match actual config value
  - Update object expectation: [1,2,3] → {"key":"value"} to match actual config

* Fix variation type mismatches in provider test configurations
  - Change "variationType": "NUMBER" → "NUMERIC" for UFC compliance
  - Change "variationType": "FLOAT" → "NUMERIC" for UFC compliance
  - Change "variationType": "OBJECT" → "JSON" for UFC compliance
  - Aligns with VariationType constants and internal evaluator type mapping

All OpenFeature tests now pass: 128 examples, 0 failures, 1 pending.
Complete end-to-end integration working from provider → engine → internal evaluator.
- Remove duplicate :agent_info in components.rb
- Fix OpenFeature capability check to match other components pattern
- Revert evaluation_engine.rb to target branch version (not part of internal evaluator functionality)
- Revert evaluation_engine.rbs to target branch version
- Revert component_spec.rb to target branch version
- Revert resolution_details.rbs to target branch version (since we're not touching resolution_details.rb)
The InternalEvaluator was expecting flags at the root level but tests were using the nested Universal Flag Configuration (UFC) format with data.attributes.flags structure. This caused evaluation_engine_spec.rb and other tests to fail with nil values instead of expected flag results.

Changes:
- Update InternalEvaluator to extract flags from data.attributes.flags
- Standardize all test specs to use consistent nested UFC format:
  - evaluation_engine_spec.rb (already used nested format)
  - internal_evaluator_spec.rb (updated to use full JSON structure)
  - rule_evaluation_spec.rb (converted from flat to nested format)
  - allocation_matching_spec.rb (converted from flat to nested format)

This resolves 8 test failures and maintains consistency across the OpenFeature implementation by using the standard UFC JSON structure throughout the codebase.
The OpenFeature internal evaluator was expecting nested JSON structure (data.attributes.flags) but this has been simplified to use flags directly at the root level for consistency and clarity.

Changes:
- Update InternalEvaluator parser to expect flags at root level instead of data.attributes.flags
- Simplify flags-v1.json fixture to use flat {"flags": {...}} structure
- Update all test specs to use simplified JSON format and consistent flag_config naming:
  - evaluation_engine_spec.rb: changed from nested ufc to flat flag_config
  - binding specs: standardized on flag_config variable naming
  - configuration_spec.rb: renamed ufc_attributes to flag_config
- Update all test case fixtures to match simplified format
- Update all test files to use flat JSON flag configuration format instead of nested data.attributes structure
- Standardize variable naming from 'ufc' to 'flag_config' across all test files for consistency
- Convert flag_metadata attributes to camelCase: allocationKey, variationType, doLog
- Update documentation to reference system-tests origin and remove UFC terminology
- Maintain backward compatibility with internal evaluator while simplifying test structure

This consolidation aligns the OpenFeature implementation with the simplified flag configuration format and ensures consistent metadata attribute naming across the codebase.
- Rename result creation methods to clearly describe the three evaluation cases
- Fix FlagDisabled and DefaultAllocationNull to return success results with nil values
- Update test logic to properly handle three distinct result types
- Convert flag_metadata attributes to camelCase: allocationKey, variationType, doLog
- Set error_message=nil for successful evaluations (was empty string)
- Update test expectations to match new result structure
…alidation

- Remove ERROR_CODE_MAPPING constant and return string error codes directly to match ResolutionDetails type signature
- Replace 19 common UFC test cases with libdatadog versions containing detailed resolution metadata (variant, allocationKey, variationType, doLog)
- Enrich 3 Ruby-specific test cases with expected resolution metadata
- Update test assertions to validate exact metadata values from test cases instead of just structural validation
- Update type signatures in .rbs file to reflect removed constants
- Fix attribute lookup bug where boolean false values were treated as missing due to || operator treating falsy values as nil
- Remove unnecessary symbol key fallback in get_attribute_from_context and get_targeting_key since OpenFeature SDK guarantees string keys via transform_keys(&:to_s)
- Update test expectations from symbol error codes (:flag_not_found) to string error codes ('FLAG_UNRECOGNIZED_OR_DISABLED') for consistency
- Remove outdated "UFC" terminology references
- Remove unnecessary "Rust" implementation references
- Remove vague TODO comments for structured logging
- Add error? method to check if result has an error (error_code present)
- Add result? method to check if result has successful variant
- Add log? method as Ruby-friendly alternative to do_log
- Add do_log? alias for backward compatibility
- Update type signatures for new methods
- Remove duplicate constants from configuration.rbs
- Add separate RBS files for constant modules (assignment_reason, condition_operator, error_codes, variation_type)
- Recreate internal_evaluator.rbs to match current implementation
- Delete evaluator.rbs (no corresponding Ruby file)
- Update ext.rbs to only include existing constants
- Cache string coercion in condition evaluation to prevent repeated conversions
- Add comprehensive JSON structure validation as single source of truth
- Remove redundant Array/Hash defensive wrappers from configuration classes
- Replace variation type mapping fallback with explicit validation
- Add RBS type signatures for all binding modules
- Reorder evaluation case comments sequentially (1, 2, 3)
- Remove conditional logic from get_attribute_from_context and get_targeting_key
- Update evaluation engine to pass evaluation_context&.fields to internal evaluator
- Add test verifying hash contexts work with OpenFeature SDK fields
- Improves performance by eliminating respond_to? checks in hot paths
Changes the internal evaluator to accept string literals instead of symbols for expected_type parameters, improving consistency with external APIs.

Changes:
- Update InternalEvaluator.type_matches? to handle strings ('boolean', 'string', etc.)
- Add symbol-to-string mapping in EvaluationEngine for backward compatibility
- Update RBS type signatures to reflect string parameters
- Update binding tests to use string expected_type parameters
- Fix private class method access modifiers using private_class_method
- Refactor validation logic to avoid non-local exits from iterators
- Add YARD-style annotation to get_assignment method
- Apply Standard RB formatting fixes to spec files
- Update test expectations to work with default_value behavior
- Ensure all error cases return default_value instead of nil
- Update InternalEvaluator to return main ResolutionDetails class instead of binding-specific class
- Integrate InternalEvaluator into EvaluationEngine's reconfigure! method
- Update tests to use log? attribute instead of do_log for ResolutionDetails
- Fix test mocks to target InternalEvaluator instead of NoopEvaluator
- Maintain backward compatibility and error handling
@sameerank sameerank force-pushed the FFL-1361-Evaluation-in-binding-in-ruby branch from 6b17509 to 8e9e16a Compare November 14, 2025 00:07
@github-actions
Copy link

github-actions bot commented Nov 14, 2025

Typing analysis

Note: Ignored files are excluded from the next sections.

Untyped methods

This PR introduces 36 partially typed methods. It decreases the percentage of typed methods from 54.67% to 54.14% (-0.53%).

Partially typed methods (+36-0)Introduced:
sig/datadog/open_feature/binding/configuration.rbs:19
└── def self.from_hash: (::Hash[::String, untyped] flag_data, ::String key) -> Flag
sig/datadog/open_feature/binding/configuration.rbs:23
└── def self.parse_variations: (::Hash[::String, untyped] variations_data) -> ::Hash[::String, Variation]
sig/datadog/open_feature/binding/configuration.rbs:25
└── def self.parse_allocations: (::Array[untyped] allocations_data) -> ::Array[Allocation]
sig/datadog/open_feature/binding/configuration.rbs:32
└── def initialize: (key: ::String, value: untyped) -> void
sig/datadog/open_feature/binding/configuration.rbs:34
└── def self.from_hash: (::Hash[::String, untyped] variation_data) -> Variation
sig/datadog/open_feature/binding/configuration.rbs:54
└── def self.from_hash: (::Hash[::String, untyped] allocation_data) -> Allocation
sig/datadog/open_feature/binding/configuration.rbs:58
└── def self.parse_rules: (::Array[untyped]? rules_data) -> ::Array[Rule]?
sig/datadog/open_feature/binding/configuration.rbs:60
└── def self.parse_splits: (::Array[untyped] splits_data) -> ::Array[Split]
sig/datadog/open_feature/binding/configuration.rbs:62
└── def self.parse_timestamp: (untyped timestamp_data) -> ::Time?
sig/datadog/open_feature/binding/configuration.rbs:76
└── def self.from_hash: (::Hash[::String, untyped] split_data) -> Split
sig/datadog/open_feature/binding/configuration.rbs:80
└── def self.parse_shards: (::Array[untyped] shards_data) -> ::Array[Shard]
sig/datadog/open_feature/binding/configuration.rbs:94
└── def self.from_hash: (::Hash[::String, untyped] shard_data) -> Shard
sig/datadog/open_feature/binding/configuration.rbs:98
└── def self.parse_ranges: (::Array[untyped] ranges_data) -> ::Array[ShardRange]
sig/datadog/open_feature/binding/configuration.rbs:107
└── def self.from_hash: (::Hash[::String, untyped] range_data) -> ShardRange
sig/datadog/open_feature/binding/configuration.rbs:117
└── def self.from_hash: (::Hash[::String, untyped] rule_data) -> Rule
sig/datadog/open_feature/binding/configuration.rbs:121
└── def self.parse_conditions: (::Array[untyped] conditions_data) -> ::Array[Condition]
sig/datadog/open_feature/binding/configuration.rbs:129
└── def initialize: (attribute: ::String, operator: ::String, value: untyped) -> void
sig/datadog/open_feature/binding/configuration.rbs:131
└── def self.from_hash: (::Hash[::String, untyped] condition_data) -> Condition
sig/datadog/open_feature/binding/configuration.rbs:141
└── def self.from_hash: (::Hash[::String, untyped] config_data) -> Configuration
sig/datadog/open_feature/binding/internal_evaluator.rbs:26
└── def validate_flag_structure: (::Hash[::String, untyped] flag_data) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:28
└── def validate_allocation_structure: (::Hash[::String, untyped] allocation_data) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:30
└── def validate_split_structure: (::Hash[::String, untyped] split_data) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:32
└── def validate_shard_structure: (::Hash[::String, untyped] shard_data) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:34
└── def validate_rule_structure: (::Hash[::String, untyped] rule_data) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:36
└── def membership_matches?: (::String? attr_str, (::Array[untyped] | untyped) condition_values, bool expected_membership) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:54
└── def evaluate_rule: (Rule rule, ::Hash[::String, untyped]? evaluation_context) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:56
└── def evaluate_condition: (Condition condition, ::Hash[::String, untyped]? evaluation_context) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:58
└── def get_attribute_from_context: (::String attribute_name, ::Hash[::String, untyped]? evaluation_context) -> untyped
sig/datadog/open_feature/binding/internal_evaluator.rbs:60
└── def evaluate_comparison: (untyped attribute_value, untyped condition_value, ::Symbol operator) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:62
└── def evaluate_membership: (untyped attribute_value, untyped condition_values, bool expected_membership) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:64
└── def evaluate_regex: (untyped attribute_value, untyped pattern, bool expected_match) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:66
└── def evaluate_null_check: (untyped attribute_value, untyped expected_null) -> bool
sig/datadog/open_feature/binding/internal_evaluator.rbs:68
└── def coerce_to_number: (untyped value) -> ::Float?
sig/datadog/open_feature/binding/internal_evaluator.rbs:70
└── def coerce_to_string: (untyped value) -> ::String?
sig/datadog/open_feature/binding/internal_evaluator.rbs:72
└── def coerce_to_boolean: (untyped value) -> bool?
sig/datadog/open_feature/binding/internal_evaluator.rbs:74
└── def get_targeting_key: (::Hash[::String, untyped]? evaluation_context) -> ::String?

Untyped other declarations

This PR introduces 2 untyped other declarations and 1 partially typed other declaration. It increases the percentage of typed other declarations from 68.16% to 69.21% (+1.05%).

Untyped other declarations (+2-0)Introduced:
sig/datadog/open_feature/binding/configuration.rbs:30
└── attr_reader value: untyped
sig/datadog/open_feature/binding/configuration.rbs:127
└── attr_reader value: untyped
Partially typed other declarations (+1-0)Introduced:
sig/datadog/open_feature/binding/configuration.rbs:68
└── attr_reader extra_logging: ::Hash[::String, untyped]

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept to the end of the line to remove it from the stats.

@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Nov 14, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage
Patch Coverage: 93.91%
Total Coverage: 98.46% (+0.11%)

View detailed report

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 2bae8a0 | Docs | Datadog PR Page | Was this helpful? Give us feedback!

- Replace binding-specific ResolutionDetails with main ResolutionDetails class
- Integrate InternalEvaluator into EvaluationEngine's reconfigure! method
- Update tests to use log? attribute instead of do_log for ResolutionDetails
- Fix test mocks to target InternalEvaluator instead of NoopEvaluator
- Remove duplicate binding/resolution_details.rb and .rbs files
- Clean up excessive comments in binding files to match project style
- Preserve essential case delineation comments (Case 1/2/3 evaluation model)
- Maintain backward compatibility and defensive error handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Involves Datadog core libraries openfeature A new component that provider an ability to configure feature flags

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants