From bd8abfc920f6c91ec74ee8d0a50d019021ac7839 Mon Sep 17 00:00:00 2001 From: Daniel Schwartz-Narbone Date: Wed, 14 Jan 2026 13:33:28 -0500 Subject: [PATCH] feat(profiling)!: take sample types by enum --- datadog-profiling-replayer/src/replayer.rs | 28 +- examples/cxx/profiling.cpp | 12 +- examples/ffi/exporter.cpp | 12 +- examples/ffi/profile_intern.cpp | 11 +- examples/ffi/profiles.c | 11 +- libdd-profiling-ffi/src/profiles/datatypes.rs | 90 ++----- libdd-profiling/README.md | 8 +- libdd-profiling/benches/add_samples.rs | 4 +- libdd-profiling/examples/profiles.rs | 5 +- libdd-profiling/src/{api.rs => api/mod.rs} | 9 +- libdd-profiling/src/api/sample_type.rs | 255 ++++++++++++++++++ libdd-profiling/src/cxx.rs | 155 ++++++++--- libdd-profiling/src/internal/mod.rs | 1 - .../src/internal/owned_types/mod.rs | 51 ---- .../src/internal/profile/fuzz_tests.rs | 37 +-- libdd-profiling/src/internal/profile/mod.rs | 195 ++++---------- 16 files changed, 524 insertions(+), 360 deletions(-) rename libdd-profiling/src/{api.rs => api/mod.rs} (99%) create mode 100644 libdd-profiling/src/api/sample_type.rs delete mode 100644 libdd-profiling/src/internal/owned_types/mod.rs diff --git a/datadog-profiling-replayer/src/replayer.rs b/datadog-profiling-replayer/src/replayer.rs index ae3e2037d5..6908645ee7 100644 --- a/datadog-profiling-replayer/src/replayer.rs +++ b/datadog-profiling-replayer/src/replayer.rs @@ -24,8 +24,8 @@ pub struct Replayer<'pprof> { pub start_time: SystemTime, pub duration: Duration, pub end_time: SystemTime, // start_time + duration - pub sample_types: Vec>, - pub period: Option>, + pub sample_types: Vec, + pub period: Option, pub endpoints: Vec<(u64, &'pprof str)>, pub samples: Vec<(Option, api::Sample<'pprof>)>, } @@ -57,29 +57,27 @@ impl<'pprof> Replayer<'pprof> { fn sample_types<'a>( profile_index: &'a ProfileIndex<'pprof>, - ) -> anyhow::Result>> { + ) -> anyhow::Result> { let mut sample_types = Vec::with_capacity(profile_index.pprof.sample_types.len()); for sample_type in profile_index.pprof.sample_types.iter() { - sample_types.push(api::ValueType::new( - profile_index.get_string(sample_type.r#type)?, - profile_index.get_string(sample_type.unit)?, - )) + let type_str = profile_index.get_string(sample_type.r#type)?; + let unit = profile_index.get_string(sample_type.unit)?; + let vt = api::ValueType::new(type_str, unit); + sample_types.push(vt.try_into()?); } Ok(sample_types) } - fn period<'a>( - profile_index: &'a ProfileIndex<'pprof>, - ) -> anyhow::Result>> { + fn period<'a>(profile_index: &'a ProfileIndex<'pprof>) -> anyhow::Result> { let value = profile_index.pprof.period; match profile_index.pprof.period_type { Some(period_type) => { - let r#type = api::ValueType::new( - profile_index.get_string(period_type.r#type)?, - profile_index.get_string(period_type.unit)?, - ); - Ok(Some(api::Period { r#type, value })) + let type_str = profile_index.get_string(period_type.r#type)?; + let unit = profile_index.get_string(period_type.unit)?; + let vt = api::ValueType::new(type_str, unit); + let sample_type = vt.try_into()?; + Ok(Some(api::Period { sample_type, value })) } None => Ok(None), } diff --git a/examples/cxx/profiling.cpp b/examples/cxx/profiling.cpp index 29acbf73fe..6d1ee6a050 100644 --- a/examples/cxx/profiling.cpp +++ b/examples/cxx/profiling.cpp @@ -16,17 +16,15 @@ int main() { std::cout << "=== Datadog Profiling CXX Bindings Example ===" << std::endl; std::cout << "\nCreating Profile..." << std::endl; - ValueType wall_time{ - .type_ = "wall-time", - .unit = "nanoseconds" - }; - Period period{ - .value_type = wall_time, + .value_type = SampleType::WallTime, .value = 60 }; - auto profile = Profile::create({wall_time}, period); + // Create profile with predefined sample types + // Note: ExperimentalCount, ExperimentalNanoseconds, and ExperimentalBytes + // are available for custom profiling metrics + auto profile = Profile::create({SampleType::WallTime}, period); std::cout << "✅ Profile created" << std::endl; std::cout << "Adding upscaling rules..." << std::endl; diff --git a/examples/ffi/exporter.cpp b/examples/ffi/exporter.cpp index 091515ae26..5cfdb19b9e 100644 --- a/examples/ffi/exporter.cpp +++ b/examples/ffi/exporter.cpp @@ -31,13 +31,13 @@ int main(int argc, char *argv[]) { const auto service = argv[1]; - const ddog_prof_ValueType wall_time = { - .type_ = DDOG_CHARSLICE_C_BARE("wall-time"), - .unit = DDOG_CHARSLICE_C_BARE("nanoseconds"), + // Use the SampleType enum instead of ValueType struct + const ddog_prof_SampleType wall_time = DDOG_PROF_SAMPLE_TYPE_WALL_TIME; + const ddog_prof_Slice_SampleType sample_types = {&wall_time, 1}; + const ddog_prof_Period period = { + .sample_type = wall_time, + .value = 60, }; - - const ddog_prof_Slice_ValueType sample_types = {&wall_time, 1}; - const ddog_prof_Period period = {wall_time, 60}; ddog_prof_Profile_NewResult profile_new_result = ddog_prof_Profile_new(sample_types, &period); if (profile_new_result.tag != DDOG_PROF_PROFILE_NEW_RESULT_OK) { print_error("Failed to make new profile: ", profile_new_result.err); diff --git a/examples/ffi/profile_intern.cpp b/examples/ffi/profile_intern.cpp index 7a8227bda5..f0dea4d7a5 100644 --- a/examples/ffi/profile_intern.cpp +++ b/examples/ffi/profile_intern.cpp @@ -69,12 +69,13 @@ void wait_for_user(std::string s) { } int main(void) { - const ddog_prof_ValueType wall_time = { - .type_ = to_slice_c_char("wall-time"), - .unit = to_slice_c_char("nanoseconds"), + // Use the SampleType enum instead of ValueType struct + const ddog_prof_SampleType wall_time = DDOG_PROF_SAMPLE_TYPE_WALL_TIME; + const ddog_prof_Slice_SampleType sample_types = {&wall_time, 1}; + const ddog_prof_Period period = { + .sample_type = wall_time, + .value = 60, }; - const ddog_prof_Slice_ValueType sample_types = {&wall_time, 1}; - const ddog_prof_Period period = {wall_time, 60}; ddog_prof_Profile_NewResult new_result = ddog_prof_Profile_new(sample_types, &period); if (new_result.tag != DDOG_PROF_PROFILE_NEW_RESULT_OK) { diff --git a/examples/ffi/profiles.c b/examples/ffi/profiles.c index 9c5798d483..a4b0eb2800 100644 --- a/examples/ffi/profiles.c +++ b/examples/ffi/profiles.c @@ -10,12 +10,13 @@ #define NUM_SAMPLES 5000000 int main(void) { - const ddog_prof_ValueType wall_time = { - .type_ = DDOG_CHARSLICE_C("wall-time"), - .unit = DDOG_CHARSLICE_C("nanoseconds"), + // Use the SampleType enum instead of ValueType struct + const ddog_prof_SampleType wall_time = DDOG_PROF_SAMPLE_TYPE_WALL_TIME; + const ddog_prof_Slice_SampleType sample_types = {&wall_time, 1}; + const ddog_prof_Period period = { + .sample_type = wall_time, + .value = 60, }; - const ddog_prof_Slice_ValueType sample_types = {&wall_time, 1}; - const ddog_prof_Period period = {wall_time, 60}; // Create a ProfilesDictionary for the new API ddog_prof_ProfilesDictionaryHandle dict = {0}; diff --git a/libdd-profiling-ffi/src/profiles/datatypes.rs b/libdd-profiling-ffi/src/profiles/datatypes.rs index 8cc3f72f1b..c7ec589d5c 100644 --- a/libdd-profiling-ffi/src/profiles/datatypes.rs +++ b/libdd-profiling-ffi/src/profiles/datatypes.rs @@ -96,27 +96,14 @@ impl From> for SerializeResult { } } -#[repr(C)] -#[derive(Copy, Clone)] -pub struct ValueType<'a> { - pub type_: CharSlice<'a>, - pub unit: CharSlice<'a>, -} - -impl<'a> ValueType<'a> { - pub fn new(type_: &'a str, unit: &'a str) -> Self { - Self { - type_: type_.into(), - unit: unit.into(), - } - } -} +/// Sample types supported by Datadog profilers. +/// These correspond to the ValueType entries used in pprof profiles. +/// Re-exported from libdd_profiling::api for FFI use. +pub use api::SampleType; -#[repr(C)] -pub struct Period<'a> { - pub type_: ValueType<'a>, - pub value: i64, -} +/// Re-export Period from the API for consistency. +/// The FFI Period is identical to the API Period. +pub use api::Period; #[repr(C)] #[derive(Copy, Clone, Default)] @@ -283,24 +270,6 @@ impl<'a> From<&'a Mapping<'a>> for api::StringIdMapping { } } -impl<'a> From<&'a ValueType<'a>> for api::ValueType<'a> { - fn from(vt: &'a ValueType<'a>) -> Self { - Self::new( - vt.type_.try_to_utf8().unwrap_or(""), - vt.unit.try_to_utf8().unwrap_or(""), - ) - } -} - -impl<'a> From<&'a Period<'a>> for api::Period<'a> { - fn from(period: &'a Period<'a>) -> Self { - Self { - r#type: api::ValueType::from(&period.type_), - value: period.value, - } - } -} - impl<'a> TryFrom<&'a Function<'a>> for api::Function<'a> { type Error = Utf8Error; @@ -423,7 +392,7 @@ impl<'a> From> for api::StringIdSample<'a> { /// `ddog_prof_Profile_drop` when you are done with the profile. /// /// # Arguments -/// * `sample_types` +/// * `sample_types` - A slice of SampleType enums /// * `period` - Optional period of the profile. Passing None/null translates to zero values. /// * `start_time` - Optional time the profile started at. Passing None/null will use the current /// time. @@ -434,7 +403,7 @@ impl<'a> From> for api::StringIdSample<'a> { #[no_mangle] #[must_use] pub unsafe extern "C" fn ddog_prof_Profile_new( - sample_types: Slice, + sample_types: Slice, period: Option<&Period>, ) -> ProfileNewResult { profile_new(sample_types, period, None) @@ -446,7 +415,7 @@ pub unsafe extern "C" fn ddog_prof_Profile_new( /// # Arguments /// * `out` - a non-null pointer to an uninitialized Profile. /// * `dict`: a valid reference to a ProfilesDictionary handle. -/// * `sample_types` +/// * `sample_types` - A slice of SampleType enums /// * `period` - Optional period of the profile. Passing None/null translates to zero values. /// /// # Safety @@ -463,7 +432,7 @@ pub unsafe extern "C" fn ddog_prof_Profile_new( pub unsafe extern "C" fn ddog_prof_Profile_with_dictionary( out: *mut Profile, dict: &ArcHandle, - sample_types: Slice, + sample_types: Slice, period: Option<&Period>, ) -> ProfileStatus { ensure_non_null_out_parameter!(out); @@ -481,18 +450,15 @@ pub unsafe extern "C" fn ddog_prof_Profile_with_dictionary( unsafe fn profile_with_dictionary( dict: &ArcHandle, - sample_types: Slice, + sample_types: Slice, period: Option<&Period>, ) -> Result { let sample_types = sample_types.try_as_slice()?; let dict = dict.try_clone_into_arc()?; - let mut types = Vec::new(); - types.try_reserve_exact(sample_types.len())?; - types.extend(sample_types.iter().map(api::ValueType::from)); - let period = period.map(Into::into); + let period = period.copied(); - match internal::Profile::try_new_with_dictionary(&types, period, dict) { + match internal::Profile::try_new_with_dictionary(sample_types, period, dict) { Ok(ok) => Ok(Profile::new(ok)), Err(err) => Err(ProfileError::from(err)), } @@ -503,7 +469,7 @@ unsafe fn profile_with_dictionary( #[must_use] /// TODO: @ivoanjo Should this take a `*mut ManagedStringStorage` like Profile APIs do? pub unsafe extern "C" fn ddog_prof_Profile_with_string_storage( - sample_types: Slice, + sample_types: Slice, period: Option<&Period>, string_storage: ManagedStringStorage, ) -> ProfileNewResult { @@ -511,22 +477,22 @@ pub unsafe extern "C" fn ddog_prof_Profile_with_string_storage( } unsafe fn profile_new( - sample_types: Slice, + sample_types: Slice, period: Option<&Period>, string_storage: Option, ) -> ProfileNewResult { - let types: Vec = sample_types.into_slice().iter().map(Into::into).collect(); - let period = period.map(Into::into); + let types = sample_types.into_slice(); + let period = period.copied(); let result = match string_storage { - None => internal::Profile::try_new(&types, period) + None => internal::Profile::try_new(types, period) .context("failed to initialize a profile without managed string storage"), Some(s) => { let string_storage = match get_inner_string_storage(s, true) { Ok(string_storage) => string_storage, Err(err) => return ProfileNewResult::Err(err.into()), }; - internal::Profile::try_with_string_storage(&types, period, string_storage) + internal::Profile::try_with_string_storage(types, period, string_storage) .context("failed to initialize a profile with managed string storage") } }; @@ -937,9 +903,9 @@ mod tests { #[test] fn ctor_and_dtor() -> Result<(), Error> { unsafe { - let sample_type: *const ValueType = &ValueType::new("samples", "count"); + let sample_type = SampleType::CpuSamples; let mut profile = Result::from(ddog_prof_Profile_new( - Slice::from_raw_parts(sample_type, 1), + Slice::from_raw_parts(&sample_type, 1), None, ))?; ddog_prof_Profile_drop(&mut profile); @@ -950,9 +916,9 @@ mod tests { #[test] fn add_failure() -> Result<(), Error> { unsafe { - let sample_type: *const ValueType = &ValueType::new("samples", "count"); + let sample_type = SampleType::CpuSamples; let mut profile = Result::from(ddog_prof_Profile_new( - Slice::from_raw_parts(sample_type, 1), + Slice::from_raw_parts(&sample_type, 1), None, ))?; @@ -977,9 +943,9 @@ mod tests { #[cfg_attr(miri, ignore)] fn aggregate_samples() -> anyhow::Result<()> { unsafe { - let sample_type: *const ValueType = &ValueType::new("samples", "count"); + let sample_type = SampleType::CpuSamples; let mut profile = Result::from(ddog_prof_Profile_new( - Slice::from_raw_parts(sample_type, 1), + Slice::from_raw_parts(&sample_type, 1), None, ))?; @@ -1039,9 +1005,9 @@ mod tests { } unsafe fn provide_distinct_locations_ffi() -> Profile { - let sample_type: *const ValueType = &ValueType::new("samples", "count"); + let sample_type = SampleType::CpuSamples; let mut profile = Result::from(ddog_prof_Profile_new( - Slice::from_raw_parts(sample_type, 1), + Slice::from_raw_parts(&sample_type, 1), None, )) .unwrap(); diff --git a/libdd-profiling/README.md b/libdd-profiling/README.md index 0a95c83341..6b462809ad 100644 --- a/libdd-profiling/README.md +++ b/libdd-profiling/README.md @@ -29,12 +29,12 @@ Core profiling library for collecting, aggregating, and exporting profiling data ## Example Usage ```rust -use libdd_profiling::api::{Profile, ValueType}; +use libdd_profiling::api::{Profile, SampleType}; // Create a profile -let value_types = vec![ - ValueType::new("samples", "count"), - ValueType::new("cpu", "nanoseconds"), +let sample_types = vec![ + SampleType::CpuSamples, + SampleType::CpuTime, ]; // Add samples with stack traces diff --git a/libdd-profiling/benches/add_samples.rs b/libdd-profiling/benches/add_samples.rs index ca392216df..6f43e0f828 100644 --- a/libdd-profiling/benches/add_samples.rs +++ b/libdd-profiling/benches/add_samples.rs @@ -6,8 +6,8 @@ use libdd_profiling::api2::Location2; use libdd_profiling::profiles::datatypes::{Function, FunctionId2, MappingId2}; use libdd_profiling::{self as profiling, api, api2}; -fn make_sample_types() -> Vec> { - vec![api::ValueType::new("samples", "count")] +fn make_sample_types() -> Vec { + vec![api::SampleType::CpuSamples] } fn make_stack_api(frames: &[Frame]) -> (Vec>, Vec) { diff --git a/libdd-profiling/examples/profiles.rs b/libdd-profiling/examples/profiles.rs index ee22c4acfa..2d4fed25d4 100644 --- a/libdd-profiling/examples/profiles.rs +++ b/libdd-profiling/examples/profiles.rs @@ -8,11 +8,10 @@ use std::process::exit; // Keep this in-sync with profiles.c fn main() { - let walltime = api::ValueType::new("wall-time", "nanoseconds"); - let sample_types = [api::ValueType::new("samples", "count"), walltime]; + let sample_types = [api::SampleType::CpuSamples, api::SampleType::WallTime]; let period = api::Period { - r#type: walltime, + sample_type: api::SampleType::WallTime, value: 10000, }; diff --git a/libdd-profiling/src/api.rs b/libdd-profiling/src/api/mod.rs similarity index 99% rename from libdd-profiling/src/api.rs rename to libdd-profiling/src/api/mod.rs index dca5dd7633..f95fd84be9 100644 --- a/libdd-profiling/src/api.rs +++ b/libdd-profiling/src/api/mod.rs @@ -1,6 +1,10 @@ // Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 +mod sample_type; + +pub use sample_type::SampleType; + use libdd_profiling_protobuf::prost_impls; use std::ops::{Add, Sub}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -18,9 +22,10 @@ impl<'a> ValueType<'a> { } } +#[repr(C)] #[derive(Copy, Clone, Debug, Eq, PartialEq)] -pub struct Period<'a> { - pub r#type: ValueType<'a>, +pub struct Period { + pub sample_type: SampleType, pub value: i64, } diff --git a/libdd-profiling/src/api/sample_type.rs b/libdd-profiling/src/api/sample_type.rs new file mode 100644 index 0000000000..633e0013c7 --- /dev/null +++ b/libdd-profiling/src/api/sample_type.rs @@ -0,0 +1,255 @@ +// Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use super::ValueType; + +/// Sample types supported by Datadog's profilers. +/// +/// The initial set of variants is taken from `dd-trace-rb`'s native profiler +/// (`stack_recorder.c`), specifically the `all_value_types` list: +/// `cpu-time`, `cpu-samples`, `wall-time`, `alloc-samples`, `alloc-samples-unscaled`, +/// `heap-live-samples`, `heap-live-size`, `timeline`. +/// +/// Additional variants are supported for dd-trace-py, based on the profiler's +/// `dd_wrapper/include/types.hpp` sample categories and the actual ValueType strings +/// configured in `dd_wrapper/src/profile.cpp`. +/// +/// I/O profiling sample types are from dd-trace-php's `profiling/src/profiling/samples.rs`. +#[cfg_attr(test, derive(bolero::generator::TypeGenerator))] +#[repr(C)] +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub enum SampleType { + AllocSamples, + AllocSamplesUnscaled, + AllocSpace, + CpuTime, + CpuSamples, + ExceptionSamples, + FileIoReadSize, + FileIoReadSizeSamples, + FileIoReadTime, + FileIoReadTimeSamples, + FileIoWriteSize, + FileIoWriteSizeSamples, + FileIoWriteTime, + FileIoWriteTimeSamples, + GpuAllocSamples, + GpuFlops, + GpuFlopsSamples, + GpuSamples, + GpuSpace, + GpuTime, + HeapLiveSamples, + HeapLiveSize, + HeapSpace, + LockAcquire, + LockAcquireWait, + LockReleaseHold, + LockRelease, + SocketReadSize, + SocketReadSizeSamples, + SocketReadTime, + SocketReadTimeSamples, + SocketWriteSize, + SocketWriteSizeSamples, + SocketWriteTime, + SocketWriteTimeSamples, + Timeline, + WallSamples, + WallTime, + + // Experimental sample types for testing and development. + ExperimentalCount, + ExperimentalNanoseconds, + ExperimentalBytes, +} + +impl From<&SampleType> for ValueType<'static> { + #[inline(always)] + fn from(sample_type: &SampleType) -> Self { + (*sample_type).into() + } +} + +impl From for ValueType<'static> { + #[inline(always)] + fn from(sample_type: SampleType) -> Self { + match sample_type { + SampleType::AllocSamples => ValueType::new("alloc-samples", "count"), + SampleType::AllocSamplesUnscaled => ValueType::new("alloc-samples-unscaled", "count"), + SampleType::AllocSpace => ValueType::new("alloc-space", "bytes"), + SampleType::CpuTime => ValueType::new("cpu-time", "nanoseconds"), + SampleType::CpuSamples => ValueType::new("cpu-samples", "count"), + SampleType::ExceptionSamples => ValueType::new("exception-samples", "count"), + SampleType::FileIoReadSize => ValueType::new("file-io-read-size", "bytes"), + SampleType::FileIoReadSizeSamples => { + ValueType::new("file-io-read-size-samples", "count") + } + SampleType::FileIoReadTime => ValueType::new("file-io-read-time", "nanoseconds"), + SampleType::FileIoReadTimeSamples => { + ValueType::new("file-io-read-time-samples", "count") + } + SampleType::FileIoWriteSize => ValueType::new("file-io-write-size", "bytes"), + SampleType::FileIoWriteSizeSamples => { + ValueType::new("file-io-write-size-samples", "count") + } + SampleType::FileIoWriteTime => ValueType::new("file-io-write-time", "nanoseconds"), + SampleType::FileIoWriteTimeSamples => { + ValueType::new("file-io-write-time-samples", "count") + } + SampleType::GpuAllocSamples => ValueType::new("gpu-alloc-samples", "count"), + SampleType::GpuFlops => ValueType::new("gpu-flops", "count"), + SampleType::GpuFlopsSamples => ValueType::new("gpu-flops-samples", "count"), + SampleType::GpuSamples => ValueType::new("gpu-samples", "count"), + SampleType::GpuSpace => ValueType::new("gpu-space", "bytes"), + SampleType::GpuTime => ValueType::new("gpu-time", "nanoseconds"), + SampleType::HeapLiveSamples => ValueType::new("heap-live-samples", "count"), + SampleType::HeapLiveSize => ValueType::new("heap-live-size", "bytes"), + SampleType::HeapSpace => ValueType::new("heap-space", "bytes"), + SampleType::LockAcquire => ValueType::new("lock-acquire", "count"), + SampleType::LockAcquireWait => ValueType::new("lock-acquire-wait", "nanoseconds"), + SampleType::LockReleaseHold => ValueType::new("lock-release-hold", "nanoseconds"), + SampleType::LockRelease => ValueType::new("lock-release", "count"), + SampleType::SocketReadSize => ValueType::new("socket-read-size", "bytes"), + SampleType::SocketReadSizeSamples => { + ValueType::new("socket-read-size-samples", "count") + } + SampleType::SocketReadTime => ValueType::new("socket-read-time", "nanoseconds"), + SampleType::SocketReadTimeSamples => { + ValueType::new("socket-read-time-samples", "count") + } + SampleType::SocketWriteSize => ValueType::new("socket-write-size", "bytes"), + SampleType::SocketWriteSizeSamples => { + ValueType::new("socket-write-size-samples", "count") + } + SampleType::SocketWriteTime => ValueType::new("socket-write-time", "nanoseconds"), + SampleType::SocketWriteTimeSamples => { + ValueType::new("socket-write-time-samples", "count") + } + SampleType::Timeline => ValueType::new("timeline", "nanoseconds"), + SampleType::WallSamples => ValueType::new("wall-samples", "count"), + SampleType::WallTime => ValueType::new("wall-time", "nanoseconds"), + SampleType::ExperimentalCount => ValueType::new("experimental-count", "count"), + SampleType::ExperimentalNanoseconds => { + ValueType::new("experimental-nanoseconds", "nanoseconds") + } + SampleType::ExperimentalBytes => ValueType::new("experimental-bytes", "bytes"), + } + } +} + +impl<'a> TryFrom> for SampleType { + type Error = anyhow::Error; + + fn try_from(vt: ValueType<'a>) -> Result { + Ok(match (vt.r#type, vt.unit) { + ("alloc-samples", "count") => SampleType::AllocSamples, + ("alloc-samples-unscaled", "count") => SampleType::AllocSamplesUnscaled, + ("alloc-space", "bytes") => SampleType::AllocSpace, + ("cpu-time", "nanoseconds") => SampleType::CpuTime, + ("cpu-samples", "count") => SampleType::CpuSamples, + ("exception-samples", "count") => SampleType::ExceptionSamples, + ("file-io-read-size", "bytes") => SampleType::FileIoReadSize, + ("file-io-read-size-samples", "count") => SampleType::FileIoReadSizeSamples, + ("file-io-read-time", "nanoseconds") => SampleType::FileIoReadTime, + ("file-io-read-time-samples", "count") => SampleType::FileIoReadTimeSamples, + ("file-io-write-size", "bytes") => SampleType::FileIoWriteSize, + ("file-io-write-size-samples", "count") => SampleType::FileIoWriteSizeSamples, + ("file-io-write-time", "nanoseconds") => SampleType::FileIoWriteTime, + ("file-io-write-time-samples", "count") => SampleType::FileIoWriteTimeSamples, + ("gpu-alloc-samples", "count") => SampleType::GpuAllocSamples, + ("gpu-flops", "count") => SampleType::GpuFlops, + ("gpu-flops-samples", "count") => SampleType::GpuFlopsSamples, + ("gpu-samples", "count") => SampleType::GpuSamples, + ("gpu-space", "bytes") => SampleType::GpuSpace, + ("gpu-time", "nanoseconds") => SampleType::GpuTime, + ("heap-live-samples", "count") => SampleType::HeapLiveSamples, + ("heap-live-size", "bytes") => SampleType::HeapLiveSize, + ("heap-space", "bytes") => SampleType::HeapSpace, + ("lock-acquire", "count") => SampleType::LockAcquire, + ("lock-acquire-wait", "nanoseconds") => SampleType::LockAcquireWait, + ("lock-release-hold", "nanoseconds") => SampleType::LockReleaseHold, + ("lock-release", "count") => SampleType::LockRelease, + ("socket-read-size", "bytes") => SampleType::SocketReadSize, + ("socket-read-size-samples", "count") => SampleType::SocketReadSizeSamples, + ("socket-read-time", "nanoseconds") => SampleType::SocketReadTime, + ("socket-read-time-samples", "count") => SampleType::SocketReadTimeSamples, + ("socket-write-size", "bytes") => SampleType::SocketWriteSize, + ("socket-write-size-samples", "count") => SampleType::SocketWriteSizeSamples, + ("socket-write-time", "nanoseconds") => SampleType::SocketWriteTime, + ("socket-write-time-samples", "count") => SampleType::SocketWriteTimeSamples, + ("timeline", "nanoseconds") => SampleType::Timeline, + ("wall-samples", "count") => SampleType::WallSamples, + ("wall-time", "nanoseconds") => SampleType::WallTime, + ("experimental-count", "count") => SampleType::ExperimentalCount, + ("experimental-nanoseconds", "nanoseconds") => SampleType::ExperimentalNanoseconds, + ("experimental-bytes", "bytes") => SampleType::ExperimentalBytes, + _ => anyhow::bail!("Unknown sample type: ({}, {})", vt.r#type, vt.unit), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn sample_type_round_trip_conversion() { + // Test that converting SampleType -> ValueType -> SampleType gives the same result + let all_types = [ + SampleType::AllocSamples, + SampleType::AllocSamplesUnscaled, + SampleType::AllocSpace, + SampleType::CpuTime, + SampleType::CpuSamples, + SampleType::ExceptionSamples, + SampleType::FileIoReadSize, + SampleType::FileIoReadSizeSamples, + SampleType::FileIoReadTime, + SampleType::FileIoReadTimeSamples, + SampleType::FileIoWriteSize, + SampleType::FileIoWriteSizeSamples, + SampleType::FileIoWriteTime, + SampleType::FileIoWriteTimeSamples, + SampleType::GpuAllocSamples, + SampleType::GpuFlops, + SampleType::GpuFlopsSamples, + SampleType::GpuSamples, + SampleType::GpuSpace, + SampleType::GpuTime, + SampleType::HeapLiveSamples, + SampleType::HeapLiveSize, + SampleType::HeapSpace, + SampleType::LockAcquire, + SampleType::LockAcquireWait, + SampleType::LockReleaseHold, + SampleType::LockRelease, + SampleType::SocketReadSize, + SampleType::SocketReadSizeSamples, + SampleType::SocketReadTime, + SampleType::SocketReadTimeSamples, + SampleType::SocketWriteSize, + SampleType::SocketWriteSizeSamples, + SampleType::SocketWriteTime, + SampleType::SocketWriteTimeSamples, + SampleType::Timeline, + SampleType::WallSamples, + SampleType::WallTime, + SampleType::ExperimentalCount, + SampleType::ExperimentalNanoseconds, + SampleType::ExperimentalBytes, + ]; + + for original in all_types { + let value_type: ValueType = original.into(); + let round_trip: SampleType = value_type + .try_into() + .unwrap_or_else(|e| panic!("Failed to convert {:?}: {}", original, e)); + assert_eq!( + original, round_trip, + "Round-trip conversion failed for {:?}", + original + ); + } + } +} diff --git a/libdd-profiling/src/cxx.rs b/libdd-profiling/src/cxx.rs index cca67196d5..628f3e12b8 100644 --- a/libdd-profiling/src/cxx.rs +++ b/libdd-profiling/src/cxx.rs @@ -15,14 +15,61 @@ use crate::internal; #[cxx::bridge(namespace = "datadog::profiling")] pub mod ffi { - // Shared structs - CXX-friendly types - struct ValueType<'a> { - type_: &'a str, - unit: &'a str, + // Shared types - CXX-friendly types + // + // This enum covers the ValueType entries supported by Datadog profilers: + // - dd-trace-rb (stack_recorder.c `all_value_types`) + // - dd-trace-py (dd_wrapper/include/types.hpp + dd_wrapper/src/profile.cpp value strings) + // - dd-trace-php (profiling/src/profiling/samples.rs I/O profiling sample types) + // + // Experimental variants allow profilers to use custom sample types for testing + // and development without modifying this enum. They map to fixed (type, unit) pairs. + enum SampleType { + AllocSamples, + AllocSamplesUnscaled, + AllocSpace, + CpuTime, + CpuSamples, + ExceptionSamples, + FileIoReadSize, + FileIoReadSizeSamples, + FileIoReadTime, + FileIoReadTimeSamples, + FileIoWriteSize, + FileIoWriteSizeSamples, + FileIoWriteTime, + FileIoWriteTimeSamples, + GpuAllocSamples, + GpuFlops, + GpuFlopsSamples, + GpuSamples, + GpuSpace, + GpuTime, + HeapLiveSamples, + HeapLiveSize, + HeapSpace, + LockAcquire, + LockAcquireWait, + LockReleaseHold, + LockRelease, + SocketReadSize, + SocketReadSizeSamples, + SocketReadTime, + SocketReadTimeSamples, + SocketWriteSize, + SocketWriteSizeSamples, + SocketWriteTime, + SocketWriteTimeSamples, + Timeline, + WallSamples, + WallTime, + ExperimentalCount, + ExperimentalNanoseconds, + ExperimentalBytes, } - struct Period<'a> { - value_type: ValueType<'a>, + struct Period { + value_type: SampleType, value: i64, } @@ -84,7 +131,7 @@ pub mod ffi { // Static factory methods for Profile #[Self = "Profile"] - fn create(sample_types: Vec, period: &Period) -> Result>; + fn create(sample_types: Vec, period: &Period) -> Result>; // Profile methods fn add_sample(self: &mut Profile, sample: &Sample) -> Result<()>; @@ -215,18 +262,65 @@ pub mod ffi { // From Implementations - Convert CXX types to API types // ============================================================================ -impl<'a> From<&ffi::ValueType<'a>> for api::ValueType<'a> { - fn from(vt: &ffi::ValueType<'a>) -> Self { - api::ValueType::new(vt.type_, vt.unit) +impl TryFrom for api::SampleType { + type Error = anyhow::Error; + + fn try_from(st: ffi::SampleType) -> Result { + Ok(match st { + ffi::SampleType::AllocSamples => api::SampleType::AllocSamples, + ffi::SampleType::AllocSamplesUnscaled => api::SampleType::AllocSamplesUnscaled, + ffi::SampleType::AllocSpace => api::SampleType::AllocSpace, + ffi::SampleType::CpuTime => api::SampleType::CpuTime, + ffi::SampleType::CpuSamples => api::SampleType::CpuSamples, + ffi::SampleType::ExceptionSamples => api::SampleType::ExceptionSamples, + ffi::SampleType::FileIoReadSize => api::SampleType::FileIoReadSize, + ffi::SampleType::FileIoReadSizeSamples => api::SampleType::FileIoReadSizeSamples, + ffi::SampleType::FileIoReadTime => api::SampleType::FileIoReadTime, + ffi::SampleType::FileIoReadTimeSamples => api::SampleType::FileIoReadTimeSamples, + ffi::SampleType::FileIoWriteSize => api::SampleType::FileIoWriteSize, + ffi::SampleType::FileIoWriteSizeSamples => api::SampleType::FileIoWriteSizeSamples, + ffi::SampleType::FileIoWriteTime => api::SampleType::FileIoWriteTime, + ffi::SampleType::FileIoWriteTimeSamples => api::SampleType::FileIoWriteTimeSamples, + ffi::SampleType::GpuAllocSamples => api::SampleType::GpuAllocSamples, + ffi::SampleType::GpuFlops => api::SampleType::GpuFlops, + ffi::SampleType::GpuFlopsSamples => api::SampleType::GpuFlopsSamples, + ffi::SampleType::GpuSamples => api::SampleType::GpuSamples, + ffi::SampleType::GpuSpace => api::SampleType::GpuSpace, + ffi::SampleType::GpuTime => api::SampleType::GpuTime, + ffi::SampleType::HeapLiveSamples => api::SampleType::HeapLiveSamples, + ffi::SampleType::HeapLiveSize => api::SampleType::HeapLiveSize, + ffi::SampleType::HeapSpace => api::SampleType::HeapSpace, + ffi::SampleType::LockAcquire => api::SampleType::LockAcquire, + ffi::SampleType::LockAcquireWait => api::SampleType::LockAcquireWait, + ffi::SampleType::LockReleaseHold => api::SampleType::LockReleaseHold, + ffi::SampleType::LockRelease => api::SampleType::LockRelease, + ffi::SampleType::SocketReadSize => api::SampleType::SocketReadSize, + ffi::SampleType::SocketReadSizeSamples => api::SampleType::SocketReadSizeSamples, + ffi::SampleType::SocketReadTime => api::SampleType::SocketReadTime, + ffi::SampleType::SocketReadTimeSamples => api::SampleType::SocketReadTimeSamples, + ffi::SampleType::SocketWriteSize => api::SampleType::SocketWriteSize, + ffi::SampleType::SocketWriteSizeSamples => api::SampleType::SocketWriteSizeSamples, + ffi::SampleType::SocketWriteTime => api::SampleType::SocketWriteTime, + ffi::SampleType::SocketWriteTimeSamples => api::SampleType::SocketWriteTimeSamples, + ffi::SampleType::Timeline => api::SampleType::Timeline, + ffi::SampleType::WallSamples => api::SampleType::WallSamples, + ffi::SampleType::WallTime => api::SampleType::WallTime, + ffi::SampleType::ExperimentalCount => api::SampleType::ExperimentalCount, + ffi::SampleType::ExperimentalNanoseconds => api::SampleType::ExperimentalNanoseconds, + ffi::SampleType::ExperimentalBytes => api::SampleType::ExperimentalBytes, + _ => anyhow::bail!("invalid SampleType discriminant from C++"), + }) } } -impl<'a> From<&ffi::Period<'a>> for api::Period<'a> { - fn from(period: &ffi::Period<'a>) -> Self { - api::Period { - r#type: (&period.value_type).into(), +impl TryFrom<&ffi::Period> for api::Period { + type Error = anyhow::Error; + + fn try_from(period: &ffi::Period) -> Result { + Ok(api::Period { + sample_type: period.value_type.try_into()?, value: period.value, - } + }) } } @@ -345,12 +439,15 @@ pub struct Profile { impl Profile { pub fn create( - sample_types: Vec, + sample_types: Vec, period: &ffi::Period, ) -> anyhow::Result> { - // Convert using From trait - let types: Vec = sample_types.iter().map(Into::into).collect(); - let period_value: api::Period = period.into(); + // Convert (fallibly) from CXX types to API types + let types: Vec = sample_types + .into_iter() + .map(TryInto::try_into) + .collect::, _>>()?; + let period_value: api::Period = period.try_into()?; // Profile::try_new interns the strings let inner = internal::Profile::try_new(&types, Some(period_value))?; @@ -673,20 +770,13 @@ mod tests { const TEST_LIB_VERSION: &str = "1.0.0"; const TEST_FAMILY: &str = "test"; - fn create_test_value_type() -> ffi::ValueType<'static> { - ffi::ValueType { - type_: "wall-time", - unit: "nanoseconds", - } - } - fn create_test_profile() -> Box { - let wall_time = create_test_value_type(); + let wall_time = ffi::SampleType::WallTime; let period = ffi::Period { value_type: wall_time, value: 60, }; - Profile::create(vec![create_test_value_type()], &period).unwrap() + Profile::create(vec![ffi::SampleType::WallTime], &period).unwrap() } fn create_test_location(address: u64, line: i64) -> ffi::Location<'static> { @@ -925,12 +1015,9 @@ mod tests { }) .is_err()); - // ValueType conversion - let vt: api::ValueType = (&ffi::ValueType { - type_: "cpu-samples", - unit: "count", - }) - .into(); + // SampleType conversion + let st: api::SampleType = ffi::SampleType::CpuSamples.try_into().unwrap(); + let vt: api::ValueType<'static> = st.into(); assert_eq!(vt.r#type, "cpu-samples"); assert_eq!(vt.unit, "count"); diff --git a/libdd-profiling/src/internal/mod.rs b/libdd-profiling/src/internal/mod.rs index d934de756f..56e4fe65c6 100644 --- a/libdd-profiling/src/internal/mod.rs +++ b/libdd-profiling/src/internal/mod.rs @@ -8,7 +8,6 @@ mod label; mod location; mod mapping; mod observation; -mod owned_types; mod profile; mod sample; mod stack_trace; diff --git a/libdd-profiling/src/internal/owned_types/mod.rs b/libdd-profiling/src/internal/owned_types/mod.rs deleted file mode 100644 index d4955292f1..0000000000 --- a/libdd-profiling/src/internal/owned_types/mod.rs +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/ -// SPDX-License-Identifier: Apache-2.0 - -use crate::api; - -#[cfg_attr(test, derive(bolero::generator::TypeGenerator))] -#[derive(Clone, Debug)] -pub struct ValueType { - pub typ: Box, - pub unit: Box, -} - -impl<'a> From<&'a api::ValueType<'a>> for ValueType { - #[inline] - fn from(value_type: &'a api::ValueType<'a>) -> Self { - Self { - typ: Box::from(value_type.r#type), - unit: Box::from(value_type.unit), - } - } -} - -impl<'a> From<&'a ValueType> for api::ValueType<'a> { - fn from(value: &'a ValueType) -> Self { - Self::new(&value.typ, &value.unit) - } -} - -#[cfg_attr(test, derive(bolero::generator::TypeGenerator))] -#[derive(Clone, Debug)] -pub struct Period { - pub typ: ValueType, - pub value: i64, -} - -impl<'a> From> for Period { - #[inline] - fn from(period: api::Period<'a>) -> Self { - Period::from(&period) - } -} - -impl<'a> From<&'a api::Period<'a>> for Period { - #[inline] - fn from(period: &'a api::Period<'a>) -> Self { - Self { - typ: ValueType::from(&period.r#type), - value: period.value, - } - } -} diff --git a/libdd-profiling/src/internal/profile/fuzz_tests.rs b/libdd-profiling/src/internal/profile/fuzz_tests.rs index bbb3e0c406..a134462254 100644 --- a/libdd-profiling/src/internal/profile/fuzz_tests.rs +++ b/libdd-profiling/src/internal/profile/fuzz_tests.rs @@ -291,10 +291,7 @@ impl<'a> From<&'a Sample> for api::Sample<'a> { } #[track_caller] -fn assert_sample_types_eq( - profile: &pprof::Profile, - expected_sample_types: &[owned_types::ValueType], -) { +fn assert_sample_types_eq(profile: &pprof::Profile, expected_sample_types: &[api::SampleType]) { assert_eq!( profile.sample_types.len(), expected_sample_types.len(), @@ -305,11 +302,9 @@ fn assert_sample_types_eq( .iter() .zip(expected_sample_types.iter()) { - assert_eq!( - *string_table_fetch(profile, typ.r#type), - *expected_typ.r#typ - ); - assert_eq!(*string_table_fetch(profile, typ.unit), *expected_typ.unit); + let expected_vt: api::ValueType<'static> = (*expected_typ).into(); + assert_eq!(*string_table_fetch(profile, typ.r#type), expected_vt.r#type); + assert_eq!(*string_table_fetch(profile, typ.unit), expected_vt.unit); } } @@ -430,7 +425,7 @@ fn assert_samples_eq( fn fuzz_add_sample<'a>( timestamp: &Option, sample: &'a Sample, - expected_sample_types: &[owned_types::ValueType], + expected_sample_types: &[api::SampleType], profile: &mut Profile, samples_with_timestamps: &mut Vec<&'a Sample>, samples_without_timestamps: &mut HashMap<(&'a [Location], &'a [Label]), Vec>, @@ -508,7 +503,7 @@ fn fuzz_failure_001() { #[test] #[cfg_attr(miri, ignore)] fn test_fuzz_add_sample() { - let sample_types_gen = Vec::::produce(); + let sample_types_gen = Vec::::produce(); let samples_gen = Vec::<(Option, FuzzSample)>::produce(); bolero::check!() @@ -519,11 +514,7 @@ fn test_fuzz_add_sample() { .map(|(tstamp, sample)| (*tstamp, Sample::from(sample))) .collect::>(); - let sample_types: Vec<_> = expected_sample_types - .iter() - .map(api::ValueType::from) - .collect(); - let mut expected_profile = Profile::new(&sample_types, None); + let mut expected_profile = Profile::new(expected_sample_types, None); let mut samples_with_timestamps = Vec::new(); let mut samples_without_timestamps: HashMap<(&[Location], &[Label]), Vec> = HashMap::new(); @@ -558,9 +549,7 @@ fn fuzz_add_sample_with_fixed_sample_length() { .with_shrink_time(Duration::from_secs(60)) .with_generator(sample_length_gen) .and_then(|sample_len| { - let sample_types = Vec::::produce() - .with() - .len(sample_len); + let sample_types = Vec::::produce().with().len(sample_len); let timestamps = Option::::produce(); let locations = Vec::::produce(); @@ -579,8 +568,7 @@ fn fuzz_add_sample_with_fixed_sample_length() { (sample_types, samples) }) .for_each(|(sample_types, samples)| { - let api_sample_types: Vec<_> = sample_types.iter().map(api::ValueType::from).collect(); - let mut profile = Profile::new(&api_sample_types, None); + let mut profile = Profile::new(sample_types, None); let mut samples_with_timestamps = Vec::new(); let mut samples_without_timestamps: HashMap<(&[Location], &[Label]), Vec> = HashMap::new(); @@ -686,9 +674,7 @@ fn fuzz_api_function_calls() { bolero::check!() .with_generator(sample_length_gen) .and_then(|sample_len| { - let sample_types = Vec::::produce() - .with() - .len(sample_len); + let sample_types = Vec::::produce().with().len(sample_len); let operations = Vec::::produce(); (sample_types, operations) @@ -696,8 +682,7 @@ fn fuzz_api_function_calls() { .for_each(|(sample_types, operations)| { let operations = operations.iter().map(Operation::from).collect::>(); - let api_sample_types: Vec<_> = sample_types.iter().map(api::ValueType::from).collect(); - let mut profile = Profile::new(&api_sample_types, None); + let mut profile = Profile::new(sample_types, None); let mut samples_with_timestamps: Vec<&Sample> = Vec::new(); let mut samples_without_timestamps: HashMap<(&[Location], &[Label]), Vec> = HashMap::new(); diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index 4a4e1f9d28..280cebcd7a 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -15,7 +15,6 @@ use crate::api::ManagedStringId; use crate::collections::identifiable::*; use crate::collections::string_storage::{CachedProfileId, ManagedStringStorage}; use crate::collections::string_table::{self, StringTable}; -use crate::internal::owned_types; use crate::iter::{IntoLendingIterator, LendingIterator}; use crate::profiles::collections::ArcOverflow; use crate::profiles::datatypes::ProfilesDictionary; @@ -36,14 +35,6 @@ pub struct Profile { /// Translates from the new Id2 APIs to the older internal APIs. Long-term, /// this should probably use the dictionary directly. profiles_dictionary_translator: Option, - /// When profiles are reset, the sample-types need to be preserved. This - /// maintains them in a way that does not depend on the string table. The - /// Option part is this is taken from the old profile and moved to the new - /// one. - owned_sample_types: Option>, - /// When profiles are reset, the period needs to be preserved. This - /// stores it in a way that does not depend on the string table. - owned_period: Option, active_samples: AtomicU64, endpoints: Endpoints, functions: FxIndexSet, @@ -53,8 +44,8 @@ pub struct Profile { locations: FxIndexSet, mappings: FxIndexSet, observations: Observations, - period: Option<(i64, ValueType)>, - sample_types: Box<[ValueType]>, + period: Option, + sample_types: Box<[api::SampleType]>, stack_traces: FxIndexSet, start_time: SystemTime, strings: StringTable, @@ -381,7 +372,7 @@ impl Profile { /// This is used heavily enough in tests to make a helper. #[cfg(test)] - pub fn new(sample_types: &[api::ValueType], period: Option) -> Self { + pub fn new(sample_types: &[api::SampleType], period: Option) -> Self { #[allow(clippy::unwrap_used)] Self::try_new(sample_types, period).unwrap() } @@ -397,33 +388,23 @@ impl Profile { /// /// It's recommended to use [`Profile::try_new_with_dictionary`] instead. pub fn try_new( - sample_types: &[api::ValueType], + sample_types: &[api::SampleType], period: Option, ) -> io::Result { - Self::try_new_internal( - Self::backup_period(period), - Self::backup_sample_types(sample_types), - None, - None, - ) + Self::try_new_internal(period, sample_types.to_vec().into_boxed_slice(), None, None) } /// Tries to create a profile with the given period and sample types. #[inline(never)] #[cold] pub fn try_new_with_dictionary( - sample_types: &[api::ValueType], + sample_types: &[api::SampleType], period: Option, profiles_dictionary: crate::profiles::collections::Arc, ) -> io::Result { - let mut owned_sample_types = Vec::new(); - // Using try_reserve_exact because it will be converted to Box<[]>, - // so excess capacity would make that conversion more expensive. - owned_sample_types.try_reserve_exact(sample_types.len())?; - owned_sample_types.extend(sample_types.iter().map(owned_types::ValueType::from)); Self::try_new_internal( - period.map(owned_types::Period::from), - Some(owned_sample_types.into_boxed_slice()), + period, + sample_types.to_vec().into_boxed_slice(), None, Some(ProfilesDictionaryTranslator::new(profiles_dictionary)), ) @@ -431,13 +412,13 @@ impl Profile { #[inline] pub fn try_with_string_storage( - sample_types: &[api::ValueType], + sample_types: &[api::SampleType], period: Option, string_storage: Arc>, ) -> io::Result { Self::try_new_internal( - Self::backup_period(period), - Self::backup_sample_types(sample_types), + period, + sample_types.to_vec().into_boxed_slice(), Some(string_storage), None, ) @@ -463,8 +444,8 @@ impl Profile { .transpose()?; let mut profile = Profile::try_new_internal( - self.owned_period.take(), - self.owned_sample_types.take(), + self.period, + self.sample_types.clone(), self.string_storage.clone(), profiles_dictionary_translator, ) @@ -599,10 +580,19 @@ impl Profile { // // In this case, we use `sample_types` during upscaling of `samples`, // so we must serialize `Sample` before `SampleType`. - for sample_type in self.sample_types.iter() { - Record::<_, 1, NO_OPT_ZERO>::from(*sample_type).encode(writer)?; + // Clone to avoid holding an immutable borrow of `self.sample_types` while interning. + let sample_types = self.sample_types.clone(); + for sample_type in sample_types.iter().copied() { + let sample_type = self.intern_sample_type(sample_type); + Record::<_, 1, NO_OPT_ZERO>::from(sample_type).encode(writer)?; } + // Period type needs string interning, which must happen before we start moving fields out + // of `self`. (Many fields below are consumed via `into_iter()`.) + let period_type_and_value: Option<(ValueType, i64)> = self + .period + .map(|period| (self.intern_sample_type(period.sample_type), period.value)); + for (offset, item) in self.mappings.into_iter().enumerate() { let mapping = protobuf::Mapping { id: Record::from((offset + 1) as u64), @@ -653,7 +643,7 @@ impl Profile { Record::<_, 9, OPT_ZERO>::from(time_nanos).encode(writer)?; Record::<_, 10, OPT_ZERO>::from(duration_nanos).encode(writer)?; - if let Some((period, period_type)) = self.period { + if let Some((period_type, period)) = period_type_and_value { Record::<_, 11, OPT_ZERO>::from(period_type).encode(writer)?; Record::<_, 12, OPT_ZERO>::from(period).encode(writer)?; }; @@ -823,14 +813,13 @@ impl Profile { self.stack_traces.try_dedup(StackTrace { locations }) } - #[inline] - fn backup_period(src: Option) -> Option { - src.as_ref().map(owned_types::Period::from) - } - - #[inline] - fn backup_sample_types(src: &[api::ValueType]) -> Option> { - Some(src.iter().map(owned_types::ValueType::from).collect()) + #[inline(always)] + fn intern_sample_type(&mut self, sample_type: api::SampleType) -> ValueType { + let vt: api::ValueType<'static> = sample_type.into(); + ValueType { + r#type: Record::from(self.intern(vt.r#type)), + unit: Record::from(self.intern(vt.unit)), + } } /// Fetches the endpoint information for the label. There may be errors, @@ -922,16 +911,14 @@ impl Profile { /// Creates a profile from the period, sample types, and start time using /// the owned values. fn try_new_internal( - owned_period: Option, - owned_sample_types: Option>, + period: Option, + sample_types: Box<[api::SampleType]>, string_storage: Option>>, profiles_dictionary_translator: Option, ) -> io::Result { let start_time = SystemTime::now(); let mut profile = Self { profiles_dictionary_translator, - owned_period, - owned_sample_types, active_samples: Default::default(), endpoints: Default::default(), functions: Default::default(), @@ -942,8 +929,8 @@ impl Profile { locations: Default::default(), mappings: Default::default(), observations: Default::default(), - period: None, - sample_types: Box::new([]), + period, + sample_types, stack_traces: Default::default(), start_time, strings: Default::default(), @@ -961,35 +948,6 @@ impl Profile { profile.endpoints.endpoint_label = profile.intern("trace endpoint"); profile.timestamp_key = profile.intern("end_timestamp_ns"); - // Break "cannot borrow `*self` as mutable because it is also borrowed - // as immutable" by moving it out, borrowing it, and putting it back. - let owned_sample_types = profile.owned_sample_types.take(); - profile.sample_types = match &owned_sample_types { - None => Box::new([]), - Some(sample_types) => sample_types - .iter() - .map(|sample_type| ValueType { - r#type: Record::from(profile.intern(&sample_type.typ)), - unit: Record::from(profile.intern(&sample_type.unit)), - }) - .collect(), - }; - profile.owned_sample_types = owned_sample_types; - - // Break "cannot borrow `*self` as mutable because it is also borrowed - // as immutable" by moving it out, borrowing it, and putting it back. - let owned_period = profile.owned_period.take(); - if let Some(owned_types::Period { value, typ }) = &owned_period { - profile.period = Some(( - *value, - ValueType { - r#type: Record::from(profile.intern(&typ.typ)), - unit: Record::from(profile.intern(&typ.unit)), - }, - )); - }; - profile.owned_period = owned_period; - profile.observations = Observations::try_new(profile.sample_types.len())?; Ok(profile) } @@ -1106,7 +1064,7 @@ mod api_tests { #[test] fn interning() { - let sample_types = [api::ValueType::new("samples", "count")]; + let sample_types = [api::SampleType::CpuSamples]; let mut profiles = Profile::new(&sample_types, None); let expected_id = StringId::from_offset(profiles.interned_strings_count()); @@ -1121,10 +1079,7 @@ mod api_tests { #[test] fn api() { - let sample_types = [ - api::ValueType::new("samples", "count"), - api::ValueType::new("wall-time", "nanoseconds"), - ]; + let sample_types = [api::SampleType::CpuSamples, api::SampleType::WallTime]; let mapping = api::Mapping { filename: "php", @@ -1172,7 +1127,7 @@ mod api_tests { } fn provide_distinct_locations() -> Profile { - let sample_types = [api::ValueType::new("samples", "count")]; + let sample_types = [api::SampleType::CpuSamples]; let mapping = api::Mapping { filename: "php", @@ -1383,9 +1338,9 @@ mod api_tests { /* The previous test (reset) checked quite a few properties already, so * this one will focus only on the period. */ - let sample_types = [api::ValueType::new("wall-time", "nanoseconds")]; + let sample_types = [api::SampleType::WallTime]; let period = api::Period { - r#type: sample_types[0], + sample_type: sample_types[0], value: 10_000_000, }; let mut profile = Profile::new(&sample_types, Some(period)); @@ -1394,26 +1349,13 @@ mod api_tests { .reset_and_return_previous() .expect("reset to succeed"); - // Resolve the string values to check that they match (their string - // table offsets may not match). - let mut strings: Vec> = Vec::with_capacity(profile.strings.len()); - let mut strings_iter = profile.strings.into_lending_iter(); - while let Some(item) = strings_iter.next() { - strings.push(Box::from(item)); - } - - for (value, period_type) in [profile.period.unwrap(), prev.period.unwrap()] { - assert_eq!(value, period.value); - let r#type: &str = &strings[usize::from(period_type.r#type.value)]; - let unit: &str = &strings[usize::from(period_type.unit.value)]; - assert_eq!(r#type, period.r#type.r#type); - assert_eq!(unit, period.r#type.unit); - } + assert_eq!(profile.period, Some(period)); + assert_eq!(prev.period, Some(period)); } #[test] fn adding_local_root_span_id_with_string_value_fails() { - let sample_types = [api::ValueType::new("wall-time", "nanoseconds")]; + let sample_types = [api::SampleType::WallTime]; let mut profile: Profile = Profile::new(&sample_types, None); @@ -1435,10 +1377,7 @@ mod api_tests { #[test] fn lazy_endpoints() -> anyhow::Result<()> { - let sample_types = [ - api::ValueType::new("samples", "count"), - api::ValueType::new("wall-time", "nanoseconds"), - ]; + let sample_types = [api::SampleType::CpuSamples, api::SampleType::WallTime]; let mut profile: Profile = Profile::new(&sample_types, None); @@ -1528,10 +1467,7 @@ mod api_tests { #[test] fn endpoint_counts_empty_test() { - let sample_types = [ - api::ValueType::new("samples", "count"), - api::ValueType::new("wall-time", "nanoseconds"), - ]; + let sample_types = [api::SampleType::CpuSamples, api::SampleType::WallTime]; let profile: Profile = Profile::new(&sample_types, None); @@ -1545,10 +1481,7 @@ mod api_tests { #[test] fn endpoint_counts_test() -> anyhow::Result<()> { - let sample_types = [ - api::ValueType::new("samples", "count"), - api::ValueType::new("wall-time", "nanoseconds"), - ]; + let sample_types = [api::SampleType::CpuSamples, api::SampleType::WallTime]; let mut profile: Profile = Profile::new(&sample_types, None); @@ -1577,7 +1510,7 @@ mod api_tests { #[test] fn local_root_span_id_label_cannot_occur_more_than_once() { - let sample_types = [api::ValueType::new("wall-time", "nanoseconds")]; + let sample_types = [api::SampleType::WallTime]; let mut profile: Profile = Profile::new(&sample_types, None); @@ -1607,10 +1540,7 @@ mod api_tests { #[test] fn test_no_upscaling_if_no_rules() { - let sample_types = vec![ - api::ValueType::new("samples", "count"), - api::ValueType::new("wall-time", "nanoseconds"), - ]; + let sample_types = vec![api::SampleType::CpuSamples, api::SampleType::WallTime]; let mut profile: Profile = Profile::new(&sample_types, None); @@ -1640,11 +1570,11 @@ mod api_tests { assert_eq!(first.values[1], 10000); } - fn create_samples_types() -> Vec> { + fn create_samples_types() -> Vec { vec![ - api::ValueType::new("samples", "count"), - api::ValueType::new("wall-time", "nanoseconds"), - api::ValueType::new("cpu-time", "nanoseconds"), + api::SampleType::CpuSamples, + api::SampleType::WallTime, + api::SampleType::CpuTime, ] } @@ -2596,16 +2526,7 @@ mod api_tests { #[test] fn local_root_span_id_label_as_i64() -> anyhow::Result<()> { - let sample_types = vec![ - api::ValueType { - r#type: "samples", - unit: "count", - }, - api::ValueType { - r#type: "wall-time", - unit: "nanoseconds", - }, - ]; + let sample_types = vec![api::SampleType::CpuSamples, api::SampleType::WallTime]; let mut profile = Profile::new(&sample_types, None); @@ -2726,7 +2647,7 @@ mod api_tests { // Create a ProfilesDictionary with realistic data from Ruby app let dict = crate::profiles::datatypes::ProfilesDictionary::try_new().unwrap(); - let sample_types = vec![api::ValueType::new("samples", "count")]; + let sample_types = vec![api::SampleType::CpuSamples]; // Ruby stack trace (leaf-to-root order) // Taken from a Ruby app, everything here is source-available @@ -2854,7 +2775,7 @@ mod api_tests { let sample_type = &pprof.sample_types[0]; let type_str = &pprof.string_table[sample_type.r#type as usize]; let unit_str = &pprof.string_table[sample_type.unit as usize]; - assert_eq!(type_str, "samples"); + assert_eq!(type_str, "cpu-samples"); assert_eq!(unit_str, "count"); // Verify the mapping is present and has the correct filename @@ -2928,7 +2849,7 @@ mod api_tests { world_id = storage_guard.intern("world").unwrap(); } - let sample_types = [api::ValueType::new("samples", "count")]; + let sample_types = [api::SampleType::CpuSamples]; let mut profile = Profile::try_with_string_storage(&sample_types, None, storage.clone()).unwrap(); @@ -2981,7 +2902,7 @@ mod api_tests { #[test] fn reproduce_crash_with_anyhow_bailout() { - let sample_types = [api::ValueType::new("samples", "count")]; + let sample_types = [api::SampleType::CpuSamples]; let mapping = api::Mapping { filename: "test.php", ..Default::default()