Expose start, end, and duration in APIs #16
Conversation
There was a problem hiding this comment.
Code-wise the change seems reasonable, but my main point of confusion is not understanding the need for end time and duration to be decoupled.
Regardless of that, I do think it makes a lot of sense for the caller to be able to control the timestamps and other time-related things, as it makes libddprof usable in more scenarios for a low cost in terms of complexity.
ddprof-ffi/src/exporter.rs
Outdated
| #[export_name = "ddprof_ffi_Request_drop"] | ||
| pub extern "C" fn request_drop(_request: Option<Box<Request>>) {} |
There was a problem hiding this comment.
Minor: I've noticed that in the ffi we have a bit of a mix around function naming -- sometimes we use
#[no_mangle]
pub extern "C" fn ddprof_ffi_this_is_the_C_function_name() {}others we use
#[export_name = "ddprof_ffi_this_is_the_C_function_name"]
pub extern "C" fn pretty_rust_name() {}I slightly favor the first option, but regardless of which one we pick, I think it would be nice to be consistent here. Changing it all is perhaps not for this PR, but perhaps we could decide now which version we want and document it somewhere?
(I don't mind submitting a follow-up PR with the clean-up myself so that the ffi is consistent with what we decide)
ddprof-profiles/examples/profiles.rs
Outdated
| let duration = started.elapsed(); | ||
| profile.set_duration(Some(duration)); | ||
|
|
||
| match profile.serialize(None) { |
There was a problem hiding this comment.
I know we somewhat touched this in slack but looking at it again, I realized I didn't understood the ramifications of your proposal.
When would the end time NOT be start + duration? That's how pprof already represents it and intuitively that makes sense to me, so it's kinda weird that when set_duration gets called, we don't set the end time as well.
There was a problem hiding this comment.
Was my explanation in standup good enough? I also changed the behavior when no duration is given to try to calculate it from the end and start times.
ddprof-profiles/src/lib.rs
Outdated
| pub fn build(self) -> Profile { | ||
| let mut profile = Profile::new(); | ||
| let mut profile = Profile::new(self.time.unwrap_or_else(SystemTime::now)); | ||
|
|
||
| profile.duration = self.duration; |
There was a problem hiding this comment.
This is a bit weird to me. The ProfileBuilder() is the thing one uses to build a profile, so when would duration not be None here? 🤔
There was a problem hiding this comment.
Hmm. It seems like setting the duration in multiple places is not great. Let me see what it looks like with just setting it at serialization time.
There was a problem hiding this comment.
This looks good to me.
I'm still not completely convinced of the need to set duration and end_time independently BUT on this latest version of the change I think we have best of both worlds:
- (What I claim to be) reasonable defaults when passing just
NULLfor all ofstart_time/end_time/duration - Support for actually setting all three, for advanced use cases
I did spot a potential issue in the pprof encoding, but if I'm right the fix is a one-liner and given our discussion on slack I see no reason for me to hold off on the approval and let you look into it
.github/workflows/test.yml
Outdated
| - name: "Generate FFI" | ||
| run: bash ffi-build.sh /tmp/libddprof | ||
| - name: "Test building C bindings" | ||
| run: mkdir examples/ffi/build && cd examples/ffi/build && cmake -S .. -DDDProf_ROOT=/tmp/libddprof && cmake --build . |
There was a problem hiding this comment.
😍 Big thanks for this! This is a really really nice improvement :) :)
There was a problem hiding this comment.
I ended up pulling this into a stacked PR because getting it to work on Windows has been a challenge and I want to unblock this part: #17.
ddprof-ffi/src/profiles.rs
Outdated
| profile.reset().is_some() | ||
| pub unsafe extern "C" fn ddprof_ffi_Profile_reset( | ||
| profile: &mut ddprof_profiles::Profile, | ||
| time: Option<&Timespec>, |
There was a problem hiding this comment.
There's still quite a few places in this PR where we use time to mean start_time. I would suggest renaming them all to start_time. (I initially had it as suggested diffs on github but there's like 10 places that need renaming so I think it's just better if the IDE does it :D)
There was a problem hiding this comment.
I think I've got them all renamed now.
This allows the PHP profiler to back-date the start of the profile. When the node.js profiler was trying to use this library, they also encountered an issue with these not being exposed.
Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>
As per Ivo's recommendation in CR.
What does this PR do?
Exposes start, end, and duration in APIs. This is a breaking change.
Also adds a
ddprof_ffi_Request_dropfunction in case you don't wantto send it.
Motivation
This allows the PHP profiler to back-date the start of the profile.
When the node.js profiler was trying to use this library, they also
encountered an issue with these not being exposed.
Additional Notes
I threw in some refactoring for period and period type. Sorry. Should
have been a separate PR.
Also fixed an FFI failure that was caused by
Tagbeing extracted intoddcommon, which wasn't added to theincludesection ofcbindgen.toml. I've opened PR #17 to generate the FFI and try tobuild the examples against the FFI to prevent such breakages from
happening again.
How to test the change?
IMPORTANT! The following signatures changed:
ddprof_ffi_Profile_new. Has an additional parameter for the starttime. May pass None to have the previous behavior.
ddprof_ffi_Profile_serialize. Has additional parameters for the endtime and duration. May pass None for both. The duration's behavior
won't quite be the same as before because of potential clock skew,
but close enough. If you want to avoid that, then track duration
yourself.