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 2 commits 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
2 commits
Select commit Hold shift + click to select a range
d482c08
refactor(libdd-telemetry)!: avoid leaking libdd-common types in the p…
hoolioh Jun 23, 2026
dd135ff
chore: pack properties in a struct rather than having multiple methods
hoolioh Jun 26, 2026
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
11 changes: 9 additions & 2 deletions datadog-sidecar/src/service/sidecar_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ use libdd_capabilities_impl::NativeCapabilities;
use libdd_common::tag::Tag;
use libdd_dogstatsd_client::{new, DogStatsDActionOwned};
use libdd_remote_config::fetch::{ConfigInvariants, ConfigOptions, MultiTargetStats};
use libdd_telemetry::config::Config;
use libdd_telemetry::config::{Config, TelemetryEndpoint};
use libdd_tinybytes as tinybytes;
use libdd_trace_utils::tracer_header_tags::TracerHeaderTags;

Expand Down Expand Up @@ -709,7 +709,14 @@ impl SidecarInterface for ConnectionSidecarHandler {
libdd_telemetry::config::PROD_INTAKE_SUBDOMAIN,
&config.endpoint,
);
cfg.set_endpoint(endpoint).ok();
cfg.set_endpoint(TelemetryEndpoint {
url: Some(endpoint.url.to_string()),
api_key: endpoint.api_key.as_deref().map(str::to_owned),
test_token: endpoint.test_token.as_deref().map(str::to_owned),
timeout_ms: endpoint.timeout_ms,
use_system_resolver: endpoint.use_system_resolver,
})
.ok();
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
52 changes: 40 additions & 12 deletions libdd-crashtracker/src/crash_info/telemetry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,13 +197,23 @@ impl TelemetryCrashUploader {
// But do we want to support direct submission to the intake?

// ignore result because what are we going to do?
let _ = if endpoint.url.scheme_str() == Some("file") {
let telemetry_endpoint = if endpoint.url.scheme_str() == Some("file") {
let path = libdd_common::decode_uri_path_in_authority(&endpoint.url)
.context("file path is not valid")?;
cfg.set_host_from_url(&format!("file://{}.telemetry", path.display()))
libdd_telemetry::config::TelemetryEndpoint {
url: Some(format!("file://{}.telemetry", path.display())),
..Default::default()
}
} else {
cfg.set_endpoint(endpoint.clone())
libdd_telemetry::config::TelemetryEndpoint {
url: Some(endpoint.url.to_string()),
api_key: endpoint.api_key.as_deref().map(str::to_owned),
test_token: endpoint.test_token.as_deref().map(str::to_owned),
timeout_ms: endpoint.timeout_ms,
use_system_resolver: endpoint.use_system_resolver,
}
};
let _ = cfg.set_endpoint(telemetry_endpoint);
}

parse_tags!(
Expand Down Expand Up @@ -519,7 +529,10 @@ mod tests {
new_test_uploader_with_process_tags(seed, "entrypoint.name:cli,entrypoint.type:script");

t.cfg
.set_host_from_url(&format!("file://{}", output_filename.to_str().unwrap()))
.set_endpoint(libdd_telemetry::config::TelemetryEndpoint {
url: Some(format!("file://{}", output_filename.to_str().unwrap())),
..Default::default()
})
.unwrap();
let test_instance = super::CrashInfo::test_instance(seed);

Expand Down Expand Up @@ -585,7 +598,10 @@ mod tests {
let mut t = new_test_uploader(seed);

t.cfg
.set_host_from_url(&format!("file://{}", output_filename.to_str().unwrap()))
.set_endpoint(libdd_telemetry::config::TelemetryEndpoint {
url: Some(format!("file://{}", output_filename.to_str().unwrap())),
..Default::default()
})
.unwrap();

let sig_info = crate::SigInfo::test_instance(42);
Expand Down Expand Up @@ -661,7 +677,10 @@ mod tests {
let mut t = new_test_uploader(seed);

t.cfg
.set_host_from_url(&format!("file://{}", output_filename.to_str().unwrap()))
.set_endpoint(libdd_telemetry::config::TelemetryEndpoint {
url: Some(format!("file://{}", output_filename.to_str().unwrap())),
..Default::default()
})
.unwrap();

let sig_info = crate::SigInfo::test_instance(123);
Expand Down Expand Up @@ -771,10 +790,13 @@ mod tests {
let mut uploader = TelemetryCrashUploader::new(&metadata, &endpoint)?;
uploader
.cfg
.set_host_from_url(&format!(
"file://{}.telemetry",
output_filename.to_str().unwrap()
))
.set_endpoint(libdd_telemetry::config::TelemetryEndpoint {
url: Some(format!(
"file://{}.telemetry",
output_filename.to_str().unwrap()
)),
..Default::default()
})
.unwrap();

uploader.upload_crash_ping(&crash_ping).await?;
Expand Down Expand Up @@ -931,7 +953,10 @@ mod tests {

uploader
.cfg
.set_host_from_url(&format!("file://{}", output_filename.to_str().unwrap()))
.set_endpoint(libdd_telemetry::config::TelemetryEndpoint {
url: Some(format!("file://{}", output_filename.to_str().unwrap())),
..Default::default()
})
.unwrap();

let sig_info = crate::SigInfo::test_instance(150);
Expand Down Expand Up @@ -1006,7 +1031,10 @@ mod tests {
let mut uploader = new_test_uploader(7);
uploader
.cfg
.set_host_from_url(&format!("file://{}", output_filename.to_str().unwrap()))?;
.set_endpoint(libdd_telemetry::config::TelemetryEndpoint {
url: Some(format!("file://{}", output_filename.to_str().unwrap())),
..Default::default()
})?;

uploader
.upload_general_log(
Expand Down
5 changes: 4 additions & 1 deletion libdd-data-pipeline/src/telemetry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,10 @@ impl TelemetryClientBuilder {
pub fn set_url(mut self, url: &str) -> Self {
let _ = self
.config
.set_endpoint(libdd_common::Endpoint::from_slice(url));
.set_endpoint(libdd_telemetry::config::TelemetryEndpoint {
url: Some(url.to_owned()),
..Default::default()
});
self
}

Expand Down
117 changes: 101 additions & 16 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,10 +2,9 @@
// SPDX-License-Identifier: Apache-2.0

use ffi::slice::AsBytes;
use libdd_common::Endpoint;
use libdd_common_ffi as ffi;
use libdd_telemetry::{
data,
config, data,
worker::{TelemetryWorkerBuilder, TelemetryWorkerFlavor, TelemetryWorkerHandle},
};
use std::ptr::NonNull;
Expand Down Expand Up @@ -155,14 +154,78 @@ pub unsafe extern "C" fn ddog_telemetry_builder_run_metric_logs(
MaybeError::None
}

/// C-facing companion to [`libdd_telemetry::config::TelemetryEndpoint`]: the same
/// shape, but with caller-owned [`ffi::CharSlice`] strings instead of `String`s,
/// so `libdd_common::Endpoint` stays out of this crate's public API.
///
/// Empty `url`/`api_key`/`test_token` slices are treated as unset (leave the
/// existing value unchanged); a `timeout_ms` of 0 keeps the existing/default
/// timeout. `use_system_resolver` is always applied.
#[repr(C)]
pub struct TelemetryEndpoint<'a> {
pub url: ffi::CharSlice<'a>,
pub api_key: ffi::CharSlice<'a>,
pub timeout_ms: u64,
pub test_token: ffi::CharSlice<'a>,
pub use_system_resolver: bool,
}

impl TelemetryEndpoint<'_> {
/// Copies the caller-owned slices into an owned, `'static`
/// [`config::TelemetryEndpoint`]. The copy is mandatory: the slices point at
/// memory the C caller may free, so it cannot be borrowed into the config.
///
/// An inherent method rather than a `From` impl because the orphan rule
/// forbids implementing `From<Self> for` the foreign `config::TelemetryEndpoint`.
fn into_config(self) -> config::TelemetryEndpoint {
fn owned(slice: ffi::CharSlice) -> Option<String> {
(!slice.is_empty()).then(|| slice.to_utf8_lossy().into_owned())
}
config::TelemetryEndpoint {
url: owned(self.url),
api_key: owned(self.api_key),
test_token: owned(self.test_token),
timeout_ms: self.timeout_ms,
use_system_resolver: self.use_system_resolver,
}
}
}

/// Applies endpoint settings to the builder's telemetry config.
fn set_builder_endpoint(
telemetry_builder: &mut TelemetryWorkerBuilder,
endpoint: TelemetryEndpoint,
) -> ffi::MaybeError {
try_c!(telemetry_builder
.config
.set_endpoint(endpoint.into_config()));
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 +213 to +217

@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,
TelemetryEndpoint {
url,
api_key,
timeout_ms,
test_token,
use_system_resolver,
},
)
}
#[repr(C)]
#[allow(dead_code)]
Expand All @@ -172,38 +235,60 @@ 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,
TelemetryEndpoint {
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,
TelemetryEndpoint {
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
Loading