[PROF-11524] Fix not being able to set profile start_time at serialization #963
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ation **What does this PR do?** In #926 we removed the `start_time` argument from * `ddog_prof_Profile_new` * `ddog_prof_Profile_with_string_storage` * `ddog_prof_Profile_reset` The intention was that having it as an argument in `ddog_prof_Profile_serialize` was enough, and anyway almost everyone was passing in `null`s in the APIs above. I missed when suggesting in that PR that the `start_time` for serialize was the `start_time` for the next profile, not the one being serialized. In this PR I'm changing that behavior: the `start_time` argument to serialize now controls the time for the profile being serialized, allowing the profiling library to have exact control over this value. I've also removed the duration. [As can be seen in this github search](https://github.com/search?q=org%3ADataDog+ddog_prof_Profile_serialize&type=code) this is not expected to impact anyone: everyone's passing `NULL` for `start_time` and `duration` when calling serialize already. **Motivation:** Allow Ruby profiler to set the start_time of profiles. **Additional Notes:** N/A **How to test the change?** I've tested this with my experimental libdatadog 17 branch for Ruby.
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
i686-alpine-linux-musl
i686-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
BenchmarksComparisonBenchmark execution time: 2025-03-27 17:37:25 Comparing candidate commit 788d9a6 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 50 metrics, 2 unstable metrics. scenario:normalization/normalize_service/normalize_service/Data🐨dog🐶 繋がっ⛰てて
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
danielsn
reviewed
Mar 25, 2025
profiling-ffi/src/profiles.rs
Outdated
| 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) |
There was a problem hiding this comment.
Does this function need to take a duration?
There was a problem hiding this comment.
I think PHP is using this, at least that's why it was added originally; I didn't check yet if that's still the case 👀
There was a problem hiding this comment.
I've checked it and PHP is still using it -- https://github.com/DataDog/dd-trace-php/blob/342ff3234879e8868633423c508407ef6eb84ef0/profiling/src/profiling/uploader.rs#L188 .
danielsn
approved these changes
Mar 27, 2025
danielsn
reviewed
Mar 27, 2025
Comment on lines
762
to
+767
| if let Some(start_time) = start_time { | ||
| profile.set_start_time(start_time.into())?; | ||
| } | ||
|
|
||
| let old_profile = profile.reset_and_return_previous()?; | ||
|
|
There was a problem hiding this comment.
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())?; | |
| } |
There was a problem hiding this comment.
I missed this suggestion before the auto-merged kicked in, but I'll open a separate PR with it.
There was a problem hiding this comment.
Opened #987 with this change.
ivoanjo
added a commit
that referenced
this pull request
Mar 31, 2025
**What does this PR do?** This PR applies a suggestion from #963 (comment) to set the `start_time` after obtaining the old profile. This will hopefully make it more clear that the `start_time` is used for the old profile, not the new one. **Motivation:** Make code more readable. **Additional Notes:** N/A **How to test the change?** Existing test coverage should be enough to validate this change (for instance, the Ruby profiler test suite explicitly tests the timestamps on profiles).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
In #926 we removed the
start_timeargument fromddog_prof_Profile_newddog_prof_Profile_with_string_storageddog_prof_Profile_resetThe intention was that having it as an argument in
ddog_prof_Profile_serializewas enough, and anyway almost everyone was passing innulls in the APIs above.I missed when suggesting in that PR that the
start_timefor serialize was thestart_timefor the next profile, not the one being serialized.In this PR I'm changing that behavior: the
start_timeargument to serialize now controls the time for the profile being serialized, allowing the profiling library to have exact control over this value.I've also removed the duration.
As can be seen in this github search this is not expected to impact anyone: everyone's passing
NULLforstart_timeanddurationwhen calling serialize already.Motivation
Allow Ruby profiler to set the start_time of profiles.
Additional Notes
N/A
How to test the change?
I've tested this with my experimental libdatadog 17 branch for Ruby.