-
Notifications
You must be signed in to change notification settings - Fork 398
[FFL-1273] bindings for ffe in openfeature provider #5007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FFL-1273] bindings for ffe in openfeature provider #5007
Conversation
|
👋 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-07 01:16:03 UTC |
a9fce8c to
5b61f55
Compare
setup_ffe.sh
Outdated
| # profiling.h - minimal stub | ||
| cat > my-libdatadog-build/arm64-darwin-24/include/datadog/profiling.h << 'EOF' | ||
| #ifndef DDOG_PROFILING_H | ||
| #define DDOG_PROFILING_H | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <stdbool.h> | ||
| #include <stddef.h> | ||
| #include <stdint.h> | ||
| #include "common.h" | ||
|
|
||
| // Minimal declarations for profiling functionality | ||
| // Ruby bindings don't need full profiling API, just enough to link | ||
|
|
||
| #ifdef __cplusplus | ||
| extern "C" { | ||
| #endif // __cplusplus | ||
|
|
||
| // Stub function declarations - can be extended as needed | ||
|
|
||
| #ifdef __cplusplus | ||
| } // extern "C" | ||
| #endif // __cplusplus | ||
|
|
||
| #endif /* DDOG_PROFILING_H */ | ||
| EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip: You don't actually need to build profiling -- DD_PROFILING_NO_EXTENSION=true skips trying it to build at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporated this into the script though I'm not sure that I'm using it right.
Most of the profiling dependencies are in ext/datadog_profiling_native_extension which I'm understand that env variable lets us skip, but there's still one profiling dependency in ext/libdatadog_api
| #include <datadog/profiling.h> |
So I still ended up needing to generate the profiling.h file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaah oops that's actually an oversight from when datadog_ruby_common.h was extracted out of profiling: this can be replaced with #include <datadog/common.h>. I'll open a PR to fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be deleted after libdatadog is published with datadog-ffe-ffi, at which point we no longer need to build and install it locally
| Check_Type(targeting_key, T_STRING); | ||
| Check_Type(attributes_hash, T_HASH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I recommend using our own ENFORCE_TYPE instead of Check_Type -- they behave similarly, but ENFORCE_TYPE gets you a much better error exception message (you can easily see it working, by e.g. on purpose switching to a wrong type here and seeing the error you get)
lib/datadog/open_feature/binding.rb
Outdated
| unless Binding.supported? | ||
| raise(ArgumentError, "Feature Flags are not supported: #{Datadog::Core::LIBDATADOG_API_FAILURE}") | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this gets repeated a lot, consider perhaps extracting it? (E.g. into a Binding.ensure_supported! or something like that?)
|
I've opened #5018 with some small fixes as I mentioned in the comments. |
075de60 to
81c3973
Compare
BenchmarksBenchmark execution time: 2025-11-07 01:35:59 Comparing candidate commit 81c3973 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 44 metrics, 2 unstable metrics. |
88e5d73 to
da285f5
Compare
| static VALUE configuration_alloc(VALUE klass) { | ||
| ddog_ffe_Handle_Configuration *config = ruby_xcalloc(1, sizeof(ddog_ffe_Handle_Configuration)); | ||
| *config = NULL; // Initialize the handle to NULL | ||
| return TypedData_Wrap_Struct(klass, &configuration_typed_data, config); | ||
| } | ||
|
|
||
| static void configuration_free(void *ptr) { | ||
| ddog_ffe_Handle_Configuration *config = (ddog_ffe_Handle_Configuration *) ptr; | ||
| if (config && *config) { | ||
| ddog_ffe_configuration_drop(config); | ||
| } | ||
| ruby_xfree(ptr); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not entirely sure about this but handle is a pointer and it fits to data field (void *), so I think we can avoid allocations here, call rb_undef_alloc_func(configuration_class), and call TypedData_Wrap_Struct in configuration_initialize.
| static VALUE configuration_alloc(VALUE klass) { | |
| ddog_ffe_Handle_Configuration *config = ruby_xcalloc(1, sizeof(ddog_ffe_Handle_Configuration)); | |
| *config = NULL; // Initialize the handle to NULL | |
| return TypedData_Wrap_Struct(klass, &configuration_typed_data, config); | |
| } | |
| static void configuration_free(void *ptr) { | |
| ddog_ffe_Handle_Configuration *config = (ddog_ffe_Handle_Configuration *) ptr; | |
| if (config && *config) { | |
| ddog_ffe_configuration_drop(config); | |
| } | |
| ruby_xfree(ptr); | |
| } | |
| static void configuration_free(void *ptr) { | |
| ddog_ffe_configuration_drop(&(ddog_ffe_Handle_Configuration) ptr); | |
| } |
Similarly, for all other handle types
| // Check if resolution_details is NULL (no assignment returned) | ||
| if (resolution_details_out == NULL) { | ||
| return Qnil; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ddog_ffe_get_assignment always returns a result, so this check is not necessary
| VALUE resolution_details_obj = resolution_details_alloc(resolution_details_class); | ||
|
|
||
| ddog_ffe_Handle_ResolutionDetails *resolution_details_ptr; | ||
| TypedData_Get_Struct(resolution_details_obj, ddog_ffe_Handle_ResolutionDetails, &resolution_details_typed_data, resolution_details_ptr); | ||
|
|
||
| *resolution_details_ptr = resolution_details_out; | ||
|
|
||
| return resolution_details_obj; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit re using handles directly without extra allocation, this would simplify this section:
| VALUE resolution_details_obj = resolution_details_alloc(resolution_details_class); | |
| ddog_ffe_Handle_ResolutionDetails *resolution_details_ptr; | |
| TypedData_Get_Struct(resolution_details_obj, ddog_ffe_Handle_ResolutionDetails, &resolution_details_typed_data, resolution_details_ptr); | |
| *resolution_details_ptr = resolution_details_out; | |
| return resolution_details_obj; | |
| return TypedData_Wrap_Struct(resolution_details_class, &resolution_details_typed_data, resolution_details_out); |
| rb_define_method(resolution_details_class, "value", resolution_details_get_value, 0); | ||
| rb_define_method(resolution_details_class, "reason", resolution_details_get_reason, 0); | ||
| rb_define_method(resolution_details_class, "error_code", resolution_details_get_error_code, 0); | ||
| rb_define_method(resolution_details_class, "error_message", resolution_details_get_error_message, 0); | ||
| rb_define_method(resolution_details_class, "variant", resolution_details_get_variant, 0); | ||
| rb_define_method(resolution_details_class, "allocation_key", resolution_details_get_allocation_key, 0); | ||
| rb_define_method(resolution_details_class, "do_log", resolution_details_get_do_log, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
major: need to add flag_metadata and extra_logging methods
| # Handle the case where native evaluation returns all-nil results | ||
| # This indicates the native evaluator isn't working properly or flag not found | ||
| if result.value.nil? && result.error_code.nil? && result.reason.nil? | ||
| # All fields are nil - treat as evaluation failure | ||
| ResolutionDetails.new( | ||
| value: default_value, | ||
| variant: nil, | ||
| error_code: :flag_not_found, | ||
| error_message: "Native evaluation returned empty result", | ||
| reason: :error, | ||
| allocation_key: nil, | ||
| do_log: nil | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
major: this is an internal error, not not found case. We should report it as such
| switch (error_code) { | ||
| case DDOG_FFE_ERROR_CODE_TYPE_MISMATCH: | ||
| return ID2SYM(rb_intern("type_mismatch")); | ||
| case DDOG_FFE_ERROR_CODE_PARSE_ERROR: | ||
| return ID2SYM(rb_intern("parse_error")); | ||
| case DDOG_FFE_ERROR_CODE_FLAG_NOT_FOUND: | ||
| return ID2SYM(rb_intern("flag_not_found")); | ||
| case DDOG_FFE_ERROR_CODE_TARGETING_KEY_MISSING: | ||
| return ID2SYM(rb_intern("targeting_key_missing")); | ||
| case DDOG_FFE_ERROR_CODE_INVALID_CONTEXT: | ||
| return ID2SYM(rb_intern("invalid_context")); | ||
| case DDOG_FFE_ERROR_CODE_PROVIDER_NOT_READY: | ||
| return ID2SYM(rb_intern("provider_not_ready")); | ||
| case DDOG_FFE_ERROR_CODE_GENERAL: | ||
| return ID2SYM(rb_intern("general")); | ||
| default: | ||
| return Qnil; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: "general" is a safer default
| switch (error_code) { | |
| case DDOG_FFE_ERROR_CODE_TYPE_MISMATCH: | |
| return ID2SYM(rb_intern("type_mismatch")); | |
| case DDOG_FFE_ERROR_CODE_PARSE_ERROR: | |
| return ID2SYM(rb_intern("parse_error")); | |
| case DDOG_FFE_ERROR_CODE_FLAG_NOT_FOUND: | |
| return ID2SYM(rb_intern("flag_not_found")); | |
| case DDOG_FFE_ERROR_CODE_TARGETING_KEY_MISSING: | |
| return ID2SYM(rb_intern("targeting_key_missing")); | |
| case DDOG_FFE_ERROR_CODE_INVALID_CONTEXT: | |
| return ID2SYM(rb_intern("invalid_context")); | |
| case DDOG_FFE_ERROR_CODE_PROVIDER_NOT_READY: | |
| return ID2SYM(rb_intern("provider_not_ready")); | |
| case DDOG_FFE_ERROR_CODE_GENERAL: | |
| return ID2SYM(rb_intern("general")); | |
| default: | |
| return Qnil; | |
| } | |
| switch (error_code) { | |
| case DDOG_FFE_ERROR_CODE_OK: | |
| return Qnil; | |
| case DDOG_FFE_ERROR_CODE_TYPE_MISMATCH: | |
| return ID2SYM(rb_intern("type_mismatch")); | |
| case DDOG_FFE_ERROR_CODE_PARSE_ERROR: | |
| return ID2SYM(rb_intern("parse_error")); | |
| case DDOG_FFE_ERROR_CODE_FLAG_NOT_FOUND: | |
| return ID2SYM(rb_intern("flag_not_found")); | |
| case DDOG_FFE_ERROR_CODE_TARGETING_KEY_MISSING: | |
| return ID2SYM(rb_intern("targeting_key_missing")); | |
| case DDOG_FFE_ERROR_CODE_INVALID_CONTEXT: | |
| return ID2SYM(rb_intern("invalid_context")); | |
| case DDOG_FFE_ERROR_CODE_PROVIDER_NOT_READY: | |
| return ID2SYM(rb_intern("provider_not_ready")); | |
| case DDOG_FFE_ERROR_CODE_GENERAL: | |
| return ID2SYM(rb_intern("general")); | |
| default: | |
| return ID2SYM(rb_intern("general")); | |
| } |
| elsif result.error_code || result.value.nil? | ||
| # Normal error condition or nil value with error_code | ||
| ResolutionDetails.new( | ||
| value: default_value, | ||
| variant: result.variant, | ||
| error_code: result.error_code || :flag_not_found, | ||
| error_message: result.error_message || "Flag evaluation failed", | ||
| reason: result.reason || :error, | ||
| allocation_key: result.allocation_key, | ||
| do_log: result.do_log | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
major 🐛: this incorrectly squashes non-error evaluation that resulted in no value (e.g., flag disabled or default allocation null, or null json flags) into error.
| elsif result.error_code || result.value.nil? | |
| # Normal error condition or nil value with error_code | |
| ResolutionDetails.new( | |
| value: default_value, | |
| variant: result.variant, | |
| error_code: result.error_code || :flag_not_found, | |
| error_message: result.error_message || "Flag evaluation failed", | |
| reason: result.reason || :error, | |
| allocation_key: result.allocation_key, | |
| do_log: result.do_log | |
| ) | |
| elsif result.variant.nil? | |
| # Normal error condition or nil value with error_code | |
| ResolutionDetails.new( | |
| value: default_value, | |
| variant: result.variant, | |
| error_code: result.error_code, | |
| error_message: result.error_message, | |
| reason: result.reason, | |
| allocation_key: result.allocation_key, | |
| do_log: result.do_log | |
| ) |
| # First try to handle evaluation using Ruby-based logic | ||
| ruby_result = try_ruby_evaluation(flag_key, evaluation_context, default_value) | ||
| return ruby_result if ruby_result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
major: native evaluation should be tried first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah this is just some mess from moving code around while I debug. The evaluation code in Ruby shouldn't be used at all (and probably should be deleted) after we get the binding working
6f4c1fd to
94c24c2
Compare
- Recovered setup_ffe.sh script with libdatadog build automation - Recovered C extension feature_flags.c with function-based FFI API - Recovered comprehensive binding tests (saved as binding_spec_recovered.rb) - These files were accidentally lost when rebasing onto target branch that used different FFE approach
- Recover setup_ffe.sh, feature_flags.c, and tests lost during rebase - Add NativeEvaluator class using native C extension methods - Update Configuration/EvaluationContext to support native and Ruby modes - Add Binding.supported? method and improved setup script cleanup - Fix library_config.h API compatibility (ddog_CStr -> ddog_CharSlice)
- Add NativeEvaluator class that uses libdatadog FFI for flag evaluation - Integrate C extension (feature_flags.c) with OpenFeature::Binding module - Add automatic libdatadog_api extension loading in binding.rb - Implement ResolutionDetails class backed by native C structures - Add comprehensive native_evaluator_spec.rb test suite (14 tests) - Update Configuration class to support native mode initialization - Fix EvaluationContext to properly detect native method availability - Update setup_ffe.sh to test native evaluator end-to-end - Add null handle validation in native C functions - Temporarily disable incompatible library_config API calls
… fallback - Add native_evaluator_test_cases_spec.rb with 212 comprehensive fixture tests matching InternalEvaluator coverage - Implement hybrid Ruby-first evaluation with native fallback in NativeEvaluator for improved reliability - Add Ruby-based evaluation for disabled flags and simple allocation scenarios - Update setup_ffe.sh to run comprehensive fixture tests as part of FFE verification - Fix InternalEvaluator fixture tests to use flat ResolutionDetails structure instead of flag_metadata - Update test validation to check allocation_key and do_log directly on ResolutionDetails - Enhance NativeEvaluator to handle both 2-parameter and 4-parameter get_assignment signatures - Add comprehensive error handling and debug logging for native evaluation troubleshooting
3812d17 to
11cabd5
Compare
11cabd5
into
FFL-1361-Evaluation-in-binding-in-ruby
|
Accidental merge. Re-drafting here: #5034 |
What does this PR do?
Motivation:
Change log entry
Additional Notes:
How to test the change?