Add ddprof_ffi_Profile_set_endpoint#33
Conversation
The function is use to lazily associate endpoints to spans, to help support endpoint profiling.
| for sample in samples.iter_mut() { | ||
| let mut endpoint: Option<&i64> = None; | ||
|
|
||
| for label in &sample.label { |
There was a problem hiding this comment.
What is the advantage of using a generic label vs having a dedicated field in the sample to store the endpoint information ? I'm thinking users with many labels might have extra serialisation costs due to the lookup on all labels.
There was a problem hiding this comment.
The lookup here is to find the span id. We could indeed add a dedicated field for the span id, but I believe the cost of the lookup to be negligible (I'd be very surprised if a profile stores more than ~10 labels on a sample).
That said, we already iterate over all the labels once to serialize them, so I could merge the two loops.
| }) | ||
| .collect(); | ||
|
|
||
| if !profile.endpoints.mappings.is_empty() { |
There was a problem hiding this comment.
it is nice that users without endpoints won't pay the cost for this
|
|
||
| profile.add(sample2).expect("add to success"); | ||
|
|
||
| profile.add_endpoint(Cow::from("10"), Cow::from("my endpoint")); |
There was a problem hiding this comment.
nice, I think this goes well with a lot of the runtime requirements
| let s2 = serialized_profile.sample.get(1).expect("sample"); | ||
|
|
||
| // The trace endpoint label shouldn't be added to second sample because the span id doesn't match | ||
| assert_eq!(s2.label.len(), 2); |
There was a problem hiding this comment.
so when local root span id does not match a known endpoint, we remove the label from the serialized data
(just checking if I understand this well)
There was a problem hiding this comment.
We don't touch local root span id, but we only add the endpoint label if local root span id matches a known endpoint
There was a problem hiding this comment.
This is a good point though -- I guess since we intern(...) the endpoint, we still ship it, even if there's no matching local root span id that needs it.
Which I don't think is particularly problematic (but perhaps documenting with a comment on the API?).
r1viollet
left a comment
There was a problem hiding this comment.
I did a quick review on APIs and things look good.
A minor concern about the increase in complexity. I think a dedicated field could be better (and we would then only append to the labels).
I'm not commenting on rust side of things.
Thanks for adding this !
ivoanjo
left a comment
There was a problem hiding this comment.
This looks great! Ideally @morrisonlevi, @paullegranddc or @pawelchcki can comment on the small Rust-isms that we can perhaps improve on but here sir, take my 👍
| #[no_mangle] | ||
| pub unsafe extern "C" fn ddprof_ffi_Profile_set_endpoint<'a>( | ||
| profile: &mut ddprof_profiles::Profile, | ||
| local_root_span_id: CharSlice<'a>, | ||
| endpoint: CharSlice<'a>, | ||
| ) { |
There was a problem hiding this comment.
Minor: I suggest adding a comment explaining what this is used for (bonus points for changing one of the examples/ffi to use the API as well :D)
| period: None, | ||
| endpoints: Endpoints::new(), | ||
| }; |
There was a problem hiding this comment.
Minor: Since there's an implementation of Default for Endpoints, perhaps this one could be Default::default() as well?
(Not that I particularly like this pattern, from my not-Rust-developer-POV I'd prefer to see the types here, but I guess consistency?)
morrisonlevi
left a comment
There was a problem hiding this comment.
It looks good! I'm going to try using it; will report back soon.
morrisonlevi
left a comment
There was a problem hiding this comment.
I hit some snags trying it out from the PHP profiler because of how the profiler is written. However, I couldn't see any reason why this PR wouldn't be suitable. Have you tried out the PR in .NET? Ideally someone would send profiles, even if just a PoC, using the PR to demonstrate the real-world viability, but I'm not going to block on it.
f8fd2f9 to
f8ceb9d
Compare
f8ceb9d to
a0b7fcd
Compare
Yep, the feature on the .NET side is already fully written and tested 🙂 DataDog/dd-trace-dotnet#3015 |
…er instead of a ByteSlice (#33) * [PROF-4780] Breaking change: Change FFI File struct to contain a Buffer instead of a ByteSlice In #30 we added the `ddprof_ffi_Buffer_from_byte_slice` function to allow FFI users to provide their own data to be reported using the `ProfileExporterV3` (via `ddprof_ffi_ProfileExporterV3_build`). Thus, if one wanted to report some data, it first converted it from a `ByteSlice` into a `Buffer`, and then invoke `libddprof` with it. Here's a (simplified) example from the Ruby profiler: ```c ddprof_ffi_File files[1]; ddprof_ffi_Slice_file slice_files = {.ptr = files, .len = 1}; ddprof_ffi_Buffer *pprof_buffer = ddprof_ffi_Buffer_from_byte_slice((ddprof_ffi_ByteSlice) { .ptr = /* byte array with data we want to send */, .len = /* ... */ }); files[0] = (ddprof_ffi_File) {.name = /* ... */, .file = pprof_buffer}; ddprof_ffi_Request *request = ddprof_ffi_ProfileExporterV3_build(exporter, start, finish, slice_files, timeout_milliseconds); ddprof_ffi_Buffer_free(pprof_buffer); ``` This approach had a few downsides: 1. It copied the data to be reported twice. It would be first copied into a `Buffer`, and then inside `ddprof_ffi_ProfileExporterV3_build` it would be copied again. 2. **Callers manually needed to clean up the `Buffer` afterwards using `ddprof_ffi_Buffer_free`**. After discussing this with @morrisonlevi, we decided to go the other way: change the `File` to contain a `ByteSlice` directly. This avoids the extra copy in (1.), as well as the caller needing to manage the `Buffer` objects manually in (2.). This is a breaking API change for libddprof FFI users. * Fix exporter.cpp example compilation by enabling C++ 11 * Update exporter.cpp example with new `File` struct API * Use target_compile_features instead of cmake_cxx_standard to enable c++11 * Update examples/ffi/exporter.cpp Minor cleanup as suggested during review. Co-authored-by: Nicolas Savoire <[email protected]> Co-authored-by: Nicolas Savoire <[email protected]>
What does this PR do?
This PR implements and exposes a
ddprof_ffi_Profile_set_endpointfunction, that is used to lazily associate endpoints to spans.Motivation
The goal is to help profilers support endpoint profiling.