Skip to content

Add .clang-format with 100 line length#30

Merged
morrisonlevi merged 1 commit into
mainfrom
levi/clang-format
Jul 8, 2022
Merged

Add .clang-format with 100 line length#30
morrisonlevi merged 1 commit into
mainfrom
levi/clang-format

Conversation

@morrisonlevi

@morrisonlevi morrisonlevi commented Jul 8, 2022

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds .clang-format with 100 char line length. Reformats the examples
accordingly.

Motivation

Since Rust uses 100 character limit for ColumnLimit, may as well make
the C and C++ examples match.

Additional Notes

Nope.

How to test the change?

It's just a re-format, no need to test anything.

@morrisonlevi
Add .clang-format with 100 line length
2a77dd7 
Since Rust uses 100 character limit for ColumnLimit, may as well make
the C and C++ examples match.
@morrisonlevi morrisonlevi requested a review from a team as a code owner July 8, 2022 16:33
@morrisonlevi morrisonlevi merged commit 6bc8a37 into main Jul 8, 2022
@morrisonlevi morrisonlevi deleted the levi/clang-format branch July 8, 2022 21:59
ivoanjo added a commit that referenced this pull request Jun 18, 2025
@ivoanjo @morrisonlevi @r1viollet
RFC: Introduce ddprof_ffi_Buffer_from_byte_slice function to enable…
c7d3570 
… reporting of external data using libddprof_ffi (#30)

* RFC: Introduce `ddprof_ffi_Buffer_from_byte_slice` function to enable 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.

* Test that `byte_slice_to_buffer_conversion` copies input data

* Update and wrap comment

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.

Co-authored-by: Levi Morrison <levi.morrison@datadoghq.com>
ivoanjo added a commit that referenced this pull request Jun 18, 2025
@ivoanjo @nsavoire @r1viollet
[PROF-4780] Breaking change: Change FFI File struct to contain a Buff…
06e9611 
…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 <nicolas.savoire@datadoghq.com>

Co-authored-by: Nicolas Savoire <nicolas.savoire@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paullegranddc paullegranddc paullegranddc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@morrisonlevi @paullegranddc