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

Deliver libddprof_ffi as a shared library #22

Merged
merged 10 commits into from
Feb 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ debug = 2 # full debug info

[profile.release]
debug = 1 # line tables only
lto = true
codegen-units = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something we want to put in the cargo config, or could we not make sure to have it as a command line argument in the CI ?
I'm thinking we dan't want to slow all of our build times. Only the build time to generate the github release.
WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, locally we will use the develop profile. We can leave the release profile optimised for size.

opt-level = "s" # optimize for size
2 changes: 1 addition & 1 deletion LICENSE-3rdparty.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
root_name: "ddprof, ddprof-exporter, ddprof-ffi"
root_name: "ddprof, ddprof-exporter, ddprof-profiles"
third_party_libraries:
- package_name: aho-corasick
package_version: 0.7.18
Expand Down
4 changes: 3 additions & 1 deletion ddprof-ffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ edition = "2018"
license = "Apache-2.0"

[lib]
crate-type = ["lib", "staticlib"]
# LTO is ignored if "lib" is added as crate type
# cf. https://github.com/rust-lang/rust/issues/51009
Copy link
Contributor

Choose a reason for hiding this comment

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

nice referencing this

crate-type = ["staticlib", "cdylib"]

[dependencies]
chrono = "0.4"
Expand Down
1 change: 0 additions & 1 deletion ddprof/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,4 @@ crate-type = ["cdylib", "lib"]

[dependencies]
ddprof-exporter = { path = "../ddprof-exporter" }
ddprof-ffi = { path = "../ddprof-ffi" }
ddprof-profiles = { path = "../ddprof-profiles" }
1 change: 0 additions & 1 deletion ddprof/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,4 @@
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.

pub use ddprof_exporter as exporter;
pub use ddprof_ffi as ffi;
pub use ddprof_profiles as profiles;
17 changes: 17 additions & 0 deletions ddprof_ffi-static.pc.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Unless explicitly stated otherwise all files in this repository are licensed
# under the Apache License Version 2.0. This product includes software
# developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present
# Datadog, Inc.

prefix=${pcfiledir}/../..
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: ddprof_ffi
Description: Contains common code used to implement Datadog's Continuous Profilers.
Version: @DDProf_FFI_VERSION@
Requires:
Libs: -L${libdir} ${libdir}/libddprof_ffi.a @DDProf_FFI_LIBRARIES@
Libs.private:
Cflags: -I${includedir}
2 changes: 1 addition & 1 deletion ddprof_ffi.pc.in
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ Name: ddprof_ffi
Description: Contains common code used to implement Datadog's Continuous Profilers.
Version: @DDProf_FFI_VERSION@
Requires:
Libs: -L${libdir} -lddprof_ffi @DDProf_FFI_LIBRARIES@
Libs: -L${libdir} -lddprof_ffi
Libs.private:
Cflags: -I${includedir}
9 changes: 9 additions & 0 deletions examples/ffi/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
project(ffi_examples)
cmake_minimum_required(VERSION 3.19)

find_package(DDProf)

add_executable(exporter exporter.cpp)
target_link_libraries(exporter DDProf::FFI)
r1viollet marked this conversation as resolved.
Show resolved Hide resolved
add_executable(profiles profiles.c)
target_link_libraries(profiles DDProf::FFI)
150 changes: 150 additions & 0 deletions examples/ffi/exporter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
extern "C"
{
#include <ddprof/ffi.h>
}
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

static ddprof_ffi_ByteSlice to_byteslice(const char *s)
{
return {.ptr = (uint8_t *)s,
.len = strlen(s)};
}

static ddprof_ffi_Slice_c_char to_slice_c_char(const char *s)
{
return {.ptr = s,
.len = strlen(s)};
}
Comment on lines +10 to +20
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but this looks to me as a weird sharp edge in the libddprof API.

It's weird to have both ddprof_ffi_ByteSlice and ddprof_ffi_Slice_c_char when they are effectively the same (?).

Also, I had to write similar helpers for using libddprof, so perhaps it would make sense to export these (possibly as macros?) in ffi.h as well :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You usually don't want to encourage people to transport strings without an actual size associated to it. strlen is also not something you want encourage, so I don't know how I feel about exposing these types of APIs.
I guess we can also mention this :-)

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... I'm guessing the "don't want to encourage" may be related to performance -- is that the case?

