Conversation
📚 Documentation Check Results📦
|
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
🔒 Cargo Deny Results📦
|
BenchmarksComparisonBenchmark execution time: 2026-03-11 23:45:37 Comparing candidate commit 82435cb in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 2 unstable metrics.
|
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
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1641 +/- ##
==========================================
+ Coverage 71.25% 71.30% +0.04%
==========================================
Files 429 431 +2
Lines 63547 63964 +417
==========================================
+ Hits 45279 45608 +329
- Misses 18268 18356 +88
🚀 New features to boost your workflow:
|
| /// Sets the tracer/SDK name for OTLP resource attribute `telemetry.sdk.name` | ||
| /// (e.g. "dd-trace-py"). When unset, OTLP uses "libdatadog". | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn ddog_trace_exporter_config_set_tracer_name( |
There was a problem hiding this comment.
Is tracer name a required field in the otlp trace payload. It would be nice if we went to with a minimal implementation where we defer unnecessary configurations. Also can the tracer name be interfered from other configurations?
|
|
||
| #[derive(Debug, Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct KeyValue { |
There was a problem hiding this comment.
Did we look into vendoring or adding opentelemetry-rust as a dependency to the libdd-data-pipeline so we can re-use existing functionality (ex).
At the minimum we should make it clear which components are borrowed from opentelemetry-rust and include a versioned link to the original implementation.
There was a problem hiding this comment.
I talked to Paul about this and we agreed that adding open-telemetry as a dependency would be more work and not worth the hassle.
There was a problem hiding this comment.
At the minimum we should make it clear which components are borrowed from opentelemetry-rust and include a versioned link to the original implementation.
I can add this. 🫡
| } | ||
|
|
||
| #[derive(Debug, Serialize)] | ||
| #[serde(rename_all = "camelCase")] |
There was a problem hiding this comment.
DO we need the rename_all option here?
| } | ||
|
|
||
| /// OTLP SpanKind enum values. | ||
| pub mod span_kind { |
There was a problem hiding this comment.
Why are we using snakeCasing here? This is inconsistent with AnyValue,
There was a problem hiding this comment.
I don't really understand this comment? module names should be snake_case, and the naming does not impact how the structs are serialised
paullegranddc
left a comment
paullegranddc
left a comment
There was a problem hiding this comment.
To test the otlp trace export, you can add test agent snapshot tests here
| // Copyright 2024-Present Datadog, Inc. https://www.datadoghq.com/ | ||
| // SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
I think we should parse the configurations from the host language, and not in libdatadg.
The way we would configure the TraceExporter for agent_url for instance would be
1) language_configuration resolves agent_url from different sources
2) when the tracer wants to instantiate a TraceExporter it first creates a TraceExporterBuilder and calls setters on it (set_agent_url in this case)
3) TraceExporterBuilder::build gives a TraceExporter containing the agent_url
4) In TraceExporter::send we can just read self.agent_url
| /// Note: dynamic OTLP headers from `OTEL_EXPORTER_OTLP_HEADERS` are not forwarded because | ||
| /// [`send_with_retry`] requires `&'static str` header keys. Support for arbitrary OTEL headers | ||
| /// would require the API to accept `HashMap<String, String>`. |
There was a problem hiding this comment.
mmm true 🤔
I'll refactor send_with_retry so it's possible
| // (equivalent to OTEL_TRACES_SAMPLER=parentbased_always_on). | ||
| if let Some(ref config) = self.otlp_config { | ||
| return self.send_otlp_traces_inner(traces, config).await; | ||
| } |
There was a problem hiding this comment.
I missed it the first pass, but we should not send non sampled spans https://docs.google.com/document/d/1AsUrJxjJavLvSG33kUAJLYzGU8IYgrnf1SMbFDsuGmo/edit?tab=t.0#bookmark=id.rj2adtyzxygy
So this should be moved after the stats::process_traces_for_stats call line 630
4 participants
What does this PR do?
Summary
This PR adds an OTLP HTTP/JSON trace export path to TraceExporter. When
OTEL_TRACES_EXPORTER=otlpis set, the exporter converts Datadog msgpack trace payloads to OTLP and POSTs them to the configured OTLP endpoint instead of the Datadog agent. The caller (e.g. dd-trace-py) sends traces in the same format as before — no changes are required on the tracer side.Not in scope
Motivation
We are seeing an increasing number of scenarios where users have applications instrumented with the OTel SDK sending data to OTel collectors, and they would like to get additional features offered by the DD SDK without needing to update their OTel collector deployments. Although there will be follow-up work, this provides the ability for users to write vendor-neutral API instrumentation and emit vendor-neutral telemetry data so DD SDK users don't have to feel locked in when setting up Datadog APM.
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.