Skip to content

Add ddprof_ffi_Profile_set_endpoint#33

Merged
r1viollet merged 4 commits into
mainfrom
kevin/lazy_labels
Jul 29, 2022
Merged

Add ddprof_ffi_Profile_set_endpoint#33
r1viollet merged 4 commits into
mainfrom
kevin/lazy_labels

Conversation

@kevingosse

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR implements and exposes a ddprof_ffi_Profile_set_endpoint function, that is used to lazily associate endpoints to spans.

Motivation

The goal is to help profilers support endpoint profiling.

The function is use to lazily associate endpoints to spans,
to help support endpoint profiling.
@kevingosse
kevingosse requested a review from a team as a code owner July 25, 2022 14:31
for sample in samples.iter_mut() {
let mut endpoint: Option<&i64> = None;

for label in &sample.label {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't touch local root span id, but we only add the endpoint label if local root span id matches a known endpoint

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment added in f8fd2f9

@r1viollet r1viollet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 ivoanjo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 👍

Comment on lines +343 to +348
#[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>,
) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in f8fd2f9

Comment on lines 244 to 246
period: None,
endpoints: Endpoints::new(),
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in f8fd2f9

@morrisonlevi morrisonlevi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks good! I'm going to try using it; will report back soon.

@morrisonlevi morrisonlevi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@kevingosse

Copy link
Copy Markdown
Contributor Author

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.

Yep, the feature on the .NET side is already fully written and tested 🙂 DataDog/dd-trace-dotnet#3015

@r1viollet
r1viollet merged commit 6163674 into main Jul 29, 2022
@kevingosse
kevingosse deleted the kevin/lazy_labels branch July 29, 2022 12:57
ivoanjo added a commit that referenced this pull request Jun 18, 2025
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants