Skip to content
This repository was archived by the owner on Jun 28, 2022. It is now read-only.

RFC: Introduce ddprof_ffi_Buffer_from_byte_slice function to enable reporting of external data using libddprof_ffi#30

Merged
ivoanjo merged 3 commits into
mainfrom
ivoanjo/add-buffer-from-byte-slice
Feb 24, 2022
Merged

RFC: Introduce ddprof_ffi_Buffer_from_byte_slice function to enable reporting of external data using libddprof_ffi#30
ivoanjo merged 3 commits into
mainfrom
ivoanjo/add-buffer-from-byte-slice

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Feb 24, 2022

Copy link
Copy Markdown
Member

Context:

The ProfileExporterV3 high-level API enables libddprof_ffi callers to report profiling data without needing to know anything about the backend API.

The current signature of build (used to prepare a request) is:

 #[export_name = "ddprof_ffi_ProfileExporterV3_build"]
pub unsafe extern "C" fn profile_exporter_build(
    exporter: Option<NonNull<ProfileExporterV3>>,
    start: Timespec,
    end: Timespec,
    files: Slice<File>,
    timeout_ms: u64,
) -> Option<Box<Request>>

in particular, the payload data to be reported (usually pprof and JSON data) is provided using a File, which is defined as:

 #[repr(C)]
pub struct File<'a> {
    name: ByteSlice<'a>,
    file: Option<NonNull<Buffer>>,
}

and further, a buffer is represented as

/// Buffer holds the raw parts of a Rust Vec; it should only be created from
/// Rust, never from C.
 #[repr(C)]
pub struct Buffer {
    ptr: *const u8,
    len: size_t,
    capacity: size_t,
}

It follows from the required usage of Buffer that libddprof_ffi callers can only provide Buffers that were created by libddprof_ffi.

The only way a libddprof_ffi caller can get a Buffer containing profiling data, is through usage of ddprof_ffi_Profile_serialize which returns a EncodedProfile which contains a Buffer with the encoded data.

Thus, this means that libddprof_ffi callers cannot provide their own data to be reported, e.g. to report a pprof that was encoded using different means (for instance, if the runtime provided it), or to report the code_provenance.json or metrics.json extra files.

Use case:

My use case is the integration of libddprof_ffi into the Ruby profiler:

  1. I want to make this a staged introduction, so in the first version I will only be using libddprof_ffi to report data, not to encode it
  2. Ruby is already reporting code_provenance.json and this should continue to be reported even after moving to libddprof_ffi

Alternatives considered:

I considered two main approaches to this problem:

  1. Introduce a new ddprof_ffi_Buffer_from_byte_slice API. This API copies a ByteSlice (an array of bytes) into a Buffer, which we can then happily use.

    Advantage: simpler, as no other APIs need to change to support this

    Disadvantage: introduces an extra copy of the data (even if temporary)

  2. a) Modify profile_exporter_build to receive a ByteSlice inside File, not a Buffer

    Advantage: resulting API simpler and more concise to use; avoids extra copy from (1.)

    Disadvantage: involves several breaking API changes

  3. b) Leave existing profile_exporter_build API, but add an extra one to receive ByteSlices

    Advantage: no breaking API changes; avoids extra copy from (1.)

    Disadvantage: adds complexity and possibly duplication

In this PR, I decided to go with (1.), as it seems to me the extra copy is not that roblematic.

Additional Notes:

This is my first time writing Rust code so please DO suspect that EVERYTHING IS OFF AND THAT THIS IS A TERRIBLE MISTAKE.
And then help me figure out how to improve it :)

This writeup may be a bit overkill for this small change, but this is 20% sharing with the rest of the team what I'm doing and 80% working through this myself and making sure I understand what's up :)

How to test the change:

This is a new API; my plan is to integration test it on the dd-trace-rb side: write a test that validates that the correct HTTP requests are done, with the correct payloads.

… reporting of external data using libddprof_ffi

 # Context:

The `ProfileExporterV3` high-level API enables libddprof_ffi callers to report profiling data without needing to know anything about the backend API.

The current signature of `build` (used to prepare a request) is:

```rust
 #[export_name = "ddprof_ffi_ProfileExporterV3_build"]
pub unsafe extern "C" fn profile_exporter_build(
    exporter: Option<NonNull<ProfileExporterV3>>,
    start: Timespec,
    end: Timespec,
    files: Slice<File>,
    timeout_ms: u64,
) -> Option<Box<Request>>
```