If so, would it make sense to call it something like to_slice_slow? I guess for a bunch of hardcoded strings ("ruby", "wall-clock", "nanoseconds", "service", ...) we just use them once so the cost is trivial so we could have the ease of use instead :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I agree there is room for improvements concerning the APIs, we could provide some helpers function like to_slice_xxx on top of FFI headers. Maybe even a more friendly C++ interface.
  • It's strange that slices for type/unit/filename/build_id/name/system_name/... are different from slices used for /tags/filenames.

But this would be subject of another pull request ;)

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 feel super strongly about this, but I would usually avoid relying on C style null terminated strings. Though it is not clear how we could avoid these conversions at boundaries or how to have a common types.


struct Deleter
{
void operator()(ddprof_ffi_Profile *object)
{
ddprof_ffi_Profile_free(object);
}
void operator()(ddprof_ffi_ProfileExporterV3 *object)
{
ddprof_ffi_ProfileExporterV3_delete(object);
}
void operator()(ddprof_ffi_EncodedProfile *object)
{
ddprof_ffi_EncodedProfile_delete(object);
}
};

template <typename T>
class Holder
{
public:
explicit Holder(T *object) : _object(object) {}
~Holder() { Deleter{}(_object); }
Holder(const Holder &) = delete;
Holder &operator=(const Holder &) = delete;

operator T *() { return _object; }
T *operator->() { return _object; }

T *_object;
};

template <typename T>
void print_error(const char *s, const T &err)
{
printf("%s: (%.*s)\n", s, static_cast<int>(err.len), err.ptr);
}

int main(int argc, char* argv[])
{
if (argc != 2) {
printf("Usage: exporter SERVICE_NAME\n");
return 1;
}
const char *api_key = getenv("DD_API_KEY");
if (!api_key) {
printf("DD_API_KEY environment variable is no set\n");
return 1;
}

const auto service = argv[1];

const ddprof_ffi_ValueType wall_time = {
.type_ = to_slice_c_char("wall-time"),
.unit = to_slice_c_char("nanoseconds"),
};

const ddprof_ffi_Slice_value_type sample_types = {&wall_time, 1};
const ddprof_ffi_Period period = {wall_time, 60};
Holder<ddprof_ffi_Profile> profile{ddprof_ffi_Profile_new(sample_types, &period)};

ddprof_ffi_Line root_line = {
.function = {
.name = to_slice_c_char("{main}"),
.filename = to_slice_c_char("/srv/example/index.php")},
.line = 0,
};

ddprof_ffi_Location root_location = {
// yes, a zero-initialized mapping is valid
.mapping = {},
.lines = {&root_line, 1},
};

int64_t value = 10;
const ddprof_ffi_Label label = {
.key = to_slice_c_char("language"),
.str = to_slice_c_char("php"),
};
ddprof_ffi_Sample sample = {
.locations = {&root_location, 1},
.values = {&value, 1},
.labels = {&label, 1},
};
ddprof_ffi_Profile_add(profile, sample);

Holder<ddprof_ffi_EncodedProfile> encoded_profile{ddprof_ffi_Profile_serialize(profile)};

ddprof_ffi_EndpointV3 endpoint = ddprof_ffi_EndpointV3_agentless(to_byteslice("datad0g.com"), to_byteslice(api_key));
ddprof_ffi_Tag tags[] = {{to_byteslice("service"), to_byteslice(service)}};
ddprof_ffi_NewProfileExporterV3Result exporter_new_result =
ddprof_ffi_ProfileExporterV3_new(
to_byteslice("native"), ddprof_ffi_Slice_tag{.ptr = tags, .len = sizeof(tags) / sizeof(tags[0])}, endpoint);

if (exporter_new_result.tag == DDPROF_FFI_NEW_PROFILE_EXPORTER_V3_RESULT_ERR)
{
print_error("Failed to create exporter: ", exporter_new_result.err);
return 1;
}

Holder<ddprof_ffi_ProfileExporterV3> exporter{exporter_new_result.ok};

ddprof_ffi_Buffer profile_buffer = {
.ptr = encoded_profile->buffer.ptr,
.len = encoded_profile->buffer.len,
.capacity = encoded_profile->buffer.capacity,
};

ddprof_ffi_File files_[] = {{
.name = to_byteslice("auto.pprof"),
.file = &profile_buffer,
}};

ddprof_ffi_Slice_file files = {
.ptr = files_, .len = sizeof files_ / sizeof *files_};

ddprof_ffi_Request *request = ddprof_ffi_ProfileExporterV3_build(
exporter, encoded_profile->start, encoded_profile->end, files, 10000);

ddprof_ffi_SendResult send_result = ddprof_ffi_ProfileExporterV3_send(exporter, request);
if (send_result.tag == DDPROF_FFI_SEND_RESULT_FAILURE)
{
print_error("Failed to send profile: ", send_result.failure);
return 1;
}
else
{
printf("Response code: %d\n", send_result.http_response.code);
}
}
24 changes: 21 additions & 3 deletions ffi-build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ mkdir -v -p "$destdir/include/ddprof" "$destdir/lib/pkgconfig" "$destdir/cmake"

