-
Notifications
You must be signed in to change notification settings - Fork 15
[FFL-1284] Create datadog-ffe-ffi crate #1282
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
base: main
Are you sure you want to change the base?
Conversation
BenchmarksComparisonBenchmark execution time: 2025-11-07 17:57:42 Comparing candidate commit e290eb7 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 55 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
Group 14
Group 15
Group 16
Group 17
BaselineOmitted due to size. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1282 +/- ##
==========================================
- Coverage 71.66% 71.31% -0.36%
==========================================
Files 370 374 +4
Lines 58581 58895 +314
==========================================
+ Hits 41984 42001 +17
- Misses 16597 16894 +297
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
Creates C-compatible FFI layer for Feature Flagging & Experimentation: - Add datadog-ffe-ffi crate following ddsketch-ffi patterns - Implement Handle<T> wrappers for Configuration, EvaluationContext, Assignment - Export get_assignment, configuration_new, evaluation_context_new functions - Add memory management with drop functions - Generate C headers via cbindgen for Ruby integration - Update Cargo.toml to include new FFI crate in workspace
- Remove trailing whitespace from doc comments - Reformat function call in assignment.rs to fit line length constraints
553e2ac to
dfc60e3
Compare
- Updated build.rs to use a variable for the header name and improved readability. - Modified cbindgen.toml to reorganize export settings and enforce naming conventions for types and functions. - Added Clippy lints in lib.rs to enhance code quality and prevent common pitfalls.
Pin datadog-ffe to exact version Co-authored-by: Oleksii Shmalko <[email protected]>
refactor(ffe): simplify error handling Merge branch 'sameerank/FFL-1284-Create-datadog-ffe-ffi-crate' into ffe-simplify-error-handling Co-authored-by: sameerank <[email protected]>
The distinction served us well in Eppo but it's less useful as we're migrating to OpenFeature and it handles these various errors rather similarly.
(I think) the way serde_json implements untagged enum parsing is by first parsing into serde_json::Value and then parsing that to one of the variants. This breaks RawValue deserialization because Value does not hold onto original string. This PR makes TryParse work by quickly scanning into RawValue first, and then trying to parse it into target type. If that parsing fails, the RawValue is copied into Box.
ac5f6bd to
080057f
Compare
| VariantValue::Object(unsafe { BorrowedStr::borrow_from_str(raw.get()) }) | ||
| } | ||
| }, | ||
| _ => VariantValue::None, |
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.
One difference I'm observing
- FFI: Error cases return "empty" values (
VariantValue::None) - Ruby: Error cases return the provided
default_value
Since there's no default value argument for this method, this means that when the binding is implemented in Ruby, the Ruby code should handle returning a fallback default if the result is an empty type
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.
Correct
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.
I'm still working on getting all the test-case-*.json tests to pass in DataDog/dd-trace-rb#5007, and I suspect part of the challenge might be ambiguity around interpreting what this None result means
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.
None means that user-provided default should be used
| VariationType::Integer => FlagType::Integer, | ||
| VariationType::Numeric => FlagType::Float, |
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.
I'm trying to find where the variation types get converted to the types in the test case files
variationType --> "string", "number", "boolean", "object"
Without that conversion the computed variationType will differ from the precomputed response. I see this section
libdatadog/datadog-ffe/src/rules_based/flag_type.rs
Lines 4 to 17 in 0a63cce
| #[serde(rename_all = "snake_case")] | |
| #[repr(u8)] | |
| pub enum FlagType { | |
| #[serde(alias = "BOOLEAN")] | |
| Boolean = 1, | |
| #[serde(alias = "STRING")] | |
| String = 1 << 1, | |
| #[serde(alias = "NUMERIC")] | |
| Float = 1 << 2, | |
| #[serde(alias = "INTEGER")] | |
| Integer = 1 << 3, | |
| #[serde(alias = "JSON")] | |
| Object = 1 << 4, | |
| } |
But rename_all = "snake_case" wouldn't know to change "INTEGER" to "number"
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.
FlagType's canonical representation matches our expected precomputed API: "boolean" | "string" | "float" | "integer" | "object".
"number" is a fake flag type and is only returned from precomputed API temporarily as part of incident response. Once all clients upgrade to properly handle "integer" | "float", that's what we're going to return.
That's irrelevant to our test files though as these still use legacy names: "BOOLEAN" | "STRING" | "NUMERIC" | "INTEGER" | "JSON". This is where aliases here come into play and allow us to parse these strings in test cases as FlagType.
| pub struct ResolutionDetails { | ||
| inner: Result<Assignment, EvaluationError>, | ||
| // memoizing some fields, so we can hand off references to them: | ||
| error_message: Option<String>, | ||
| extra_logging: Vec<KeyValue<BorrowedStr, BorrowedStr>>, | ||
| flag_metadata: Vec<KeyValue<BorrowedStr, BorrowedStr>>, | ||
| } |
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.
What is the format of the output that I should be expecting?
What I currently have in DataDog/dd-trace-rb#5007 using the FFI in #1282 for the binding looks like
context = Datadog::OpenFeature::Binding::EvaluationContext.new('test_user')
native = Datadog::OpenFeature::Binding::NativeEvaluator.new(libdatadog_config)
result = native.get_assignment('test_flag', context)
# Output:
result.class # => Datadog::OpenFeature::Binding::ResolutionDetails
result.value # => "control_value"
result.variant # => "control"
result.error_code # => nil
result.error_message # => nil
result.reason # => :static
result.allocation_key # => "rollout"
result.do_log # => falseIs that right? Given that there isn't an exact 1:1 match with https://github.com/open-feature/ruby-sdk/blob/main/lib/open_feature/sdk/provider/resolution_details.rb I wonder if we should rename our internal ResolutionDetails to something else to avoid confusion
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.
ddog_ffe_ResolutionDetails is an opaque C type, it doesn't have any publicly accessible fields. The C side can interact with it via function calls only.
With the exception of value which is allowed to be missing, the type is the superset of OF's resolution details.
The Ruby extension may still rename the type and rearrange the fields as it sees fit
0a63cce to
b032372
Compare
acd42d4 to
295f929
Compare
d5d0cf1 to
4cba0bf
Compare
4cba0bf to
d04a06c
Compare
d04a06c to
e290eb7
Compare
|
Thanks for clarifying above. The PR looks good from my perspective! I would click approve if I could, but I opened the PR so I don't have that permission ... |
What does this PR do?
Adds a
datadog-ffe-fficrate, which provides a C FFI (Foreign Function Interface) bindings for thedatadog-ffe(Feature Flag Evaluation) library. This is to allowdd-trace-rbto interact with the feature flag evaluation engine through a C-compatible API.The crate exposes the following key components:
Motivation
The motivation for FFL-1284 is to enable non-Rust languages (PHP, Ruby, Python, etc.) to use the feature flag evaluator in
libdatadog, for consistent feature flag evaluation in SDKs across languages.Additional Notes
datadog-profiling-ffi,datadog-crashtracker-ffi)cbindgenfor automatic C generationstaticlib,cdylib)How to test the change?
cargo build -p datadog-ffe-fficargo build -p datadog-ffe-ffi --features cbindgen(Check that headers are generated intarget/include/)[FFL-1273] bindings for libdatadog datadog ffe api dd-trace-rb#5001[FFL-1273] bindings for ffe in openfeature provider dd-trace-rb#5007