in particular, the payload data to be reported (usually pprof and JSON data) is provided using a `File`, which is defined as:

```rust
 #[repr(C)]
pub struct File<'a> {
    name: ByteSlice<'a>,
    file: Option<NonNull<Buffer>>,
}
```

and further, a buffer is represented as

```rust
/// Buffer holds the raw parts of a Rust Vec; it should only be created from
/// Rust, never from C.
 #[repr(C)]
pub struct Buffer {
    ptr: *const u8,
    len: size_t,
    capacity: size_t,
}
```

It follows from the required usage of `Buffer` that libddprof_ffi callers can only provide `Buffers` that were created by libddprof_ffi.

The only way a libddprof_ffi caller can get a `Buffer` containing profiling data, is through usage of `ddprof_ffi_Profile_serialize` which returns a `EncodedProfile` which contains a `Buffer` with the encoded data.

Thus, this means that libddprof_ffi callers **cannot provide their own data** to be reported, e.g. to report a pprof that was encoded using different means (for instance, if the runtime provided it), or to report the `code_provenance.json` or `metrics.json` extra files.

 # Use case:

My use case is the integration of libddprof_ffi into the Ruby profiler:
1. I want to make this a staged introduction, so in the first version I will only be using libddprof_ffi to report data, not to encode it
2. Ruby is already reporting `code_provenance.json` and this should continue to be reported even after moving to libddprof_ffi

 # Alternatives considered:

I considered two main approaches to this problem:

1. Introduce a new `ddprof_ffi_Buffer_from_byte_slice` API. This API copies a `ByteSlice` (an array of bytes) into a `Buffer`, which we can then happily use.

    Advantage: simpler, as no other APIs need to change to support this

    Disadvantage: introduces an extra copy of the data (even if temporary)

2. a) Modify `profile_exporter_build` to receive a `ByteSlice` inside `File`, not a `Buffer`

    Advantage: resulting API simpler and more concise to use; avoids extra copy from (1.)

    Disadvantage: involves several breaking API changes

2. b) Leave existing `profile_exporter_build` API, but add an extra one to receive `ByteSlice`s

    Advantage: no breaking API changes; avoids extra copy from (1.)

    Disadvantage: adds complexity and possibly duplication

In this PR, I decided to go with (1.), as it seems to me the extra copy is not that roblematic.

 # Additional Notes:

This is my first time writing Rust code so please DO suspect that EVERYTHING IS OFF AND THAT THIS IS A TERRIBLE MISTAKE.
And then help me figure out how to improve it :)

This writeup may be a bit overkill for this small change, but this is 20% sharing with the rest of the team what I'm doing and 80% working through this myself and making sure I understand what's up :)

 # How to test the change:

This is a new API; my plan is to integration test it on the dd-trace-rb side: write a test that validates that the correct HTTP requests are done, with the correct payloads.
Comment thread ddprof-ffi/src/exporter.rs Outdated
let converted: Box<Buffer> = unsafe { ddprof_ffi_Buffer_from_byte_slice(slice) };
let converted_contents: &[u8] = unsafe { (*converted).as_slice() };

assert_eq!(b"dummy data", converted_contents);

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.

Perhaps the test should be done once the raw data is dropped to ensure we are reading correctly off of the copy ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. I don't think it makes sense to drop the raw data since it's not being heap allocated, but in 3bf0fc6 I enhanced the test so that the raw data gets mutated and we validate that the copy is unchanged.

@r1viollet

Copy link
Copy Markdown
Contributor

So the extra copy is only for the data that is not yet owned by libddprof, so the metrics data. That should be totally fine. The API seems like a good addition to me !
I'm bad at Rust so I can't provide you with feedback on the Rust part.

ivoanjo and others added 2 commits February 24, 2022 13:20
I try to keep comments and code to 80 lines despite Rust's preference
for the 100 column limit. I don't make code worse just to make 80 cols,
but for comments it's pretty easy.
@ivoanjo
ivoanjo merged commit 265a85f into main Feb 24, 2022
@ivoanjo
ivoanjo deleted the ivoanjo/add-buffer-from-byte-slice branch February 24, 2022 16:10
ivoanjo added a commit that referenced this pull request Mar 9, 2022
…er 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);
```

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.
ivoanjo added a commit that referenced this pull request Mar 9, 2022
…er 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.
ivoanjo added a commit that referenced this pull request Mar 11, 2022
…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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants