Skip to content

[PROF-11524] Fix not being able to set profile start_time at serialization #963

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/ffi/exporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ int main(int argc, char *argv[]) {
}

ddog_prof_Profile_SerializeResult serialize_result =
ddog_prof_Profile_serialize(profile.get(), nullptr, nullptr, nullptr);
ddog_prof_Profile_serialize(profile.get(), nullptr, nullptr);
if (serialize_result.tag == DDOG_PROF_PROFILE_SERIALIZE_RESULT_ERR) {
print_error("Failed to serialize profile: ", serialize_result.err);
ddog_Error_drop(&serialize_result.err);
Expand Down
25 changes: 7 additions & 18 deletions profiling-ffi/src/profiles/datatypes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use ddcommon_ffi::{wrap_with_ffi_result, Error, Handle, Timespec, ToInner};
use function_name::named;
use std::num::NonZeroI64;
use std::str::Utf8Error;
use std::time::{Duration, SystemTime};
use std::time::SystemTime;

/// Represents a profile. Do not access its member for any reason, only use
/// the C API functions on this struct.
Expand Down Expand Up @@ -742,42 +742,31 @@ pub unsafe extern "C" fn ddog_prof_EncodedProfile_bytes<'a>(
///
/// # Arguments
/// * `profile` - a reference to the profile being serialized.
/// * `start_time` - start time for the serialized profile.
/// * `end_time` - optional end time of the profile. If None/null is passed, the current time will
/// be used.
/// * `duration_nanos` - Optional duration of the profile. Passing None or a negative duration will
/// mean the duration will based on the end time minus the start time, but under anomalous
/// conditions this may fail as system clocks can be adjusted, or the programmer accidentally
/// passed an earlier time. The duration of the serialized profile will be set to zero for these
/// cases.
/// * `start_time` - Optional start time for the next profile.
///
/// # Safety
/// The `profile` must point to a valid profile object.
/// The `end_time` must be null or otherwise point to a valid TimeSpec object.
/// The `duration_nanos` must be null or otherwise point to a valid i64.
/// The `start_time` and `end_time` must be null or otherwise point to a valid TimeSpec object.
#[must_use]
#[no_mangle]
pub unsafe extern "C" fn ddog_prof_Profile_serialize(
profile: *mut Profile,
end_time: Option<&Timespec>,
duration_nanos: Option<&i64>,
start_time: Option<&Timespec>,
end_time: Option<&Timespec>,
) -> SerializeResult {
(|| {
let profile = profile_ptr_to_inner(profile)?;

let old_profile = profile.reset_and_return_previous()?;
if let Some(start_time) = start_time {
profile.set_start_time(start_time.into())?;
}

let old_profile = profile.reset_and_return_previous()?;

Comment on lines 762 to +767
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this clearer;

Suggested change
if let Some(start_time) = start_time {
profile.set_start_time(start_time.into())?;
}
let old_profile = profile.reset_and_return_previous()?;
let old_profile = profile.reset_and_return_previous()?;
if let Some(start_time) = start_time {
old_profile.set_start_time(start_time.into())?;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed this suggestion before the auto-merged kicked in, but I'll open a separate PR with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #987 with this change.

let end_time = end_time.map(SystemTime::from);
let duration = match duration_nanos {
None => None,
Some(x) if *x < 0 => None,
Some(x) => Some(Duration::from_nanos((*x) as u64)),
};
old_profile.serialize_into_compressed_pprof(end_time, duration)
old_profile.serialize_into_compressed_pprof(end_time, None)
})()
.context("ddog_prof_Profile_serialize failed")
.into()
Expand Down
Loading