feat(data-pipeline)!: OTLP gRPC trace export - #2171
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 812e43b | Docs | Datadog PR Page | Give us feedback! |
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
The in-process exporter<->h2-server round trip is too timing-sensitive for heavily contended CI runners: the client `send` has hit its request timeout on macos-15 (GitHub) and the alpine/arm release-build matrix (GitLab) across multiple runs, despite passing locally (20/20) and on less-loaded runners, and despite the live backend verification in PR #2171 succeeding. Mark it #[ignore] so it no longer runs in the default CI suites (it can't flake the PR or burn release-build runner time), while keeping it runnable on demand via `--run-ignored all`. The gRPC export path stays covered by this crate's unit tests (ProstCodec, grpc_status_to_error, attach_metadata, build_grpc_channel, builder dispatch, protocol parse) and by the live backend verification. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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
|
![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: 9dcddce271
ℹ️ 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".
The in-process exporter<->h2-server round trip is too timing-sensitive for heavily contended CI runners: the client `send` has hit its request timeout on macos-15 (GitHub) and the alpine/arm release-build matrix (GitLab) across multiple runs, despite passing locally (20/20) and on less-loaded runners, and despite the live backend verification in PR #2171 succeeding. Mark it #[ignore] so it no longer runs in the default CI suites (it can't flake the PR or burn release-build runner time), while keeping it runnable on demand via `--run-ignored all`. The gRPC export path stays covered by this crate's unit tests (ProstCodec, grpc_status_to_error, attach_metadata, build_grpc_channel, builder dispatch, protocol parse) and by the live backend verification. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
9dcddce to
093ed21
Compare
BenchmarksComparisonBenchmark execution time: 2026-07-24 21:16:28 Comparing candidate commit 812e43b in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 142 metrics, 0 unstable metrics.
|
There was a problem hiding this comment.
First, apologies for waiting so long before giving you a first review. We'll avoid that kind of delay in the future.
As far as the PR goes, I have left a bunch of comments but there's no big technical blocker on my side. However I can't approve right away because the size impact is quite important. We don't have binary size quality gates (yet), but downstream consumers of libdatadog (tracers) do, which can lead to an unfortunate situation where this would get merged but tracers would be unable to update to future releases.
A first naive question: could/should this be feature-gated? I assume you'd want all tracers to support otlp out of the box, so the answers might be no, bu asking just in case.
A good test for the impact of the binary size increase: would you mind trying to create a PR against dd-trace-py pointing to this PR's libdatadog ref, and see if it triggers the size gates for e.g. serverless?
| framed | ||
| } | ||
|
|
||
| pub fn grpc_export_benches(c: &mut Criterion) { |
There was a problem hiding this comment.
What are we benchmarking in this function? I'm asking because we had slow benchmark jobs recently (and I've been one of the usual offender :whistling:). I would say we should only add benchmark if:
- we bench internal code, and not external libraries (it can be useful to bench end-to-end since we do rely a lot on external libraries, but I don't think this is the right place for those benches)
- we bench performance-sensitive code / code on the hot path
- we are interested in the evolution of the performance of this code. Like, we want to avoid having regression here, or we want to optimize it in the future and thus would like to have a baseline.
Do you think those applies here?
| /// All three OTel-standard protocol strings parse successfully; the selection | ||
| /// controls which send path the exporter uses: | ||
| /// - `http/json` and `http/protobuf` → OTLP over HTTP/1.1 via | ||
| /// [`HttpClientCapability`](libdd_capabilities::HttpClientCapability). | ||
| /// - `grpc` → OTLP over HTTP/2 via a tonic [`Channel`](tonic::transport::Channel). | ||
| /// | ||
| /// Plaintext gRPC (`http://` scheme, port 4317) is supported. TLS gRPC | ||
| /// (`https://` scheme) is not yet implemented — use a TLS-terminating sidecar. |
There was a problem hiding this comment.
Nit: I find this comment confused and not very useful. I think the first line and each variant's description is enough.
| /// gRPC over HTTP/2. Protobuf-encoded body with 5-byte gRPC framing. | ||
| /// Default port is 4317. Only plaintext (`http://`) is supported. |
There was a problem hiding this comment.
(Unless any of what comes after that in non-standard, I don't think it's relevant here)
| /// gRPC over HTTP/2. Protobuf-encoded body with 5-byte gRPC framing. | |
| /// Default port is 4317. Only plaintext (`http://`) is supported. | |
| /// gRPC over HTTP/2 |
| match self { | ||
| OtlpProtocol::HttpJson => libdd_common::header::APPLICATION_JSON, | ||
| OtlpProtocol::HttpProtobuf => libdd_common::header::APPLICATION_PROTOBUF, | ||
| OtlpProtocol::Grpc => unreachable!("gRPC path does not call content_type()"), |
There was a problem hiding this comment.
I would avoid unreachable here, because it makes a hypothesis on every possible caller, past and future. Basically we don't have enough information to prove it's unreachable; if someone somedays adds code that call protocol.content_type() without filtering Grpc out, this will panic.
Suggestion: either return some default value that vaguely makes sense, or if there is not, make this function return an optional. The caller can then take the responsibility of unwrap(), but at least they have to make that choice explicitly.
| OtlpProtocol::HttpProtobuf => { | ||
| Ok(libdd_trace_utils::otlp_encoder::encode_otlp_protobuf(req)) | ||
| } | ||
| OtlpProtocol::Grpc => unreachable!("gRPC path does not call encode()"), |
| /// | ||
| /// Only one variant is active per instance; mutual exclusivity is enforced at | ||
| /// build time. |
There was a problem hiding this comment.
| /// | |
| /// Only one variant is active per instance; mutual exclusivity is enforced at | |
| /// build time. |
There was a problem hiding this comment.
OtlpExportMode reworked in the rebuild; it's a one-line doc now.
| r.client_computed_stats = self.otlp_stats_enabled; | ||
| r | ||
| }; | ||
| let resource_info = self.build_otlp_resource_info(); |
There was a problem hiding this comment.
Not really related to this PR, but should we cache the resource info in the struct instead of creating it and cloning strings on every request? Unless the metadata can change? Or maybe map_trace_to_otlp should take LanguageMetadata instead. Or make ResourceInfo borrowed. But it does sounds wasteful to clone 7ish strings on each send (that are probably not changing often or at all), just to pass a reference to them to map_traces_to_otlp after.
There was a problem hiding this comment.
Did it as a nice bonus in 812e43b but let me know if you'd prefer I split this out into another PR
| if self.otlp.is_some() { | ||
| libdd_trace_utils::span::trace_utils::drop_chunks(&mut traces); | ||
| if traces.is_empty() { | ||
| return Ok(AgentResponse::Unchanged); | ||
| } | ||
| return self.send_otlp_traces_inner(traces, config).await; | ||
| } | ||
| match &self.otlp { | ||
| Some(OtlpExportMode::Http(config)) => { | ||
| return self.send_otlp_traces_inner(traces, config).await; | ||
| } | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| Some(OtlpExportMode::Grpc(transport)) => { | ||
| return self.send_otlp_grpc_inner(traces, transport).await; | ||
| } | ||
| None => {} | ||
| } |
There was a problem hiding this comment.
| if self.otlp.is_some() { | |
| libdd_trace_utils::span::trace_utils::drop_chunks(&mut traces); | |
| if traces.is_empty() { | |
| return Ok(AgentResponse::Unchanged); | |
| } | |
| return self.send_otlp_traces_inner(traces, config).await; | |
| } | |
| match &self.otlp { | |
| Some(OtlpExportMode::Http(config)) => { | |
| return self.send_otlp_traces_inner(traces, config).await; | |
| } | |
| #[cfg(not(target_arch = "wasm32"))] | |
| Some(OtlpExportMode::Grpc(transport)) => { | |
| return self.send_otlp_grpc_inner(traces, transport).await; | |
| } | |
| None => {} | |
| } | |
| if let Some(otlp) = &self.otlp { | |
| libdd_trace_utils::span::trace_utils::drop_chunks(&mut traces); | |
| if traces.is_empty() { | |
| return Ok(AgentResponse::Unchanged); | |
| } | |
| match otlp { | |
| OtlpExportMode::Http(config) => { | |
| return self.send_otlp_traces_inner(traces, config).await; | |
| } | |
| #[cfg(not(target_arch = "wasm32"))] | |
| OtlpExportMode::Grpc(transport) => { | |
| return self.send_otlp_grpc_inner(traces, transport).await; | |
| } | |
| } | |
| } |
| # tonic's transport (hyper/tokio/socket2) does not build for wasm32, so the gRPC | ||
| # OTLP transport — and prost, which only the gRPC exporter uses — are gated off | ||
| # wasm targets along with the code that uses them. |
There was a problem hiding this comment.
| # tonic's transport (hyper/tokio/socket2) does not build for wasm32, so the gRPC | |
| # OTLP transport — and prost, which only the gRPC exporter uses — are gated off | |
| # wasm targets along with the code that uses them. | |
| # tonic's transport does not build for wasm32. Prost is only used by the gRPC | |
| # exporter, so it's gated as well. |
There was a problem hiding this comment.
Deps section was reworked for the new transport in #2273; that comment's shorter now.
| #[cfg(test)] | ||
| mod grpc_export_tests { | ||
| use bytes::Bytes; | ||
| use h2::server; |
There was a problem hiding this comment.
Any reason for using a h2 server instead of the usual http mock server we've been using elsewhere in tests?
There was a problem hiding this comment.
httpmock here is HTTP/1.1-only (no http2 feature) and has no trailer support, so it can't serve gRPC - the unary status rides in HTTP/2 trailers. That's why the test hand-rolls an h2 server.
|
So, we've had a discussion with @VianneyRuhlmann about the whole worker situation. We believe we can side-step the "task in the background that needs to be restared" entirely by plugging in our own |
Wires OTLP gRPC trace export into TraceExporter on top of the fork-safe gRPC transport, selectable via OTEL_EXPORTER_OTLP_TRACES_PROTOCOL=grpc. OtlpProtocol gains a Grpc variant; the exporter dispatches through a new OtlpExportMode (Http | Grpc) to either the existing HTTP path or the gRPC transport (send_otlp_traces_grpc), with bounded exponential retry on transient IO matching the HTTP path. The OTLP resource info is built once at construction and shared by both paths. gRPC is native-only; wasm32 rejects it at build time. Includes a public-API end-to-end gRPC export test. BREAKING CHANGE: adds the Grpc variant to the exhaustive public OtlpProtocol enum, so exhaustive matches on it must add an arm. libdatadog consumers pin by version and pick this up on the next release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
093ed21 to
812e43b
Compare
bm1549
changed the title
2 participants
What does this PR do?
Adds OTLP gRPC as a trace-export transport, selectable via
OTEL_EXPORTER_OTLP_TRACES_PROTOCOL=grpc.OtlpProtocolgains aGrpcvariant;TraceExporterdispatches through a newOtlpExportMode(Http|Grpc) to either the existing HTTP path or the gRPC transport, with bounded exponential retry on transient IO matching the HTTP path. The OTLP resource info is built once at construction. gRPC is native-only; wasm32 rejects it at build. The FFI acceptsgrpc, so SDKs get gRPC export through libdatadog.Stacked on #2273 (the fork-safe gRPC transport primitive). Review/merge that first; this PR's diff is only the export-integration layer on top of it.
Motivation
libdatadog's OTLP trace export was HTTP-only. Java and .NET do gRPC OTLP by hand; PHP, Python, Ruby, and Rust export through libdatadog FFI, so adding gRPC here unblocks all four and lets SDKs honor
OTEL_EXPORTER_OTLP_TRACES_PROTOCOL=grpcinstead of coercing it to HTTP.Additional Notes
tower::Serviceover hyper h2c, per-request, fork-safe, no worker); this PR wires it into the exporter.OtlpProtocol::from_str("grpc")now succeeds (was rejected). Breaking change on the exhaustive public enum, per the commit's BREAKING CHANGE footer.Cancelled) with the sameOTLP_MAX_RETRIES + 1attempts and exponential backoff as the HTTP path.How to test the change?
cargo nextest run -p libdd-data-pipeline -p libdd-data-pipeline-fficovers unit + FFI tests, including a public-API end-to-end gRPC export test (TraceExporterBuilder→send→ in-process h2 gRPC server that decodes theExportTraceServiceRequest), and builder tests for grpc-endpoint success, no-endpoint agent fallback, and https rejection.cargo clippy -p libdd-data-pipeline -p libdd-data-pipeline-ffi --all-targets -- -D warnings,cargo +nightly fmt --check, andcargo check -p libdd-data-pipeline --target wasm32-unknown-unknown --no-default-features(gRPC is wasm-gated) pass.