Skip to content

refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API #2152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hoolioh wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API #2152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion datadog-sidecar/src/service/sidecar_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,12 @@ impl SidecarInterface for ConnectionSidecarHandler {
libdd_telemetry::config::PROD_INTAKE_SUBDOMAIN,
&config.endpoint,
);
cfg.set_endpoint(endpoint).ok();
// Set the api key before the uri so the telemetry path is resolved correctly.
cfg.set_endpoint_api_key(endpoint.api_key.as_deref()).ok();
cfg.set_endpoint_uri(endpoint.url.clone()).ok();
cfg.set_endpoint_timeout_ms(endpoint.timeout_ms);
cfg.set_endpoint_test_token(endpoint.test_token.clone());
cfg.set_endpoint_use_system_resolver(endpoint.use_system_resolver);
cfg.telemetry_heartbeat_interval = config.telemetry_heartbeat_interval;
cfg.telemetry_extended_heartbeat_interval =
config.telemetry_extended_heartbeat_interval;
Expand Down
5 changes: 2 additions & 3 deletions examples/ffi/telemetry.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ int main(void) {
TRY(ddog_telemetry_builder_instantiate(&builder, service, lang, lang_version, tracer_version));

ddog_CharSlice endpoint_char = DDOG_CHARSLICE_C("file://./examples_telemetry.out");
struct ddog_Endpoint *endpoint = ddog_endpoint_from_url(endpoint_char);
TRY(ddog_telemetry_builder_with_endpoint_config_endpoint(builder, endpoint));
ddog_endpoint_drop(endpoint);
TRY(ddog_telemetry_builder_with_endpoint_config_endpoint(
builder, endpoint_char, DDOG_CHARSLICE_C(""), 0, DDOG_CHARSLICE_C(""), false));

ddog_CharSlice runtime_id = DDOG_CHARSLICE_C("fa1f0ed0-8a3a-49e8-8f23-46fb44e24579"),
service_version = DDOG_CHARSLICE_C("1.0"), env = DDOG_CHARSLICE_C("test");
Expand Down
5 changes: 2 additions & 3 deletions examples/ffi/telemetry_metrics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ int main(void) {
TRY(ddog_telemetry_builder_instantiate(&builder, service, lang, lang_version, tracer_version));

ddog_CharSlice endpoint_char = DDOG_CHARSLICE_C("file://./examples_telemetry_metrics.out");
struct ddog_Endpoint *endpoint = ddog_endpoint_from_url(endpoint_char);
TRY(ddog_telemetry_builder_with_endpoint_config_endpoint(builder, endpoint));
ddog_endpoint_drop(endpoint);
TRY(ddog_telemetry_builder_with_endpoint_config_endpoint(
builder, endpoint_char, DDOG_CHARSLICE_C(""), 0, DDOG_CHARSLICE_C(""), false));

ddog_CharSlice runtime_id = DDOG_CHARSLICE_C("fa1f0ed0-8a3a-49e8-8f23-46fb44e24579"),
service_version = DDOG_CHARSLICE_C("1.0"), env = DDOG_CHARSLICE_C("test");
Expand Down
7 changes: 6 additions & 1 deletion libdd-crashtracker/src/crash_info/telemetry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,12 @@ impl TelemetryCrashUploader {
.context("file path is not valid")?;
cfg.set_host_from_url(&format!("file://{}.telemetry", path.display()))
} else {
cfg.set_endpoint(endpoint.clone())
cfg.set_endpoint_timeout_ms(endpoint.timeout_ms);
cfg.set_endpoint_test_token(endpoint.test_token.clone());
cfg.set_endpoint_use_system_resolver(endpoint.use_system_resolver);
// Set the api key before the uri so the telemetry path is resolved correctly.
cfg.set_endpoint_api_key(endpoint.api_key.as_deref())
.and_then(|()| cfg.set_endpoint_uri(endpoint.url.clone()))
};
}

Expand Down
4 changes: 1 addition & 3 deletions libdd-data-pipeline/src/telemetry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ impl TelemetryClientBuilder {

/// Sets the url where the metrics will be sent.
pub fn set_url(mut self, url: &str) -> Self {
let _ = self
.config
.set_endpoint(libdd_common::Endpoint::from_slice(url));
let _ = self.config.set_endpoint_url(url);
self
}

Expand Down
93 changes: 78 additions & 15 deletions libdd-telemetry-ffi/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// SPDX-License-Identifier: Apache-2.0

use ffi::slice::AsBytes;
use libdd_common::Endpoint;
use libdd_common_ffi as ffi;
use libdd_telemetry::{
data,
Expand Down Expand Up @@ -155,14 +154,60 @@ pub unsafe extern "C" fn ddog_telemetry_builder_run_metric_logs(
MaybeError::None
}

/// Applies endpoint settings to the builder's telemetry config from primitive
/// values, so `libdd_common::Endpoint` stays out of this crate's public API.
///
/// `api_key` and `test_token` are treated as unset when empty; a `timeout_ms` of
/// 0 keeps the existing/default timeout.
fn set_builder_endpoint(
telemetry_builder: &mut TelemetryWorkerBuilder,
url: ffi::CharSlice,
api_key: ffi::CharSlice,
timeout_ms: u64,
test_token: ffi::CharSlice,
use_system_resolver: bool,
) -> ffi::MaybeError {
let url = try_c!(url.try_to_utf8());
let api_key = api_key.to_utf8_lossy();
let test_token = test_token.to_utf8_lossy();
let config = &mut telemetry_builder.config;
// Set the api key before the url so the telemetry path is resolved correctly.
if !api_key.is_empty() {
try_c!(config.set_endpoint_api_key(Some(api_key.as_ref())));
}
Comment on lines +175 to +177

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear stale API keys when replacing endpoints

When the builder was instantiated from an environment with _DD_DIRECT_SUBMISSION_ENABLED=true and DD_API_KEY set, passing an empty api_key here no longer clears that key while replacing the endpoint. The previous Endpoint-based setter replaced the whole endpoint, so ddog_endpoint_from_url(...) produced an endpoint with no API key; now the old key is preserved and set_endpoint_url rewrites HTTP URLs to the direct-submission path, causing callers that explicitly configure an agent/file endpoint without a key to send with the stale API key and potentially to /api/v2/apmtelemetry instead of the agent proxy path.

Useful? React with 👍 / 👎.

@yannham yannham Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the solution is to set to None if the api key is empty, instead of not setting anything?

try_c!(config.set_endpoint_url(url));
if timeout_ms != 0 {
config.set_endpoint_timeout_ms(timeout_ms);
}
if !test_token.is_empty() {
config.set_endpoint_test_token(Some(test_token.into_owned()));
}
config.set_endpoint_use_system_resolver(use_system_resolver);
ffi::MaybeError::None
}

#[no_mangle]
#[allow(clippy::missing_safety_doc)]
/// Sets the telemetry endpoint from its component parts.
///
/// * `api_key` / `test_token`: ignored when empty.
/// * `timeout_ms`: pass 0 to keep the existing/default timeout.
pub unsafe extern "C" fn ddog_telemetry_builder_with_endpoint_config_endpoint(
telemetry_builder: &mut TelemetryWorkerBuilder,
endpoint: &Endpoint,
url: ffi::CharSlice,
api_key: ffi::CharSlice,
timeout_ms: u64,
test_token: ffi::CharSlice,
use_system_resolver: bool,
Comment on lines +197 to +201

@yannham yannham Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The endpoint seems to carry quite a bit of different fields, that now have to be set manually one by one and inflate function arguments (there's a risk to forget about one field when setting the endpoint now instead of cfg.set_endpoint previously). I'm just thinking out loud, but would it make sense to re-introduce an Endpoint (or a different name) but stand-alone in libdd-telemetry that packs url, api_key, timeout_ms, together ? At least if we always want to set them all at once.

@hoolioh hoolioh Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but I ended up adding more parameters here. However you have a point here, maybe taking the same approach as in TelemetryClientConfig would be better? WDYT?

) -> ffi::MaybeError {
try_c!(telemetry_builder.config.set_endpoint(endpoint.clone()));
ffi::MaybeError::None
set_builder_endpoint(
telemetry_builder,
url,
api_key,
timeout_ms,
test_token,
use_system_resolver,
)
}
#[repr(C)]
#[allow(dead_code)]
Expand All @@ -172,38 +217,56 @@ pub enum TelemetryWorkerBuilderEndpointProperty {

#[no_mangle]
#[allow(clippy::missing_safety_doc)]
/// Sets a property from it's string value.
/// Sets the endpoint property from its component parts.
///
/// Available properties:
///
/// * config.endpoint
pub unsafe extern "C" fn ddog_telemetry_builder_with_property_endpoint(
telemetry_builder: &mut TelemetryWorkerBuilder,
_property: TelemetryWorkerBuilderEndpointProperty,
endpoint: &Endpoint,
url: ffi::CharSlice,
api_key: ffi::CharSlice,
timeout_ms: u64,
test_token: ffi::CharSlice,
use_system_resolver: bool,
) -> ffi::MaybeError {
try_c!(telemetry_builder.config.set_endpoint(endpoint.clone()));
ffi::MaybeError::None
set_builder_endpoint(
telemetry_builder,
url,
api_key,
timeout_ms,
test_token,
use_system_resolver,
)
}
#[no_mangle]
#[allow(clippy::missing_safety_doc)]
/// Sets a property from it's string value.
/// Sets a named endpoint property from its component parts.
///
/// Available properties:
///
/// * config.endpoint
pub unsafe extern "C" fn ddog_telemetry_builder_with_endpoint_named_property(
telemetry_builder: &mut TelemetryWorkerBuilder,
property: ffi::CharSlice,
endpoint: &Endpoint,
url: ffi::CharSlice,
api_key: ffi::CharSlice,
timeout_ms: u64,
test_token: ffi::CharSlice,
use_system_resolver: bool,
) -> ffi::MaybeError {
let property = try_c!(property.try_to_utf8());

match property {
"config . endpoint" => {
try_c!(telemetry_builder.config.set_endpoint(endpoint.clone()));
}
_ => return ffi::MaybeError::None,
"config . endpoint" => set_builder_endpoint(
telemetry_builder,
url,
api_key,
timeout_ms,
test_token,
use_system_resolver,
),
_ => ffi::MaybeError::None,
}
ffi::MaybeError::None
}
28 changes: 18 additions & 10 deletions libdd-telemetry-ffi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ mod tests {
use crate::{builder::*, worker_handle::*};
use ffi::tags::{ddog_Vec_Tag_new, ddog_Vec_Tag_push, PushTagResult};
use ffi::MaybeError;
use libdd_common::Endpoint;
use libdd_common_ffi as ffi;
use libdd_telemetry::{
data::{
Expand All @@ -137,9 +136,14 @@ mod tests {
let mut builder = builder.assume_init();

let f = tempfile::NamedTempFile::new().unwrap();
let url = format!("file://{}", f.path().to_str().unwrap());
ddog_telemetry_builder_with_endpoint_config_endpoint(
&mut builder,
&Endpoint::from_slice(&format!("file://{}", f.path().to_str().unwrap())),
ffi::CharSlice::from(url.as_str()),
ffi::CharSlice::from(""),
0,
ffi::CharSlice::from(""),
false,
)
.unwrap_none();

Expand Down Expand Up @@ -306,13 +310,15 @@ mod tests {
let mut builder = builder.assume_init();

let f = tempfile::NamedTempFile::new().unwrap();
let url = format!("file://{}", f.path().as_os_str().to_str().unwrap());
assert_eq!(
ddog_telemetry_builder_with_endpoint_config_endpoint(
&mut builder,
&Endpoint::from_slice(&format!(
"file://{}",
f.path().as_os_str().to_str().unwrap()
)),
ffi::CharSlice::from(url.as_str()),
ffi::CharSlice::from(""),
0,
ffi::CharSlice::from(""),
false,
),
MaybeError::None
);
Expand Down Expand Up @@ -351,13 +357,15 @@ mod tests {
let mut builder = builder.assume_init();

let f = tempfile::NamedTempFile::new().unwrap();
let url = format!("file://{}", f.path().as_os_str().to_str().unwrap());
assert_eq!(
ddog_telemetry_builder_with_endpoint_config_endpoint(
&mut builder,
&Endpoint::from_slice(&format!(
"file://{}",
f.path().as_os_str().to_str().unwrap()
)),
ffi::CharSlice::from(url.as_str()),
ffi::CharSlice::from(""),
0,
ffi::CharSlice::from(""),
false,
),
MaybeError::None
);
Expand Down
4 changes: 1 addition & 3 deletions libdd-telemetry/examples/tm-metrics-worker-test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ fn main() -> Result<(), Box<dyn Error>> {
builder.config.telemetry_debug_logging_enabled = true;
builder
.config
.set_endpoint(libdd_common::Endpoint::from_slice(
"file://./tm-metrics-worker-test.output",
))
.set_endpoint_url("file://./tm-metrics-worker-test.output")
.unwrap();
builder.config.telemetry_heartbeat_interval = Duration::from_secs(1);
builder.config.debug_enabled = true;
Expand Down
12 changes: 4 additions & 8 deletions libdd-telemetry/examples/tm-send-sketch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@
// Datadog, Inc.

use std::{
borrow::Cow,
sync::atomic::{AtomicU64, Ordering},
time::SystemTime,
};

use http::{header::CONTENT_TYPE, Uri};
use libdd_common::Endpoint;
use http::header::CONTENT_TYPE;
use libdd_telemetry::{
build_host,
config::Config,
Expand Down Expand Up @@ -111,12 +109,10 @@ async fn async_main() {
let mut config = Config::from_env();
config.direct_submission_enabled = true;
config.debug_enabled = true;
let api_key = std::env::var("DD_API_KEY").unwrap();

@yannham yannham Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this existed before this PR, but we should avoid reading env vars in libdatadog. Not sure this one can really be avoided in a cross-platform way though...

@hoolioh hoolioh Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we shouldn't be changing behaviour in a library based on environment vars, it can cause problems for our consumers. Since the scope for the PR is to solve problems with the releases we can fix the env var issue in a subsequent PR?

config.set_endpoint_api_key(Some(&api_key)).unwrap();
config
.set_endpoint(Endpoint {
url: Uri::from_static("https://instrumentation-telemetry-intake.datad0g.com"),
api_key: Some(Cow::Owned(std::env::var("DD_API_KEY").unwrap())),
..Default::default()
})
.set_endpoint_url("https://instrumentation-telemetry-intake.datad0g.com")
.unwrap();
push_telemetry(&config, &req).await.unwrap();
}
5 changes: 1 addition & 4 deletions libdd-telemetry/examples/tm-worker-test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,7 @@ fn main() -> Result<(), Box<dyn Error>> {
builder.config = libdd_telemetry::config::Config::from_env();
builder
.config
.set_endpoint(libdd_common::Endpoint {
url: libdd_common::parse_uri("file://./tm-worker-test.output").unwrap(),
..Default::default()
})
.set_endpoint_url("file://./tm-worker-test.output")
.unwrap();
builder.config.telemetry_heartbeat_interval = Duration::from_secs(1);

Expand Down
Loading
Loading