diff --git a/Cargo.lock b/Cargo.lock index 65fd72a44..4b96ede0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -282,7 +282,7 @@ dependencies = [ [[package]] name = "datadog-profiling" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "bytes", @@ -312,7 +312,7 @@ dependencies = [ [[package]] name = "datadog-profiling-ffi" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "chrono", @@ -326,7 +326,7 @@ dependencies = [ [[package]] name = "ddcommon" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "futures", @@ -350,7 +350,7 @@ dependencies = [ [[package]] name = "ddcommon-ffi" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "ddcommon", @@ -358,7 +358,7 @@ dependencies = [ [[package]] name = "ddtelemetry" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "bytes", @@ -392,7 +392,7 @@ dependencies = [ [[package]] name = "ddtelemetry-ffi" -version = "1.0.1" +version = "2.0.0" dependencies = [ "ddcommon-ffi", "ddtelemetry", diff --git a/ddcommon-ffi/Cargo.toml b/ddcommon-ffi/Cargo.toml index 0210c22eb..0a35f58be 100644 --- a/ddcommon-ffi/Cargo.toml +++ b/ddcommon-ffi/Cargo.toml @@ -3,7 +3,7 @@ [package] name = "ddcommon-ffi" -version = "1.0.1" +version = "2.0.0" edition = "2021" license = "Apache-2.0" diff --git a/ddcommon-ffi/src/vec.rs b/ddcommon-ffi/src/vec.rs index 8c9ad1248..34435fc75 100644 --- a/ddcommon-ffi/src/vec.rs +++ b/ddcommon-ffi/src/vec.rs @@ -59,7 +59,7 @@ impl From> for Vec { impl From for Vec { fn from(err: anyhow::Error) -> Self { let mut vec = vec![]; - write!(vec, "{}", err).expect("write to vec to always succeed"); + write!(vec, "{err}").expect("write to vec to always succeed"); Self::from(vec) } } diff --git a/ddcommon/Cargo.toml b/ddcommon/Cargo.toml index d88ade4c6..cf5c9d999 100644 --- a/ddcommon/Cargo.toml +++ b/ddcommon/Cargo.toml @@ -5,7 +5,7 @@ edition = "2021" license = "Apache-2.0" name = "ddcommon" -version = "1.0.1" +version = "2.0.0" [lib] crate-type = ["lib"] diff --git a/ddcommon/src/tag.rs b/ddcommon/src/tag.rs index 58eaf02d1..344905755 100644 --- a/ddcommon/src/tag.rs +++ b/ddcommon/src/tag.rs @@ -55,10 +55,10 @@ impl Tag { let mut chars = chunk.chars(); if chars.next() == Some(':') { - return Err(format!("tag '{}' begins with a colon", chunk).into()); + return Err(format!("tag '{chunk}' begins with a colon").into()); } if chars.last() == Some(':') { - return Err(format!("tag '{}' ends with a colon", chunk).into()); + return Err(format!("tag '{chunk}' ends with a colon").into()); } Ok(Tag { @@ -74,7 +74,7 @@ impl Tag { let key = key.as_ref(); let value = value.as_ref(); - Tag::from_value(format!("{}:{}", key, value)) + Tag::from_value(format!("{key}:{value}")) } } diff --git a/ddtelemetry-ffi/Cargo.toml b/ddtelemetry-ffi/Cargo.toml index 4d4c07aaf..ecac14ccd 100644 --- a/ddtelemetry-ffi/Cargo.toml +++ b/ddtelemetry-ffi/Cargo.toml @@ -3,14 +3,14 @@ [package] name = "ddtelemetry-ffi" -version = "1.0.1" +version = "2.0.0" edition = "2021" [lib] crate-type = ["lib", "staticlib", "cdylib"] [dependencies] -ddtelemetry = { path = "../ddtelemetry", version = "1.0.1" } -ddcommon-ffi = { path = "../ddcommon-ffi", version = "1.0.1" } +ddtelemetry = { path = "../ddtelemetry" } +ddcommon-ffi = { path = "../ddcommon-ffi" } paste = "1" libc = "0.2" diff --git a/ddtelemetry/Cargo.toml b/ddtelemetry/Cargo.toml index ed6015a5e..a0729d354 100644 --- a/ddtelemetry/Cargo.toml +++ b/ddtelemetry/Cargo.toml @@ -2,7 +2,7 @@ edition = "2021" license = "Apache 2.0" name = "ddtelemetry" -version = "1.0.1" +version = "2.0.0" [dependencies] anyhow = {version = "1.0"} diff --git a/ddtelemetry/benches/ipc.rs b/ddtelemetry/benches/ipc.rs index 28ef8f34b..294ff5e30 100644 --- a/ddtelemetry/benches/ipc.rs +++ b/ddtelemetry/benches/ipc.rs @@ -47,7 +47,7 @@ fn criterion_benchmark(c: &mut Criterion) { _ => panic!("shouldn't happen"), }; - println!("Total requests handled: {}", requests_received); + println!("Total requests handled: {requests_received}"); drop(transport); worker.join().unwrap(); diff --git a/ddtelemetry/src/config.rs b/ddtelemetry/src/config.rs index 2152dcc81..c548753af 100644 --- a/ddtelemetry/src/config.rs +++ b/ddtelemetry/src/config.rs @@ -50,7 +50,7 @@ impl FromEnv { let agent_host = env::var(DD_AGENT_HOST).unwrap_or_else(|_| String::from(DEFAULT_AGENT_HOST)); - format!("http://{}:{}", agent_host, agent_port) + format!("http://{agent_host}:{agent_port}") } fn get_intake_base_url() -> String { @@ -63,9 +63,9 @@ impl FromEnv { if let Ok(dd_site) = env::var(DD_SITE) { if dd_site.is_empty() { - format!("{}.{}", PROD_INTAKE_FORMAT_PREFIX, DEFAULT_DD_SITE) + format!("{PROD_INTAKE_FORMAT_PREFIX}.{DEFAULT_DD_SITE}") } else { - format!("{}.{}", PROD_INTAKE_FORMAT_PREFIX, dd_site) + format!("{PROD_INTAKE_FORMAT_PREFIX}.{dd_site}") } } else { String::from(STAGING_INTAKE) @@ -77,9 +77,9 @@ impl FromEnv { let telemetry_url = if api_key.is_some() { let telemetry_intake_base_url = Self::get_intake_base_url(); - format!("{}{}", telemetry_intake_base_url, DIRECT_TELEMETRY_URL_PATH) + format!("{telemetry_intake_base_url}{DIRECT_TELEMETRY_URL_PATH}") } else { - format!("{}{}", &agent_url, AGENT_TELEMETRY_URL_PATH) + format!("{}{AGENT_TELEMETRY_URL_PATH}", &agent_url) }; let telemetry_uri = Uri::from_str(&telemetry_url).ok()?; diff --git a/ddtelemetry/src/fork.rs b/ddtelemetry/src/fork.rs index 156b21a14..c6d41b97d 100644 --- a/ddtelemetry/src/fork.rs +++ b/ddtelemetry/src/fork.rs @@ -142,7 +142,7 @@ pub mod tests { sock_a.read_to_string(&mut out).unwrap(); assert_child_exit!(pid); - assert_eq!(format!("child-{}", pid), out); + assert_eq!(format!("child-{pid}"), out); } #[test] diff --git a/ddtelemetry/src/ipc/platform/unix/mod.rs b/ddtelemetry/src/ipc/platform/unix/mod.rs index b76f58607..f801c3aa0 100644 --- a/ddtelemetry/src/ipc/platform/unix/mod.rs +++ b/ddtelemetry/src/ipc/platform/unix/mod.rs @@ -23,7 +23,6 @@ mod tests { fs::File, io::{self, Read, Seek, Write}, os::unix::prelude::{AsRawFd, RawFd}, - path::Path, }; use crate::ipc::platform::{metadata::ChannelMetadata, unix::message::MAX_FDS}; @@ -53,7 +52,7 @@ mod tests { .map(|p| format!("{}", p)) .unwrap_or_else(|| "self".into()); - let fds_path = Path::new("/proc").join(proc).join("fd"); + let fds_path = std::path::Path::new("/proc").join(proc).join("fd"); let fds = std::fs::read_dir(fds_path)? .filter_map(|r| r.ok()) .filter_map(|r| { diff --git a/ddtelemetry/src/metrics.rs b/ddtelemetry/src/metrics.rs index 261c057ca..61ac5ffbf 100644 --- a/ddtelemetry/src/metrics.rs +++ b/ddtelemetry/src/metrics.rs @@ -212,14 +212,14 @@ mod test { for (i, &a) in assertions.iter().enumerate() { if a(e) { if used[i] { - panic!("Assertion {} has been used multiple times", i); + panic!("Assertion {i} has been used multiple times"); } found = true; break; } } if !found { - panic!("No assertion found for elem {:?}", e) + panic!("No assertion found for elem {e:?}") } } } diff --git a/profiling-ffi/Cargo.toml b/profiling-ffi/Cargo.toml index 82c61ea81..6864d6c80 100644 --- a/profiling-ffi/Cargo.toml +++ b/profiling-ffi/Cargo.toml @@ -3,7 +3,7 @@ [package] name = "datadog-profiling-ffi" -version = "1.0.1" +version = "2.0.0" edition = "2021" license = "Apache-2.0" diff --git a/profiling-ffi/src/profiles.rs b/profiling-ffi/src/profiles.rs index 9c20f595b..88a95da9c 100644 --- a/profiling-ffi/src/profiles.rs +++ b/profiling-ffi/src/profiles.rs @@ -353,7 +353,7 @@ pub extern "C" fn ddog_prof_Profile_add( /// /// # Arguments /// * `profile` - a reference to the profile that will contain the samples. -/// * `local_root_span_id` - the value of the local root span id label to look for. +/// * `local_root_span_id` /// * `endpoint` - the value of the endpoint label to add for matching samples. /// /// # Safety @@ -361,14 +361,12 @@ pub extern "C" fn ddog_prof_Profile_add( /// module. /// This call is _NOT_ thread-safe. #[no_mangle] -pub unsafe extern "C" fn ddog_prof_Profile_set_endpoint<'a>( +pub unsafe extern "C" fn ddog_prof_Profile_set_endpoint( profile: &mut datadog_profiling::profile::Profile, - local_root_span_id: CharSlice<'a>, - endpoint: CharSlice<'a>, + local_root_span_id: u64, + endpoint: CharSlice, ) { - let local_root_span_id = local_root_span_id.to_utf8_lossy(); let endpoint = endpoint.to_utf8_lossy(); - profile.add_endpoint(local_root_span_id, endpoint); } @@ -451,9 +449,7 @@ pub unsafe extern "C" fn ddog_prof_Profile_serialize( Some(x) if *x < 0 => None, Some(x) => Some(Duration::from_nanos((*x) as u64)), }; - match || -> anyhow::Result { - Ok(profile.serialize(end_time, duration)?) - }() { + match profile.serialize(end_time, duration) { Ok(ok) => SerializeResult::Ok(ok.into()), Err(err) => SerializeResult::Err(err.into()), } diff --git a/profiling/Cargo.toml b/profiling/Cargo.toml index 9690daada..8231c7e8b 100644 --- a/profiling/Cargo.toml +++ b/profiling/Cargo.toml @@ -3,7 +3,7 @@ [package] name = "datadog-profiling" -version = "1.0.1" +version = "2.0.0" edition = "2021" license = "Apache-2.0" diff --git a/profiling/src/exporter/config.rs b/profiling/src/exporter/config.rs index 5a869ad88..cdf5fe607 100644 --- a/profiling/src/exporter/config.rs +++ b/profiling/src/exporter/config.rs @@ -23,7 +23,7 @@ pub fn agent(base_url: Uri) -> anyhow::Result { Some(pq) => { let path = pq.path(); let path = path.strip_suffix('/').unwrap_or(path); - Some(format!("{}/profiling/v1/input", path).parse()?) + Some(format!("{path}/profiling/v1/input").parse()?) } }; parts.path_and_query = p_q; diff --git a/profiling/src/profile/mod.rs b/profiling/src/profile/mod.rs index 381f90695..a662a61d1 100644 --- a/profiling/src/profile/mod.rs +++ b/profiling/src/profile/mod.rs @@ -91,7 +91,7 @@ pub struct Profile { } pub struct Endpoints { - mappings: FxIndexMap, + mappings: FxIndexMap, local_root_span_id_label: i64, endpoint_label: i64, stats: ProfiledEndpointsStats, @@ -234,8 +234,12 @@ impl Default for Endpoints { } impl Profile { - /// Creates a profile with `start_time`. Initializes the string table to - /// include the empty string. All other fields are default. + /// Creates a profile with `start_time`. + /// Initializes the string table to hold: + /// - "" (the empty string) + /// - "local root span id" + /// - "trace endpoint" + /// All other fields are default. pub fn new(start_time: SystemTime) -> Self { /* Do not use Profile's default() impl here or it will cause a stack * overflow, since that default impl calls this method. @@ -253,9 +257,16 @@ impl Profile { }; profile.intern(""); + profile.endpoints.local_root_span_id_label = profile.intern("local root span id"); + profile.endpoints.endpoint_label = profile.intern("trace endpoint"); profile } + #[cfg(test)] + fn interned_strings_count(&self) -> usize { + self.strings.len() + } + /// Interns the `str` as a string, returning the id in the string table. fn intern(&mut self, str: &str) -> i64 { // strings are special because the empty string is actually allowed at @@ -312,28 +323,41 @@ impl Profile { PProfId(index + 1) } - pub fn add(&mut self, sample: api::Sample) -> Result { - if sample.values.len() != self.sample_types.len() { - return Ok(PProfId(0)); - } + pub fn add(&mut self, sample: api::Sample) -> anyhow::Result { + anyhow::ensure!( + sample.values.len() == self.sample_types.len(), + "expected {} sample types, but sample had {} sample types", + sample.values.len(), + self.sample_types.len(), + ); let values = sample.values.clone(); - let labels = sample - .labels - .iter() - .map(|label| { - let key = self.intern(label.key); - let str = label.str.map(|s| self.intern(s)).unwrap_or(0); - let num_unit = label.num_unit.map(|s| self.intern(s)).unwrap_or(0); - - Label { - key, - str, - num: label.num, - num_unit, - } - }) - .collect(); + let mut labels = Vec::with_capacity(sample.labels.len()); + for label in sample.labels.iter() { + let key = self.intern(label.key); + let str = label.str.map(|s| self.intern(s)).unwrap_or(0); + let num_unit = label.num_unit.map(|s| self.intern(s)).unwrap_or(0); + + if key == self.endpoints.local_root_span_id_label { + // Panic: if the label.str isn't 0, then str must have been provided. + anyhow::ensure!( + str == 0, + "the label \"local root span id\" must be sent as a number, not string {}", + label.str.unwrap() + ); + anyhow::ensure!( + label.num != 0, + "the label \"local root span id\" must not be 0" + ) + } + + labels.push(Label { + key, + str, + num: label.num, + num_unit, + }); + } let mut locations: Vec = Vec::with_capacity(sample.locations.len()); for location in sample.locations.iter() { @@ -426,18 +450,14 @@ impl Profile { Some(profile) } - pub fn add_endpoint(&mut self, local_root_span_id: Cow, endpoint: Cow) { - if self.endpoints.mappings.is_empty() { - self.endpoints.local_root_span_id_label = self.intern("local root span id"); - self.endpoints.endpoint_label = self.intern("trace endpoint"); - } - - let interned_span_id = self.intern(local_root_span_id.as_ref()); + /// Add the endpoint data to the endpoint mappings. + /// The `endpoint` string will be interned. + pub fn add_endpoint(&mut self, local_root_span_id: u64, endpoint: Cow) { let interned_endpoint = self.intern(endpoint.as_ref()); self.endpoints .mappings - .insert(interned_span_id, interned_endpoint); + .insert(local_root_span_id, interned_endpoint); } pub fn add_endpoint_count(&mut self, endpoint: Cow, value: i64) { @@ -458,10 +478,10 @@ impl Profile { &self, end_time: Option, duration: Option, - ) -> Result { + ) -> anyhow::Result { let end = end_time.unwrap_or_else(SystemTime::now); let start = self.start_time; - let mut profile: pprof::Profile = self.into(); + let mut profile: pprof::Profile = self.try_into()?; profile.duration_nanos = duration .unwrap_or_else(|| { @@ -488,10 +508,39 @@ impl Profile { pub fn get_string(&self, id: i64) -> Option<&String> { self.strings.get_index(id as usize) } + + /// Fetches the endpoint information for the label. There may be errors, + /// but there may also be no endpoint information for a given endpoint, or + /// it may not be an local root span id label. Hence, the return type of + /// Result, _>. + fn get_endpoint_for_label(&self, label: &Label) -> anyhow::Result> { + if label.key != self.endpoints.local_root_span_id_label { + return Ok(None); + } + + anyhow::ensure!( + label.str == 0, + "the local root span id label value must be sent as a number, not a string, given string id {}", + label.str + ); + + /* Safety: the value is a u64, but pprof only has signed values, so we + * transmute it; the backend does the same. + */ + let local_root_span_id: u64 = unsafe { std::intrinsics::transmute(label.num) }; + + Ok(self + .endpoints + .mappings + .get(&local_root_span_id) + .map(i64::clone)) + } } -impl From<&Profile> for pprof::Profile { - fn from(profile: &Profile) -> Self { +impl TryFrom<&Profile> for pprof::Profile { + type Error = anyhow::Error; + + fn try_from(profile: &Profile) -> anyhow::Result { let (period, period_type) = match profile.period { Some(tuple) => (tuple.0, Some(tuple.1)), None => (0, None), @@ -509,27 +558,28 @@ impl From<&Profile> for pprof::Profile { if !profile.endpoints.mappings.is_empty() { for sample in samples.iter_mut() { - let mut endpoint: Option<&i64> = None; - - for label in &sample.labels { - if label.key == profile.endpoints.local_root_span_id_label { - endpoint = profile.endpoints.mappings.get(&label.str); - break; + // There _should_ only be one local root span id label, but it's not enforced. + let lrsi_labels: Vec<_> = sample + .labels + .iter() + .filter(|label| label.key == profile.endpoints.local_root_span_id_label) + .map(Clone::clone) // Need to clone to break borrows + .collect(); + + for label in lrsi_labels { + if let Some(endpoint_value_id) = profile.get_endpoint_for_label(&label)? { + sample.labels.push(pprof::Label { + key: profile.endpoints.endpoint_label, + str: endpoint_value_id, + num: 0, + num_unit: 0, + }); } } - - if let Some(endpoint_value) = endpoint { - sample.labels.push(pprof::Label { - key: profile.endpoints.endpoint_label, - str: *endpoint_value, - num: 0, - num_unit: 0, - }); - } } } - pprof::Profile { + Ok(pprof::Profile { sample_types: profile.sample_types.clone(), samples, mappings: profile @@ -578,16 +628,14 @@ impl From<&Profile> for pprof::Profile { period, period_type, ..Default::default() - } + }) } } #[cfg(test)] mod api_test { - use crate::profile::{ - api, pprof, profiled_endpoints::ProfiledEndpointsStats, PProfId, Profile, ValueType, - }; + use super::*; use std::{borrow::Cow, collections::HashMap}; #[test] @@ -598,17 +646,14 @@ mod api_test { }]; let mut profiles = Profile::builder().sample_types(sample_types).build(); - /* There have been 3 strings: "", "samples", and "count". Since the interning index starts at - * zero, this means the next string will be 3. - */ - const EXPECTED_ID: i64 = 3; + let expected_id: i64 = profiles.interned_strings_count().try_into().unwrap(); let string = "a"; let id1 = profiles.intern(string); let id2 = profiles.intern(string); assert_eq!(id1, id2); - assert_eq!(id1, EXPECTED_ID); + assert_eq!(id1, expected_id); } #[test] @@ -744,7 +789,7 @@ mod api_test { #[test] fn impl_from_profile_for_pprof_profile() { let locations = provide_distinct_locations(); - let profile = pprof::Profile::from(&locations); + let profile = pprof::Profile::try_from(&locations).unwrap(); assert_eq!(profile.samples.len(), 2); assert_eq!(profile.mappings.len(), 1); @@ -855,6 +900,31 @@ mod api_test { ); } + #[test] + fn adding_local_root_span_id_with_string_value_fails() { + let sample_types = vec![api::ValueType { + r#type: "wall-time", + unit: "nanoseconds", + }]; + + let mut profile: Profile = Profile::builder().sample_types(sample_types).build(); + + let id_label = api::Label { + key: "local root span id", + str: Some("10"), // bad value, should use .num instead for local root span id + num: 0, + num_unit: None, + }; + + let sample = api::Sample { + locations: vec![], + values: vec![1, 10000], + labels: vec![id_label], + }; + + assert!(profile.add(sample).is_err()); + } + #[test] fn lazy_endpoints() { let sample_types = vec![ @@ -872,15 +942,15 @@ mod api_test { let id_label = api::Label { key: "local root span id", - str: Some("10"), - num: 0, + str: None, + num: 10, num_unit: None, }; let id2_label = api::Label { key: "local root span id", - str: Some("11"), - num: 0, + str: None, + num: 11, num_unit: None, }; @@ -907,9 +977,9 @@ mod api_test { profile.add(sample2).expect("add to success"); - profile.add_endpoint(Cow::from("10"), Cow::from("my endpoint")); + profile.add_endpoint(10, Cow::from("my endpoint")); - let serialized_profile: pprof::Profile = (&profile).into(); + let serialized_profile = pprof::Profile::try_from(&profile).unwrap(); assert_eq!(serialized_profile.samples.len(), 2); @@ -927,13 +997,7 @@ mod api_test { .unwrap(), "local root span id" ); - assert_eq!( - serialized_profile - .string_table - .get(l1.str as usize) - .unwrap(), - "10" - ); + assert_eq!(l1.num, 10); let l2 = s1.labels.get(1).expect("label"); @@ -1034,4 +1098,110 @@ mod api_test { assert_eq!(endpoints_stats, expected_endpoints_stats); } + + #[test] + fn local_root_span_id_label_as_i64() { + let sample_types = vec![ + api::ValueType { + r#type: "samples", + unit: "count", + }, + api::ValueType { + r#type: "wall-time", + unit: "nanoseconds", + }, + ]; + + let mut profile: Profile = Profile::builder().sample_types(sample_types).build(); + + let id_label = api::Label { + key: "local root span id", + str: None, + num: 10, + num_unit: None, + }; + + let large_span_id = u64::MAX; + // Safety: a u64 can fit into an i64, and we're testing that it's not mis-handled. + let large_num: i64 = unsafe { std::intrinsics::transmute(large_span_id) }; + + let id2_label = api::Label { + key: "local root span id", + str: None, + num: large_num, + num_unit: None, + }; + + let sample1 = api::Sample { + locations: vec![], + values: vec![1, 10000], + labels: vec![id_label], + }; + + let sample2 = api::Sample { + locations: vec![], + values: vec![1, 10000], + labels: vec![id2_label], + }; + + profile.add(sample1).expect("add to success"); + profile.add(sample2).expect("add to success"); + + profile.add_endpoint(10, Cow::from("endpoint 10")); + profile.add_endpoint(large_span_id, Cow::from("large endpoint")); + + let serialized_profile = pprof::Profile::try_from(&profile).unwrap(); + assert_eq!(serialized_profile.samples.len(), 2); + + // Find common label strings in the string table. + let locate_string = |string: &str| -> i64 { + // The table is supposed to be unique, so we shouldn't have to worry about duplicates. + serialized_profile + .string_table + .iter() + .enumerate() + .find_map(|(offset, str)| { + if str == string { + Some(offset as i64) + } else { + None + } + }) + .unwrap() + }; + + let local_root_span_id = locate_string("local root span id"); + let trace_endpoint = locate_string("trace endpoint"); + + // Set up the expected labels per sample + let expected_labels = [ + [ + pprof::Label { + key: local_root_span_id, + str: 0, + num: 10, + num_unit: 0, + }, + pprof::Label::str(trace_endpoint, locate_string("endpoint 10")), + ], + [ + pprof::Label { + key: local_root_span_id, + str: 0, + num: large_num, + num_unit: 0, + }, + pprof::Label::str(trace_endpoint, locate_string("large endpoint")), + ], + ]; + + // Finally, match the labels. + for (sample, labels) in serialized_profile + .samples + .iter() + .zip(expected_labels.iter()) + { + assert_eq!(sample.labels, labels); + } + } }