Repository navigation
feat(libdd-data-pipeline-ffi): add ddog_trace_exporter_builder_* FFI mirroring the Rust builder - #2689
Open
VianneyRuhlmann wants to merge 2 commits into
Open
feat(libdd-data-pipeline-ffi): add ddog_trace_exporter_builder_* FFI mirroring the Rust builder#2689VianneyRuhlmann wants to merge 2 commits into
VianneyRuhlmann wants to merge 2 commits into
Conversation
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0a0a7e9 | Docs | View more details | Give us feedback! |
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
…mirroring the Rust builder Add a ddog_trace_exporter_builder_* FFI that maps 1:1 onto TraceExporterBuilder, alongside the existing ddog_trace_exporter_config_* API which is now deprecated (Rust #[deprecated] plus a portable DDOG_TRACE_EXPORTER_DEPRECATED macro in the generated header; behavior unchanged). - enable-only setters for health metrics, stats and client-computed stats/headers; enable_stats takes a bucket interval in seconds - ddog_trace_exporter_builder_build consumes the builder, including on error - new C types: ddog_ExporterTelemetryConfig (2 fields), ddog_ExporterTagList, ddog_ExporterHeader(List) for Vec-based builder methods; telemetry enable and instrumentation sessions are separate bindings - parity: 47 of 48 builder methods bound (build_async is not representable; the 2 stats-obfuscation-gated ones are excluded because cbindgen does not evaluate #[cfg]) - migrate examples/ffi/trace_exporter.c to the builder API
VianneyRuhlmann
force-pushed
the
vianney/remove-config-in-ffi
branch
from
October 19, 2026 12:47
f85766e to
4f7d8c3
Compare
Contributor
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
|
VianneyRuhlmann
marked this pull request as ready for review
November 9, 2026 20:18
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. |
![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: 0a0a7e9468
ℹ️ 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".
Comment on lines
+141
to
+144
| pub unsafe extern "C" fn ddog_trace_exporter_builder_free( | ||
| handle: Box<TraceExporterBuilder<ForkSafeRuntime>>, | ||
| ) { | ||
| drop(handle); |
There was a problem hiding this comment.
Wrap this drop in catch_panic! (or add a justified panic-boundary allowance only if the entire destructor chain is proven panic-free). A builder may own nontrivial state such as the last ForkSafeRuntime reference, so if destruction unwinds, the panic currently crosses the extern "C" boundary and aborts the embedding process instead of being contained.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
Comment on lines
+408
to
+414
| pub unsafe extern "C" fn ddog_trace_exporter_builder_enable_stats( | ||
| builder: Option<&mut TraceExporterBuilder<ForkSafeRuntime>>, | ||
| bucket_interval_secs: u64, | ||
| ) -> Option<Box<ExporterError>> { | ||
| catch_panic!( | ||
| if let Option::Some(builder) = builder { | ||
| builder.enable_stats(Duration::from_secs(bucket_interval_secs)); |
There was a problem hiding this comment.
Reject bucket_interval_secs == 0 rather than storing Duration::ZERO. When OTLP metrics are configured, build immediately constructs a SpanConcentrator, whose timestamp alignment performs modulo by the bucket size and therefore panics; on the agent path the same panic occurs later when the agent enables client-side stats. Thus an accepted C input can make exporter construction return a panic error or prevent stats from starting instead of returning a clear invalid-argument error here.
AGENTS.md reference: AGENTS.md:L95-L98
Useful? React with 👍 / 👎.
Comment on lines
+700
to
+703
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn ddog_trace_exporter_builder_default_stats_cardinality_limit() | ||
| -> CardinalityLimitConfig { | ||
| CardinalityLimitConfig::default() |
There was a problem hiding this comment.
Wrap this new exported accessor with catch_panic!, or place the required justified allow(ffi-panic-boundary) comment directly above it if CardinalityLimitConfig::default is intentionally treated as panic-free. As written, this is an unacknowledged extern "C" boundary, so a future panic introduced anywhere in the default implementation would unwind across C and abort the host process.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
This branch has not been deployed
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
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.
Summary
This PR deprecates the intermediate config struct in libdd-data-pipeline-ffi to expose the builder directly removing discrepancies in the setters and reduces the drift between the native api and the ffi api.
Key points
ddog_trace_exporter_builder_buildconsumes the builder, including on build failure; legacyddog_trace_exporter_newstill leaves the config with the caller.enable_statstakes a bucket interval in seconds (10= default).ddog_ExporterTelemetryConfig(2 fields),ddog_ExporterTagList,ddog_ExporterHeader/ddog_ExporterHeaderListfor theVec-based builder methods. Telemetry enable and instrumentation-sessions are separate bindings (no more 2-in-1).set_lang_version→set_language_version,set_version→set_app_version,set_compute_stats→enable_stats, etc.build_asyncis not representable over C FFI. The three feature-gated methods are excluded here — cbindgen does not evaluate#[cfg], so gated bindings would be declared in the default header while missing from the compiled library: twostats-obfuscation-gated methods are added in a stacked PR that makes the feature default-on;enable_observations(external-observations) is likewise deferred to a follow-up that resolves its feature gating.examples/ffi/trace_exporter.cmigrated to the builder API.Validation
cargo check, clippy-D warnings, doctests, pinned-nightly fmt, 189 tests across the changed crates (MSRV 1.87 and stable, withexternal-observationsenabled), fullcargo ffi-test(12 pass;ffefails on a pre-existing missing fixture, identical onmain). Both C API paths (builder + unchanged legacy example frommain) compile and run.Notes for reviewers
-Werror=deprecated-declarationsmust suppress or migrate.