version=$(awk -F\" '$1 ~ /^version/ { print $2 }' < ddprof-ffi/Cargo.toml)
target="$(rustc -vV | awk '/^host:/ { print $2 }')"
shared_library_suffix=".so"
remove_rpath=0

# Rust provides this note about the link libraries:
# note: Link against the following native artifacts when linking against this
Expand All @@ -26,10 +28,13 @@ case "$target" in
"x86_64-alpine-linux-musl"|"aarch64-alpine-linux-musl")
expected_native_static_libs=" -lssp_nonshared -lgcc_s -lc"
native_static_libs=" -lssp_nonshared -lc"
# on alpine musl, Rust adds some weird runpath to cdylibs
remove_rpath=1
;;
"x86_64-apple-darwin")
expected_native_static_libs=" -framework Security -liconv -lSystem -lresolv -lc -lm -liconv"
native_static_libs="${expected_native_static_libs}"
shared_library_suffix=".dylib"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking the mac os builds !

;;
"x86_64-unknown-linux-gnu"|"aarch64-unknown-linux-gnu")
expected_native_static_libs=" -ldl -lrt -lpthread -lgcc_s -lc -lm -lrt -lpthread -lutil -ldl -lutil"
Expand All @@ -43,9 +48,12 @@ esac

echo "Recognized platform '${target}'. Adding libs: ${native_static_libs}"
sed < ddprof_ffi.pc.in "s/@DDProf_FFI_VERSION@/${version}/g" \
| sed "s/@DDProf_FFI_LIBRARIES@/${native_static_libs}/g" \
> "$destdir/lib/pkgconfig/ddprof_ffi.pc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The concept of "native static libs" doesn't apply to a shared object. There will be dependencies, of course, but this isn't how we should be populating them for the .so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I did put them in Libs.private when I had still hope to be able to make a single pkg-config file for both shared and static library, but since I could not do it, it's better to completely remove them for the shared library.


sed < ddprof_ffi-static.pc.in "s/@DDProf_FFI_VERSION@/${version}/g" \
| sed "s/@DDProf_FFI_LIBRARIES@/${native_static_libs}/g" \
> "$destdir/lib/pkgconfig/ddprof_ffi-static.pc"

sed < cmake/DDProfConfig.cmake.in \
> "$destdir/cmake/DDProfConfig.cmake" \
"s/@DDProf_FFI_LIBRARIES@/${native_static_libs}/g"
Expand All @@ -54,9 +62,19 @@ cp -v LICENSE LICENSE-3rdparty.yml NOTICE "$destdir/"

export RUSTFLAGS="${RUSTFLAGS:- -C relocation-model=pic}"

echo "Building the libddprof_ffi.a library (may take some time)..."
echo "Building the libddprof_ffi library (may take some time)..."
cargo build --release --target "${target}"
cp -v "target/${target}/release/libddprof_ffi.a" "$destdir/lib/"
cp -v "target/${target}/release/libddprof_ffi.a" "target/${target}/release/libddprof_ffi${shared_library_suffix}" "$destdir/lib/"

if [[ "$remove_rpath" -eq 1 ]]; then
patchelf --remove-rpath "$destdir/lib/libddprof_ffi${shared_library_suffix}"
fi

# objcopy might not be available on macOS
if command -v objcopy > /dev/null; then
# Remove .llvmbc section which is not useful for clients
objcopy --remove-section .llvmbc "$destdir/lib/libddprof_ffi.a"
fi

echo "Checking that native-static-libs are as expected for this platform..."
cd ddprof-ffi
Expand Down