Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[profiling] Use the new FFI macros and types #797

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[profiling] Use the new FFI macros and types #797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions ddcommon-ffi/src/handle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ pub struct Handle<T> {
}

pub trait ToInner<T> {
/// # Safety
/// The Handle must hold a valid `inner` which has been allocated and not freed.
unsafe fn to_inner(&self) -> anyhow::Result<&T>;
/// # Safety
/// The Handle must hold a valid `inner` which has been allocated and not freed.
unsafe fn to_inner_mut(&mut self) -> anyhow::Result<&mut T>;
Expand All @@ -21,6 +24,10 @@ pub trait ToInner<T> {
}

impl<T> ToInner<T> for *mut Handle<T> {
unsafe fn to_inner(&self) -> anyhow::Result<&T> {
self.as_ref().context("Null pointer")?.to_inner()
}

unsafe fn to_inner_mut(&mut self) -> anyhow::Result<&mut T> {
self.as_mut().context("Null pointer")?.to_inner_mut()
}
Expand All @@ -31,6 +38,12 @@ impl<T> ToInner<T> for *mut Handle<T> {
}

impl<T> ToInner<T> for Handle<T> {
unsafe fn to_inner(&self) -> anyhow::Result<&T> {
self.inner
.as_ref()
.context("inner pointer was null, indicates use after free")
}

unsafe fn to_inner_mut(&mut self) -> anyhow::Result<&mut T> {
self.inner
.as_mut()
Expand Down
18 changes: 18 additions & 0 deletions ddcommon-ffi/src/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ impl From<anyhow::Result<()>> for VoidResult {
}
}

impl From<VoidResult> for anyhow::Result<()> {
fn from(value: VoidResult) -> Self {
match value {
VoidResult::Ok(_) => Self::Ok(()),
VoidResult::Err(err) => Self::Err(err.into()),
}
}
}

/// A generic result type for when an operation may fail,
/// or may return <T> in case of success.
#[repr(C)]
Expand All @@ -49,3 +58,12 @@ impl<T> From<anyhow::Result<T>> for Result<T> {
}
}
}

impl<T> From<Result<T>> for anyhow::Result<T> {
fn from(value: Result<T>) -> Self {
match value {
Result::Ok(v) => Self::Ok(v),
Result::Err(err) => Self::Err(err.into()),
}
}
}
95 changes: 57 additions & 38 deletions examples/ffi/exporter.cpp
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't care about the C++ changes, if someone thinks it's better, it's fine to me.

Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,55 @@
#include <datadog/common.h>
#include <datadog/profiling.h>
#include <memory>
#include <string>
#include <thread>
#include <vector>

static ddog_CharSlice to_slice_c_char(const char *s) { return {.ptr = s, .len = strlen(s)}; }

struct Deleter {
void operator()(ddog_prof_Profile *object) { ddog_prof_Profile_drop(object); }
};
static ddog_CharSlice to_slice_c_char(const char *s, std::size_t size) {
return {.ptr = s, .len = size};
}
static ddog_CharSlice to_slice_string(std::string const &s) {
return {.ptr = s.data(), .len = s.length()};
}

void print_error(const char *s, const ddog_Error &err) {
auto charslice = ddog_Error_message(&err);
printf("%s (%.*s)\n", s, static_cast<int>(charslice.len), charslice.ptr);
}

#define CHECK_RESULT(typ, ok_tag) \
void check_result(typ result, const char *msg) { \
if (result.tag != ok_tag) { \
print_error(msg, result.err); \
ddog_Error_drop(&result.err); \
exit(EXIT_FAILURE); \
} \
}

CHECK_RESULT(ddog_VoidResult, DDOG_VOID_RESULT_OK)

#define EXTRACT_RESULT(typ, ok_tag) \
struct typ##Deleter { \
void operator()(ddog_prof_Handle_##typ *object) { \
ddog_prof_##typ##_drop(object); \
delete object; \
} \
}; \
std::unique_ptr<ddog_prof_Handle_##typ, typ##Deleter> extract_result( \
ddog_prof_Result_Handle##typ result, const char *msg) { \
if (result.tag != ok_tag) { \
print_error(msg, result.err); \
ddog_Error_drop(&result.err); \
exit(EXIT_FAILURE); \
} \
std::unique_ptr<ddog_prof_Handle_##typ, typ##Deleter> rval{ \
new ddog_prof_Handle_##typ{result.ok}}; \
return rval; \
}

