Repository navigation
Conversation
Adapt the shared trace model and Python wrappers to the upgraded native baseline while keeping dependent-flag capability advertisement disabled. Environment: Datadog workspace Co-Authored-By: Codex GPT-6 <noreply@localhost>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
BenchmarksBenchmark execution time: 2026-11-09 20:41:44 Comparing candidate commit d345b05 in PR branch Found 13 performance improvements and 0 performance regressions! Performance is the same for 616 metrics, 10 unstable metrics, 1 flaky benchmarks without significant changes.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d345b05922
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| Self::from_box(Box::<str>::from(self.deref())) |
There was a problem hiding this comment.
When trace-model cloning is exercised for Python-backed span text, this new Clone implementation allocates and copies every non-static value, potentially affecting hot trace/event processing. The commit provides functional tests but no benchmark results for this native runtime change, so please measure representative Python-backed string workloads and report the performance impact before merging.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Claude Code, working with Mark:
The same Clone implementation has been on main since #21576. This PR's benchmark run shows no regressions (13 improvements, 0 regressions; msgpack span encoding −7% to −9%). libdatadog's stats path copies &str with to_owned/to_string rather than calling TraceText::clone.
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The dependency pins are consistent across the manifest and lockfile, and FFE continues to advertise no dependent-flag capability. Supported-platform build and runtime compatibility remain unverified.
🤖 Bits Code Review · Commit d345b05
mark-caii
left a comment
mark-caii
left a comment
There was a problem hiding this comment.
Claude Code, working with Mark:
CI is green at d345b05. The main blocker is the libdatadog pin (see the inline comment on src/native/Cargo.toml). Other notes:
[P2] Much of this PR overlaps with work that main already has. #21576 (libdatadog v48.0.0, merged Nov 6, not in a release yet) has the same py_string.rs changes (Clone, From<String>, TraceText, ValueTypes), the same RUST_MINIMUM_VERSION in setup.py, and parts of the Cargo.toml and Cargo.lock changes. Four files will conflict when the stack reaches main. Suggestion: merge main into the stack first and keep the py_string.rs from main, because its Clone comment is more complete. This PR then shrinks to the libdatadog bump, PyO3 0.30, the #[pyclass] cleanup, and the ffe.rs doc comment.
[P2] The release note releasenotes/notes/native-toolchain-upgrade-08a2e3fdd11cbcc4.yaml can land in the wrong release. main raised the Rust minimum to 1.91.1 in #21576 without a release note, and that change is not in a release tag yet. Suggestion: add the note on main and drop it here, unless this stack lands before the next release.
[P3] Stale test comment. tests/openfeature/test_agentless_poller.py:259-260 is outside this PR's diff, so this note is here and not inline. The comment says "The native dependency does not implement prerequisite evaluation yet." The linked evaluator now evaluates prerequisites; only the binding does not surface them. #21346 removes this test, so this matters only if this PR lands without #21346. In that case, suggestion: reuse the updated wording from src/native/ffe.rs: "Prerequisite records and exposure eligibility are not yet surfaced by this binding, so transports must not advertise support."
Heads up: with this evaluator, dependent-flag configs change behaviour even when the capability is not advertised.
- Before (libdatadog v46.1.0): a condition with
flagEvaluationand noattributefailed to parse. The binding returnedErrorCode.ParseErrorwithdo_log=False, and the provider returned the caller's default value. - Now: the prerequisites are evaluated, and Python gets the root flag's assignment and its
do_log. Prerequisite exposures are dropped, becauseget_assignmentdiscards the prerequisite records. A failed prerequisite maps toErrorCode.General.
This matters only if the backend sends dependent flags to clients that do not advertise support. Please confirm that the backend does not do this before #21346 advertises the capability.
| libdd-live-debugger = { git = "https://github.com/DataDog/libdatadog", rev = "v46.1.0" } | ||
| libdd-remote-config = { git = "https://github.com/DataDog/libdatadog", rev = "v46.1.0", features = [ | ||
| # Keep libdatadog crates on one revision to share native types. | ||
| libdd-ffe = { git = "https://github.com/DataDog/libdatadog", rev = "cb3bfba0d51adba06f9afca32841573fa7bad0b3", optional = true, features = ["pyo3"] } |
There was a problem hiding this comment.
Claude Code, working with Mark:
[P0, before merge] Every libdatadog rev in this file is pinned to cb3bfba0d5. This covers lines 38-91, including build_common and the Linux-only target dependencies. Please re-pin all of them to a libdatadog release tag before merge.
The PR body already says that an approved native release is needed before merge. This comment makes the crash risk at the current pin concrete.
cb3bfba0d5is not a release and is not on libdatadogmain. It is an older commit of the open evaluator PR feat(ffe): evaluate dependent flags with prerequisite telemetry libdatadog#2748, which has changes requested. That PR's head has moved to DataDog/libdatadog@62a746e. The newer head adds a heap-backed evaluation stack (DataDog/libdatadog@e9c5d82) and drops error chains iteratively (DataDog/libdatadog@a759be2).- At the pinned commit, dependency evaluation recurses once per dependency edge. A libdatadog reviewer measured a 255-edge chain with
maxDependencyDepth: 255. In release builds on macOS arm64, it overflowed 128 KiB and 256 KiB thread stacks, and Rust aborts the process on a stack overflow. This was measured upstream at DataDog/libdatadog@983b4b0, which has the same recursive code. I did not reproduce it in Python. - Python reaches this code through
resolve_value, which callsget_assignment, for any flag with aflagEvaluationcondition. This happens whether or not the capability is advertised. cb3bfba0d5is reachable only from feature branches and the PR ref. A rebase or force-push of feat(ffe): evaluate dependent flags with prerequisite telemetry libdatadog#2748, plus branch cleanup, can make it unfetchable for source builds.mainpins libdatadog to release tags (currentlyv48.0.0).
Fix: after DataDog/libdatadog#2748 is merged and released, set every libdatadog dependency to that tag. Then regenerate Cargo.lock and name the tag in the setup.py comment.
| # DEV: update this accordingly when src/native upgrades libdatadog dependency. | ||
| # libdatadog v46.1.0 requires rust 1.87.0. | ||
| RUST_MINIMUM_VERSION = "1.87.0" | ||
| # The mainline libdatadog dependency requires Rust 1.91.1. |
There was a problem hiding this comment.
Claude Code, working with Mark:
[P3] This comment says "The mainline libdatadog dependency requires Rust 1.91.1", but the pin cb3bfba0d5 is not on libdatadog main. main in this repo already uses the release form of this line, from #21576:
# libdatadog v48.0.0 requires rust 1.91.1.
When you re-pin to a release, please keep that form and set the version to the tag that you pin.
| } else { | ||
| Self::from_box(Box::<str>::from(self.deref())) |
There was a problem hiding this comment.
Claude Code, working with Mark:
The same Clone implementation has been on main since #21576. This PR's benchmark run shows no regressions (13 improvements, 0 regressions; msgpack span encoding −7% to −9%). libdatadog's stats path copies &str with to_owned/to_string rather than calling TraceText::clone.
Description
Updates the shared libdatadog revision and aligns PyO3, its build dependencies, and the minimum source-build Rust version. Adapts trace-model integration and Python extraction annotations to the new APIs while preserving the existing Python-facing native interfaces.
The FFE binding continues to advertise no dependent-flag capability. Capability activation, prerequisite telemetry, and exposure finalization belong to the feature branch layered above this prerequisite.
Testing
Risks
This revision changes shared native dependencies used by tracing, profiling, crash tracking, and Remote Configuration. It requires their supported-platform CI coverage and an approved native release before merge. Source builds require Rust 1.91.1 or later.