[ffe] add pyo3 conversion methods #1289
Open
+3,066
−3,011
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dd-oleksii
commented
Oct 30, 2025
Comment on lines
+59
to
+76
| /// Accepts either a dict with `"targeting_key"` and `"attributes"` items, or any object with | ||
| /// `targeting_key` and `attributes` attributes. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ```python | ||
| /// {"targeting_key": "user1", "attributes": {"attr1": 42}} | ||
| /// ``` | ||
| /// | ||
| /// ```python | ||
| /// @dataclass | ||
| /// class EvaluationContext: | ||
| /// targeting_key: Optional[str] | ||
| /// attributes: dict[str, Any] | ||
| /// | ||
| /// EvaluationContext(targeting_key="user1", attributes={"attr1": 42}) | ||
| /// ``` | ||
| impl<'py> FromPyObject<'py> for EvaluationContext { |
There was a problem hiding this comment.
@avara1986 for evaluation context, either a dict or an object with targeting_key/attributes is accepted. (We can technically pass OF's EvaluationContext if we filter out unsupported attributes.)
BenchmarksComparisonBenchmark execution time: 2025-11-01 02:04:20 Comparing candidate commit 36fe6be 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 #1289 +/- ##
==========================================
- Coverage 72.03% 72.00% -0.03%
==========================================
Files 369 369
Lines 58241 58328 +87
==========================================
+ Hits 41953 42001 +48
- Misses 16288 16327 +39
🚀 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
|
|
Correct me if I’m wrong, but libdatadog is language-agnostic and wouldn’t require pyo3, right?the bindings could be added directly in ddtrace-py like: https://github.com/DataDog/dd-trace-py/blob/main/src/native/ffande.rs |
Yes, somewhat. The code here is not bindings per se but helper functions that can convert from Python to Rust and back. The reason for adding it here is to satisfy Rust's orphan rule—we technically cannot The code here is added under "pyo3" feature, so other consumers of the library don't need to pull python-related dependencies. The way to think about it is that pyo3 is not a required dependency but "datadog-ffe can play nicely with pyo3 if you enable this feature." |
leoromanovsky
approved these changes
Oct 30, 2025
| /// - `bool` | ||
| /// - `NoneType` | ||
| /// | ||
| /// Note that nesting is not currently supported and will throw an error. |
| return Ok(Attribute(AttributeImpl::Number(s.value()))); | ||
| } | ||
| if let Ok(s) = value.downcast::<PyInt>() { | ||
| return Ok(Attribute(AttributeImpl::Number(s.extract::<f64>()?))); |
There was a problem hiding this comment.
Not really. We only support float attributes, so ints are squashed to floats here
| targeting_key: targeting_key | ||
| .map(|it| it.extract()) | ||
| .transpose()? | ||
| .unwrap_or_else(|| Str::from_static_str("").into()), |
There was a problem hiding this comment.
confirming empty strings are acceptable input
These will be defined in dd-trace-py to better match what Python/OF need.
This reverts commit b9c68ca. Test generation in build.rs made tests hard to edit.
dd-oleksii
force-pushed
the
ffe-pyo3-methods
branch
from
a29bb79 to
8816cf4
Compare
This is required to allow tracers return more precise error codes.
dd-oleksii
force-pushed
the
ffe-pyo3-methods
branch
from
8816cf4 to
572804b
Compare
avara1986
requested changes
Oct 31, 2025
There was a problem hiding this comment.
The UniversalFlagConfigWire structure is not correct compared to the JSON object coming from Remote Config. Consider merging this branch (and please have an expert review it, it’s my first Rust commit):
https://github.com/DataDog/libdatadog/compare/ffe-pyo3-methods...avara1986/ffe-pyo3-methods?expand=1
|
@avara1986 thanks for finding this! it's not quite related to pyo3 changes here but I fixed that by dropping support for JSON:API |
sameerank
reviewed
Oct 31, 2025
| /// ffe-specific [`Error`] enum. | ||
| pub type Result<T> = std::result::Result<T, Error>; | ||
|
|
||
| /// Enum representing possible errors that can occur in the feature flagging SDK. |
There was a problem hiding this comment.
Just an FYI: removing this shouldn't impact anything from the FFI's perspective. The FFI converts all errors to a generic ddog_Error type using a wrap_with_ffi_result method
Within wrap_with_ffi_result, there's a anyhow::Error → ddcommon_ffi::Error conversion that flattens all error details into a string message
libdatadog/ddcommon-ffi/src/error.rs
Lines 62 to 68 in 4828b75
And so there's no distinction of Rust-specific error types once they get to C
There was a problem hiding this comment.
Well... you'll probably want to change that once you get to reporting reason, error_core, and error_message to OF. It's going to painful to figure these out from error strings (which are not guaranteed to be stable).
You may want to take a look at what we do in Python here: https://github.com/DataDog/dd-trace-py/pull/15098/files#diff-d2ca810369a5f55bf62d0810211bf0d40df1f74ede215ffc430486bb1cce7545. It's basically converting Result<Assignment, EvaluationFailure> to ResolutionDetails which is very similar to what OF expects + a couple of extra fields that we need.
It might even be a good idea to move ResolutionDetails into datadog-ffe, so we can share that code between python, ruby and other SDKs
There was a problem hiding this comment.
Thank you for referring me to that PR, particularly the methods you have to map the native errors to OF errors are helpful: _map_reason_to_openfeature and _map_error_code_to_openfeature. We'll update Ruby to do the same mapping.
Moving ResolutionDetails into datadog-ffe doesn't sound like a bad idea. If the OF specs for python, ruby, and other SDKs never drift, that would be ideal. But even if one of them diverges, I imagine we could just augment the code for that specific SDK
6 tasks
avara1986
approved these changes
Nov 3, 2025
|
LGTM. Those changes work fine in dd-trace-py and system-tests 👍 ✔️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Add pyo3 conversion methods for
datadog-ffe.Motivation
This will allow our Python tracer to easily call into
datadog-ffewith Rust automatically doing conversion from Python to Rust types (and vice versa)How to test the change?
Will be tested as part of ddtrace.