EXTRACT_RESULT(Profile, DDOG_PROF_RESULT_HANDLE_PROFILE_OK_HANDLE_PROFILE)

int main(int argc, char *argv[]) {
if (argc != 2) {
printf("Usage: exporter SERVICE_NAME\n");
Expand All @@ -38,14 +74,8 @@ int main(int argc, char *argv[]) {

const ddog_prof_Slice_ValueType sample_types = {&wall_time, 1};
const ddog_prof_Period period = {wall_time, 60};
ddog_prof_Profile_NewResult profile_new_result =
ddog_prof_Profile_new(sample_types, &period, nullptr);
if (profile_new_result.tag != DDOG_PROF_PROFILE_NEW_RESULT_OK) {
print_error("Failed to make new profile: ", profile_new_result.err);
ddog_Error_drop(&profile_new_result.err);
exit(EXIT_FAILURE);
}
std::unique_ptr<ddog_prof_Profile, Deleter> profile{&profile_new_result.ok};
auto profile =
extract_result(ddog_prof_Profile_new(sample_types, &period, nullptr), "Can't get profile");

ddog_prof_Location root_location = {
// yes, a zero-initialized mapping is valid
Expand All @@ -67,30 +97,18 @@ int main(int argc, char *argv[]) {
.values = {&value, 1},
.labels = {&label, 1},
};
auto add_result = ddog_prof_Profile_add(profile.get(), sample, 0);
if (add_result.tag != DDOG_PROF_PROFILE_RESULT_OK) {
print_error("Failed to add sample to profile: ", add_result.err);
ddog_Error_drop(&add_result.err);
return 1;
}
check_result(ddog_prof_Profile_add(profile.get(), sample, 0), "failed to add");

uintptr_t offset[1] = {0};
ddog_prof_Slice_Usize offsets_slice = {.ptr = offset, .len = 1};
ddog_CharSlice empty_charslice = DDOG_CHARSLICE_C_BARE("");

auto upscaling_addresult = ddog_prof_Profile_add_upscaling_rule_proportional(
profile.get(), offsets_slice, empty_charslice, empty_charslice, 1, 1);
check_result(ddog_prof_Profile_add_upscaling_rule_proportional(
profile.get(), offsets_slice, empty_charslice, empty_charslice, 1, 1),
"failed to add upscaling rule");

if (upscaling_addresult.tag == DDOG_PROF_PROFILE_RESULT_ERR) {
print_error("Failed to add an upscaling rule: ", upscaling_addresult.err);
ddog_Error_drop(&upscaling_addresult.err);
// in this specific case, we want to fail the execution. But in general, we should not
return 1;
}

ddog_prof_Profile_SerializeResult serialize_result =
ddog_prof_Profile_serialize(profile.get(), nullptr, nullptr, nullptr);
if (serialize_result.tag == DDOG_PROF_PROFILE_SERIALIZE_RESULT_ERR) {
auto serialize_result = ddog_prof_Profile_serialize(profile.get(), nullptr, nullptr, nullptr);
if (serialize_result.tag != DDOG_PROF_RESULT_ENCODED_PROFILE_OK_ENCODED_PROFILE) {
print_error("Failed to serialize profile: ", serialize_result.err);
ddog_Error_drop(&serialize_result.err);
return 1;
Expand All @@ -110,9 +128,9 @@ int main(int argc, char *argv[]) {
return 1;
}

ddog_prof_Exporter_NewResult exporter_new_result =
ddog_prof_Exporter_new(DDOG_CHARSLICE_C_BARE("exporter-example"), DDOG_CHARSLICE_C_BARE("1.2.3"),
DDOG_CHARSLICE_C_BARE("native"), &tags, endpoint);
ddog_prof_Exporter_NewResult exporter_new_result = ddog_prof_Exporter_new(
DDOG_CHARSLICE_C_BARE("exporter-example"), DDOG_CHARSLICE_C_BARE("1.2.3"),
DDOG_CHARSLICE_C_BARE("native"), &tags, endpoint);
ddog_Vec_Tag_drop(tags);

if (exporter_new_result.tag == DDOG_PROF_EXPORTER_NEW_RESULT_ERR) {
Expand All @@ -137,14 +155,15 @@ int main(int argc, char *argv[]) {
ddog_CharSlice internal_metadata_example = DDOG_CHARSLICE_C_BARE(
"{\"no_signals_workaround_enabled\": \"true\", \"execution_trace_enabled\": \"false\"}");

ddog_CharSlice info_example = DDOG_CHARSLICE_C_BARE(
"{\"application\": {\"start_time\": \"2024-01-24T11:17:22+0000\"}, \"platform\": {\"kernel\": \"Darwin Kernel 22.5.0\"}}");
ddog_CharSlice info_example =
DDOG_CHARSLICE_C_BARE("{\"application\": {\"start_time\": \"2024-01-24T11:17:22+0000\"}, "
"\"platform\": {\"kernel\": \"Darwin Kernel 22.5.0\"}}");

auto res = ddog_prof_Exporter_set_timeout(exporter, 30000);
if (res.tag == DDOG_PROF_OPTION_ERROR_SOME_ERROR) {
print_error("Failed to set the timeout", res.some);
ddog_Error_drop(&res.some);
return 1;
print_error("Failed to set the timeout", res.some);
ddog_Error_drop(&res.some);
return 1;
}

ddog_prof_Exporter_Request_BuildResult build_result = ddog_prof_Exporter_Request_build(
Expand Down
14 changes: 7 additions & 7 deletions examples/ffi/profiles.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ int main(void) {
const ddog_prof_Slice_ValueType sample_types = {&wall_time, 1};
const ddog_prof_Period period = {wall_time, 60};

ddog_prof_Profile_NewResult new_result = ddog_prof_Profile_new(sample_types, &period, NULL);
if (new_result.tag != DDOG_PROF_PROFILE_NEW_RESULT_OK) {
ddog_prof_Result_HandleProfile new_result = ddog_prof_Profile_new(sample_types, &period, NULL);
if (new_result.tag != DDOG_PROF_RESULT_HANDLE_PROFILE_OK_HANDLE_PROFILE) {
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't write C API these days, but this looks worse. Can we use renames and/or aliases to improve this? Examples:

  • ddog_prof_Profile_NewResult aliased to ddog_prof_Result_HandleProfile
  • ddog_prof_Profile aliased to ddog_prof_Handle_Profile

Not sure what to do about the enum. Maybe @ivoanjo has thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm yeah I think Levi has a good point here, the "handle" change does look a bit like libdatadog internal implementation details being leaked across the FFI.

E.g. looking at it from the pure API client side, I'm not sure it's relevant for client to care what a ddog_prof_Profile actually is: is it a profile? a handle? something else?

I also know that (at least in the past) some of our types were a bit less what we wanted because of cbindgen limitations, and I think it's still a reasonable trade-off to have the types uglier if the rust code becomes less error-prone/etc. But ideally having both (clear naming for ffi and better rust types) would be great.

ddog_prof_Profile aliased to ddog_prof_Handle_Profile

TL;DR: 👍 on this one :)

On the Result change...

We've been using the convention of calling Module_operation returning Module_OperationResult since this PR. (And we did explore a few more options at the time)

One advantage of this new convention is that it may result (no pun intended) in fewer ...Result types, because they are now named by their payload's type, rather than the operation's type.

I still think I slightly favor more the current convention, but I think both seem quite reasonable. Here's a few examples for a comparison:

Current convention with this PR's convention Notes
ddog_prof_Profile_NewResult ddog_prof_Result_HandleProfile
ddog_prof_Exporter_NewResult ddog_prof_Result_Exporter
ddog_prof_Exporter_Request_BuildResult ddog_prof_Result_Request? ddog_prof_Result_Exporter_Request? How to handle the deeper namespace?
ddog_prof_Exporter_SendResult ddog_prof_Result_HttpStatus ? ddog_Result_HttpStatus ? How to handle when the type in the result is not named after the operation?
ddog_prof_Profile_Result ddog_prof_ResultBool? ddog_ResultBool? (Similar to above)
ddog_prof_Profile_SerializeResult ddog_prof_Result_EncodedProfile

(There's also a few more results in ddcommon-ffi)

Not wanting to discourage changes (I think one of the key ingredients of libdatadog's massive success so far has been having "no sacred cows", and instead always striving to leave things better, even if in the very short term it causes a bit of migration pain) I'll suggest that if we plan to introduce the new convention, I don't think we should mix both -- I think we should rip-off the band-aid and convert everything to the new convention.

ddog_CharSlice message = ddog_Error_message(&new_result.err);
fprintf(stderr, "%.*s", (int)message.len, message.ptr);
ddog_Error_drop(&new_result.err);
exit(EXIT_FAILURE);
}

ddog_prof_Profile *profile = &new_result.ok;
ddog_prof_Handle_Profile *profile = &new_result.ok;

ddog_prof_Location root_location = {
// yes, a zero-initialized mapping is valid
Expand All @@ -47,8 +47,8 @@ int main(void) {
for (int i = 0; i < 10000000; i++) {
label.num = i;

ddog_prof_Profile_Result add_result = ddog_prof_Profile_add(profile, sample, 0);
if (add_result.tag != DDOG_PROF_PROFILE_RESULT_OK) {
ddog_VoidResult add_result = ddog_prof_Profile_add(profile, sample, 0);
if (add_result.tag == DDOG_VOID_RESULT_ERR) {
ddog_CharSlice message = ddog_Error_message(&add_result.err);
fprintf(stderr, "%.*s", (int)message.len, message.ptr);
ddog_Error_drop(&add_result.err);
Expand All @@ -58,8 +58,8 @@ int main(void) {
// printf("Press any key to reset and drop...");
// getchar();

ddog_prof_Profile_Result reset_result = ddog_prof_Profile_reset(profile, NULL);
if (reset_result.tag != DDOG_PROF_PROFILE_RESULT_OK) {
ddog_VoidResult reset_result = ddog_prof_Profile_reset(profile, NULL);
if (reset_result.tag == DDOG_VOID_RESULT_ERR) {
ddog_CharSlice message = ddog_Error_message(&reset_result.err);
fprintf(stderr, "%.*s", (int)message.len, message.ptr);
ddog_Error_drop(&reset_result.err);
Expand Down
1 change: 1 addition & 0 deletions profiling-ffi/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,4 @@ symbolizer-ffi = { path = "../symbolizer-ffi", optional = true, default-features
symbolic-demangle = { version = "12.8.0", default-features = false, features = ["rust", "cpp", "msvc"] }
symbolic-common = "12.8.0"
data-pipeline-ffi = { path = "../data-pipeline-ffi", default-features = false, optional = true }
function_name = "0.3.0"
6 changes: 2 additions & 4 deletions profiling-ffi/cbindgen.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,15 @@ renaming_overrides_prefixing = true
"Timespec" = "ddog_Timespec"
"Vec_Tag" = "ddog_Vec_Tag"
"Vec_U8" = "ddog_Vec_U8"
"VoidResult" = "ddog_VoidResult"

"ProfilingEndpoint" = "ddog_prof_Endpoint"
"ExporterNewResult" = "ddog_prof_Exporter_NewResult"
"File" = "ddog_prof_Exporter_File"
"ProfileExporter" = "ddog_prof_Exporter"
"ProfileNewResult" = "ddog_prof_Profile_NewResult"
"ProfileResult" = "ddog_prof_Profile_Result"
"ProfilingEndpoint" = "ddog_prof_Endpoint"
"Request" = "ddog_prof_Exporter_Request"
"RequestBuildResult" = "ddog_prof_Exporter_Request_BuildResult"
"SendResult" = "ddog_prof_Exporter_SendResult"
"SerializeResult" = "ddog_prof_Profile_SerializeResult"
"Slice_File" = "ddog_prof_Exporter_Slice_File"

[export.mangle]
Expand Down
Loading
Loading