From 4990cc4d8b44b349ec7d53b4d785fdc4b4cb44f3 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 12 Dec 2022 13:53:54 -0700 Subject: [PATCH 1/9] Convert local root span id to u64, fix clippy lints Bump to v2.0.0 --- Cargo.lock | 12 +++--- ddcommon-ffi/Cargo.toml | 2 +- ddcommon-ffi/src/vec.rs | 2 +- ddcommon/Cargo.toml | 2 +- ddcommon/src/tag.rs | 6 +-- ddtelemetry-ffi/Cargo.toml | 6 +-- ddtelemetry/Cargo.toml | 2 +- ddtelemetry/benches/ipc.rs | 2 +- ddtelemetry/src/config.rs | 10 ++--- ddtelemetry/src/fork.rs | 2 +- ddtelemetry/src/ipc/platform/unix/mod.rs | 1 - ddtelemetry/src/metrics.rs | 4 +- profiling-ffi/Cargo.toml | 2 +- profiling-ffi/src/profiles.rs | 10 ++--- profiling/Cargo.toml | 2 +- profiling/src/exporter/config.rs | 2 +- profiling/src/profile/mod.rs | 52 +++++++++++++++++++----- 17 files changed, 73 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7de780369..e39f0df26 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -283,7 +283,7 @@ dependencies = [ [[package]] name = "datadog-profiling" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "bytes", @@ -313,7 +313,7 @@ dependencies = [ [[package]] name = "datadog-profiling-ffi" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "chrono", @@ -327,7 +327,7 @@ dependencies = [ [[package]] name = "ddcommon" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "futures", @@ -351,7 +351,7 @@ dependencies = [ [[package]] name = "ddcommon-ffi" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "ddcommon", @@ -359,7 +359,7 @@ dependencies = [ [[package]] name = "ddtelemetry" -version = "1.0.1" +version = "2.0.0" dependencies = [ "anyhow", "bytes", @@ -393,7 +393,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..89e154ed0 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}; 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 05c33773a..ba3cd6cb7 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..341e8be0f 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); } diff --git a/profiling/Cargo.toml b/profiling/Cargo.toml index 8f498021d..3acba9693 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..832f8286e 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, @@ -426,18 +426,19 @@ impl Profile { Some(profile) } - pub fn add_endpoint(&mut self, local_root_span_id: Cow, endpoint: Cow) { + /// Add the endpoint data to the endpoint mappings. The `endpoint` will be + /// interned, as will the strings "local root span id" and "trace endpoint". + pub fn add_endpoint(&mut self, local_root_span_id: u64, 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()); 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) { @@ -488,6 +489,35 @@ impl Profile { pub fn get_string(&self, id: i64) -> Option<&String> { self.strings.get_index(id as usize) } + + fn get_endpoint_for_label(&self, label: &Label) -> anyhow::Result<&i64> { + let local_root_span_id: u64 = if label.str == 0 { + /* Safety: the value is a u64, but pprof only has signed values, + * so we transmute it; the backend does the same. + */ + unsafe { std::intrinsics::transmute(label.num) } + } else { + /* Sad path: have to fetch the string from the table and convert + * it back into a u64. Clients should change to use numbers! + */ + let string = match self.strings.get_index(label.str as usize) { + None => anyhow::bail!( + "failed to retrieve index {} from the string table when looking for endpoint data", + label.str + ), + Some(string) => string, + }; + string.as_str().parse()? + }; + + match self.endpoints.mappings.get(&local_root_span_id) { + None => Err(anyhow::anyhow!( + "failed to retrieve endpoint information for local root span id {}", + local_root_span_id + )), + Some(endpoint_string_id) => Ok(endpoint_string_id), + } + } } impl From<&Profile> for pprof::Profile { @@ -509,14 +539,14 @@ 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 { + let endpoint = sample.labels.iter().find_map(|label| { + // todo: log errors in get_endpoint_for_label (should not happen) if label.key == profile.endpoints.local_root_span_id_label { - endpoint = profile.endpoints.mappings.get(&label.str); - break; + profile.get_endpoint_for_label(label).ok() + } else { + None } - } + }); if let Some(endpoint_value) = endpoint { sample.labels.push(pprof::Label { @@ -907,7 +937,7 @@ 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(); From 85cdec258bf53a68545fcad98c5c6a0997484932 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 12 Dec 2022 14:03:43 -0700 Subject: [PATCH 2/9] Fix path --- ddtelemetry/src/ipc/platform/unix/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtelemetry/src/ipc/platform/unix/mod.rs b/ddtelemetry/src/ipc/platform/unix/mod.rs index 89e154ed0..f801c3aa0 100644 --- a/ddtelemetry/src/ipc/platform/unix/mod.rs +++ b/ddtelemetry/src/ipc/platform/unix/mod.rs @@ -52,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| { From e3912615b04f6e0e30f0763cf503add54953cfe5 Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Tue, 20 Dec 2022 11:25:58 +0100 Subject: [PATCH 3/9] add test for root_span_id label as `str` or `i64` --- profiling/src/profile/mod.rs | 78 ++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/profiling/src/profile/mod.rs b/profiling/src/profile/mod.rs index 832f8286e..0d190135d 100644 --- a/profiling/src/profile/mod.rs +++ b/profiling/src/profile/mod.rs @@ -1064,4 +1064,82 @@ mod api_test { assert_eq!(endpoints_stats, expected_endpoints_stats); } + + #[test] + fn root_span_id_label_as_str_or_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 id2_label = api::Label { + key: "local root span id", + str: Some("11"), + num: 0, + 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("my endpoint")); + + profile.add_endpoint(11, Cow::from("my endpoint")); + + let serialized_profile: pprof::Profile = (&profile).into(); + + assert_eq!(serialized_profile.samples.len(), 2); + + // check for endpoint label in first sample + let s1 = serialized_profile.samples.get(0).expect("sample"); + + assert_eq!(s1.labels.len(), 2); + + let l1 = s1.labels.get(0).expect("label"); + + assert_eq!(l1.num, id_label.num); + + // check for endpoint label in second sample + let s2 = serialized_profile.samples.get(1).expect("sample"); + + assert_eq!(s2.labels.len(), 2); + + let l2 = s2.labels.get(0).expect("label"); + + assert_eq!( + serialized_profile + .string_table + .get(l2.str as usize) + .unwrap(), + "11" + ); + } } From 3eb917ab0fc8720a5f6361c531aa64b447ab82ad Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 4 Jan 2023 15:59:55 -0700 Subject: [PATCH 4/9] Test trace endpoint labels too --- profiling/src/profile/mod.rs | 73 +++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/profiling/src/profile/mod.rs b/profiling/src/profile/mod.rs index 0d190135d..e4a269123 100644 --- a/profiling/src/profile/mod.rs +++ b/profiling/src/profile/mod.rs @@ -1066,7 +1066,7 @@ mod api_test { } #[test] - fn root_span_id_label_as_str_or_i64() { + fn local_root_span_id_label_as_str_or_i64() { let sample_types = vec![ api::ValueType { r#type: "samples", @@ -1107,39 +1107,58 @@ mod api_test { }; profile.add(sample1).expect("add to success"); - profile.add(sample2).expect("add to success"); - profile.add_endpoint(10, Cow::from("my endpoint")); - - profile.add_endpoint(11, Cow::from("my endpoint")); + profile.add_endpoint(10, Cow::from("endpoint 10")); + profile.add_endpoint(11, Cow::from("endpoint 11")); let serialized_profile: pprof::Profile = (&profile).into(); - assert_eq!(serialized_profile.samples.len(), 2); - // check for endpoint label in first sample - let s1 = serialized_profile.samples.get(0).expect("sample"); - - assert_eq!(s1.labels.len(), 2); - - let l1 = s1.labels.get(0).expect("label"); - - assert_eq!(l1.num, id_label.num); - - // check for endpoint label in second sample - let s2 = serialized_profile.samples.get(1).expect("sample"); - - assert_eq!(s2.labels.len(), 2); - - let l2 = s2.labels.get(0).expect("label"); - - assert_eq!( + // 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 - .get(l2.str as usize) - .unwrap(), - "11" - ); + .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::str(local_root_span_id, locate_string("11")), + pprof::Label::str(trace_endpoint, locate_string("endpoint 11")), + ], + ]; + + // Finally, match the labels. + for (sample, labels) in serialized_profile + .samples + .iter() + .zip(expected_labels.iter()) + { + assert_eq!(sample.labels, labels); + } } } From 0a83855f36edadfccf801d0cdb7625595a3db5f5 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 4 Jan 2023 22:23:12 -0700 Subject: [PATCH 5/9] Ensure local root span id uses .num label value --- profiling/src/profile/mod.rs | 211 ++++++++++++++++++++--------------- 1 file changed, 123 insertions(+), 88 deletions(-) diff --git a/profiling/src/profile/mod.rs b/profiling/src/profile/mod.rs index e4a269123..c65958500 100644 --- a/profiling/src/profile/mod.rs +++ b/profiling/src/profile/mod.rs @@ -312,28 +312,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() { @@ -459,10 +472,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(|| { @@ -490,38 +503,37 @@ impl Profile { self.strings.get_index(id as usize) } - fn get_endpoint_for_label(&self, label: &Label) -> anyhow::Result<&i64> { - let local_root_span_id: u64 = if label.str == 0 { - /* Safety: the value is a u64, but pprof only has signed values, - * so we transmute it; the backend does the same. - */ - unsafe { std::intrinsics::transmute(label.num) } - } else { - /* Sad path: have to fetch the string from the table and convert - * it back into a u64. Clients should change to use numbers! - */ - let string = match self.strings.get_index(label.str as usize) { - None => anyhow::bail!( - "failed to retrieve index {} from the string table when looking for endpoint data", - label.str - ), - Some(string) => string, - }; - string.as_str().parse()? - }; - - match self.endpoints.mappings.get(&local_root_span_id) { - None => Err(anyhow::anyhow!( - "failed to retrieve endpoint information for local root span id {}", - local_root_span_id - )), - Some(endpoint_string_id) => Ok(endpoint_string_id), + /// 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(match self.endpoints.mappings.get(&local_root_span_id) { + None => None, + Some(endpoint_string_id) => Some(*endpoint_string_id), + }) } } -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), @@ -539,27 +551,28 @@ impl From<&Profile> for pprof::Profile { if !profile.endpoints.mappings.is_empty() { for sample in samples.iter_mut() { - let endpoint = sample.labels.iter().find_map(|label| { - // todo: log errors in get_endpoint_for_label (should not happen) - if label.key == profile.endpoints.local_root_span_id_label { - profile.get_endpoint_for_label(label).ok() - } else { - None + // 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 @@ -608,16 +621,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] @@ -774,7 +785,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); @@ -885,6 +896,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![ @@ -902,15 +938,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, }; @@ -939,7 +975,7 @@ mod api_test { 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); @@ -957,13 +993,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"); @@ -1066,7 +1096,7 @@ mod api_test { } #[test] - fn local_root_span_id_label_as_str_or_i64() { + fn local_root_span_id_label_as_i64() { let sample_types = vec![ api::ValueType { r#type: "samples", @@ -1089,8 +1119,8 @@ mod api_test { let id2_label = api::Label { key: "local root span id", - str: Some("11"), - num: 0, + str: None, + num: 11, num_unit: None, }; @@ -1112,7 +1142,7 @@ mod api_test { profile.add_endpoint(10, Cow::from("endpoint 10")); profile.add_endpoint(11, Cow::from("endpoint 11")); - let serialized_profile: pprof::Profile = (&profile).into(); + let serialized_profile = pprof::Profile::try_from(&profile).unwrap(); assert_eq!(serialized_profile.samples.len(), 2); // Find common label strings in the string table. @@ -1147,7 +1177,12 @@ mod api_test { pprof::Label::str(trace_endpoint, locate_string("endpoint 10")), ], [ - pprof::Label::str(local_root_span_id, locate_string("11")), + pprof::Label { + key: local_root_span_id, + str: 0, + num: 11, + num_unit: 0, + }, pprof::Label::str(trace_endpoint, locate_string("endpoint 11")), ], ]; From d6ab0bddc11a5094d2dc64e4028355049fd6d221 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 4 Jan 2023 22:29:57 -0700 Subject: [PATCH 6/9] Fix clippy::manual-map lint --- profiling/src/profile/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/profiling/src/profile/mod.rs b/profiling/src/profile/mod.rs index c65958500..bcde742a5 100644 --- a/profiling/src/profile/mod.rs +++ b/profiling/src/profile/mod.rs @@ -523,10 +523,11 @@ impl Profile { */ let local_root_span_id: u64 = unsafe { std::intrinsics::transmute(label.num) }; - Ok(match self.endpoints.mappings.get(&local_root_span_id) { - None => None, - Some(endpoint_string_id) => Some(*endpoint_string_id), - }) + Ok(self + .endpoints + .mappings + .get(&local_root_span_id) + .map(i64::clone)) } } From 344b3e353d0d0748c8ac53fafa7031c3190f1e5f Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 4 Jan 2023 22:31:40 -0700 Subject: [PATCH 7/9] Fix clippy::needless_question_mark lint --- profiling-ffi/src/profiles.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/profiling-ffi/src/profiles.rs b/profiling-ffi/src/profiles.rs index 341e8be0f..88a95da9c 100644 --- a/profiling-ffi/src/profiles.rs +++ b/profiling-ffi/src/profiles.rs @@ -449,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()), } From e1314557253d30112298f20ccd0900ed05f911f1 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 6 Jan 2023 14:09:23 -0700 Subject: [PATCH 8/9] Eagerly store certain strings This avoid doing runtime checking and simplifies some logic. --- profiling/src/profile/mod.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/profiling/src/profile/mod.rs b/profiling/src/profile/mod.rs index bcde742a5..4c9330cfb 100644 --- a/profiling/src/profile/mod.rs +++ b/profiling/src/profile/mod.rs @@ -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 @@ -439,14 +450,9 @@ impl Profile { Some(profile) } - /// Add the endpoint data to the endpoint mappings. The `endpoint` will be - /// interned, as will the strings "local root span id" and "trace endpoint". + /// 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) { - 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_endpoint = self.intern(endpoint.as_ref()); self.endpoints @@ -640,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] From 1d3096ec3993d7adbb13acf057fa2ca7013cf5eb Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 9 Jan 2023 17:38:37 -0700 Subject: [PATCH 9/9] Test span ids larger than i64::MAX --- profiling/src/profile/mod.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/profiling/src/profile/mod.rs b/profiling/src/profile/mod.rs index 4c9330cfb..a662a61d1 100644 --- a/profiling/src/profile/mod.rs +++ b/profiling/src/profile/mod.rs @@ -1121,10 +1121,14 @@ mod api_test { 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: 11, + num: large_num, num_unit: None, }; @@ -1144,7 +1148,7 @@ mod api_test { profile.add(sample2).expect("add to success"); profile.add_endpoint(10, Cow::from("endpoint 10")); - profile.add_endpoint(11, Cow::from("endpoint 11")); + 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); @@ -1184,10 +1188,10 @@ mod api_test { pprof::Label { key: local_root_span_id, str: 0, - num: 11, + num: large_num, num_unit: 0, }, - pprof::Label::str(trace_endpoint, locate_string("endpoint 11")), + pprof::Label::str(trace_endpoint, locate_string("large endpoint")), ], ];