Skip to content

ekump/APMSP-2151 create ddsketch ffi crate #1135

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.

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ jobs:
env:
RUSTFLAGS: "${{ matrix.flags }}"
run: |
cargo run --bin release --features profiling,telemetry,data-pipeline,symbolizer,crashtracker,library-config,log --release -- --out $LIBDD_OUTPUT_FOLDER
cargo run --bin release --features profiling,telemetry,data-pipeline,symbolizer,crashtracker,library-config,log,ddsketch --release -- --out $LIBDD_OUTPUT_FOLDER

- name: 'Publish libdatadog'
uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # 4.6.1
Expand Down
10 changes: 10 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ members = [
"data-pipeline",
"data-pipeline-ffi",
"ddsketch",
"ddsketch-ffi",
"tinybytes",
"dogstatsd-client",
"datadog-log",
Expand Down
2 changes: 1 addition & 1 deletion LICENSE-3rdparty.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
root_name: builder, build_common, tools, datadog-alloc, datadog-crashtracker, ddcommon, ddtelemetry, datadog-ddsketch, datadog-crashtracker-ffi, ddcommon-ffi, datadog-ipc, datadog-ipc-macros, tarpc, tarpc-plugins, tinybytes, spawn_worker, cc_utils, datadog-library-config, datadog-library-config-ffi, datadog-live-debugger, datadog-live-debugger-ffi, datadog-profiling, datadog-profiling-protobuf, datadog-profiling-ffi, data-pipeline-ffi, data-pipeline, datadog-trace-protobuf, datadog-trace-utils, datadog-trace-normalization, dogstatsd-client, datadog-log-ffi, datadog-log, ddtelemetry-ffi, symbolizer-ffi, datadog-profiling-replayer, datadog-remote-config, datadog-sidecar, datadog-sidecar-macros, datadog-sidecar-ffi, datadog-trace-obfuscation, datadog-tracer-flare, sidecar_mockgen, test_spawn_from_lib
root_name: builder, build_common, tools, datadog-alloc, datadog-crashtracker, ddcommon, ddtelemetry, datadog-ddsketch, datadog-crashtracker-ffi, ddcommon-ffi, datadog-ipc, datadog-ipc-macros, tarpc, tarpc-plugins, tinybytes, spawn_worker, cc_utils, datadog-library-config, datadog-library-config-ffi, datadog-live-debugger, datadog-live-debugger-ffi, datadog-profiling, datadog-profiling-protobuf, datadog-profiling-ffi, data-pipeline-ffi, data-pipeline, datadog-trace-protobuf, datadog-trace-utils, datadog-trace-normalization, dogstatsd-client, datadog-log-ffi, datadog-log, ddsketch-ffi, ddtelemetry-ffi, symbolizer-ffi, datadog-profiling-replayer, datadog-remote-config, datadog-sidecar, datadog-sidecar-macros, datadog-sidecar-ffi, datadog-trace-obfuscation, datadog-tracer-flare, sidecar_mockgen, test_spawn_from_lib
third_party_libraries:
- package_name: addr2line
package_version: 0.24.2
Expand Down
3 changes: 2 additions & 1 deletion build-profiling-ffi.sh
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ FEATURES=(
"datadog-profiling-ffi/demangler"
"datadog-library-config-ffi"
"datadog-log-ffi"
"ddsketch-ffi"
)
if [[ "$symbolizer" -eq 1 ]]; then
FEATURES+=("symbolizer")
Expand Down Expand Up @@ -237,7 +238,7 @@ echo "Generating $destdir/include/libdatadog headers..."
rm -r $destdir/include/datadog/
mkdir $destdir/include/datadog/

CBINDGEN_HEADERS="common.h profiling.h telemetry.h crashtracker.h data-pipeline.h library-config.h log.h"
CBINDGEN_HEADERS="common.h profiling.h telemetry.h crashtracker.h data-pipeline.h library-config.h log.h ddsketch.h"
# When optional features are added, don't forget to also include the headers here
case $ARG_FEATURES in
esac
Expand Down
1 change: 1 addition & 0 deletions builder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ data-pipeline = []
symbolizer = []
library-config = []
log = []
ddsketch = []

[lib]
bench = false
Expand Down
2 changes: 2 additions & 0 deletions builder/src/bin/release.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ pub fn main() {
f.push("datadog-library-config-ffi".to_string());
#[cfg(feature = "log")]
f.push("datadog-log-ffi".to_string());
#[cfg(feature = "ddsketch")]
f.push("ddsketch-ffi".to_string());
f
};

Expand Down
2 changes: 2 additions & 0 deletions builder/src/profiling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ impl Profiling {
headers.push("library-config.h");
#[cfg(feature = "log")]
headers.push("log.h");
#[cfg(feature = "ddsketch")]
headers.push("ddsketch.h");

let mut origin_path: PathBuf = [&self.source_include, "dummy.h"].iter().collect();
let mut target_path: PathBuf = [&self.target_include, "dummy.h"].iter().collect();
Expand Down
2 changes: 2 additions & 0 deletions datadog-profiling-ffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ crashtracker-receiver = ["crashtracker-ffi", "datadog-crashtracker-ffi/receiver"
demangler = ["crashtracker-ffi", "datadog-crashtracker-ffi/demangler"]
datadog-library-config-ffi = ["dep:datadog-library-config-ffi"]
ddcommon-ffi = ["dep:ddcommon-ffi"]
ddsketch-ffi = ["dep:ddsketch-ffi"]

[build-dependencies]
build_common = { path = "../build-common" }
Expand All @@ -43,6 +44,7 @@ ddcommon = { path = "../ddcommon" }
ddcommon-ffi = { path = "../ddcommon-ffi", default-features = false, optional = true }
ddtelemetry-ffi = { path = "../ddtelemetry-ffi", default-features = false, optional = true, features = ["expanded_builder_macros"] }
datadog-log-ffi = { path = "../datadog-log-ffi", default-features = false, optional = true }
ddsketch-ffi = { path = "../ddsketch-ffi", default-features = false, optional = true }
function_name = "0.3.0"
futures = { version = "0.3", default-features = false }
http-body-util = "0.1"
Expand Down
5 changes: 5 additions & 0 deletions datadog-profiling-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ pub use ddtelemetry_ffi::*;
#[allow(unused_imports)]
pub use data_pipeline_ffi::*;

// re-export ddsketch ffi
#[cfg(feature = "ddsketch-ffi")]
#[allow(unused_imports)]
pub use ddsketch_ffi::*;

// re-export library-config ffi
#[cfg(feature = "datadog-library-config-ffi")]
pub use datadog_library_config_ffi::*;
Expand Down
36 changes: 36 additions & 0 deletions ddcommon-ffi/src/cstr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,30 @@ impl CString {
Ok(Self::from_std(std::ffi::CString::new(t)?))
}

/// Creates a new `CString` from the given input, or returns an empty `CString`
/// if the input contains null bytes.
Comment on lines +63 to +64
Copy link
Member

Choose a reason for hiding this comment

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

In what situations would we have weird strings with null bytes in the middle? 👀

///
/// This method will never panic, as an empty string is guaranteed to not contain
/// null bytes and can always be converted to a `CString`.
///
/// # Examples
///
/// ```
/// use ddcommon_ffi::CString;
///
/// let good = CString::new_or_empty("hello world");
/// assert_eq!(good.as_cstr().into_std().to_str().unwrap(), "hello world");
///
/// let bad = CString::new_or_empty("hello\0world");
/// assert_eq!(bad.as_cstr().into_std().to_str().unwrap(), "");
/// ```
pub fn new_or_empty<T: Into<Vec<u8>>>(t: T) -> Self {
Self::new(t).unwrap_or_else(|_| {
#[allow(clippy::unwrap_used)]
Self::new("").unwrap()
})
}

pub fn as_cstr(&self) -> CStr<'_> {
CStr {
ptr: self.ptr,
Expand Down Expand Up @@ -118,6 +142,18 @@ mod tests {
assert_eq!(s.as_cstr().into_std().to_str().unwrap(), "hello");
}

#[test]
fn test_cstring_new_or_empty() {
let good = CString::new_or_empty("hello world");
assert_eq!(good.as_cstr().into_std().to_str().unwrap(), "hello world");

let bad = CString::new_or_empty("hello\0world");
assert_eq!(bad.as_cstr().into_std().to_str().unwrap(), "");

let empty = CString::new_or_empty("");
assert_eq!(empty.as_cstr().into_std().to_str().unwrap(), "");
}

#[test]
fn test_raw_cstr() {
let s: &'static [u8] = b"abc\0";
Expand Down
26 changes: 26 additions & 0 deletions ddsketch-ffi/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/
# SPDX-License-Identifier: Apache-2.0

[package]
name = "ddsketch-ffi"
edition.workspace = true
version.workspace = true
rust-version.workspace = true
license.workspace = true

[lib]
crate-type = ["lib", "staticlib", "cdylib"]
bench = false

[features]
default = ["cbindgen"]
cbindgen = ["build_common/cbindgen", "ddcommon-ffi/cbindgen"]

[build-dependencies]
build_common = { path = "../build-common" }

[dependencies]
datadog-ddsketch = { path = "../ddsketch" }
ddcommon-ffi = { path = "../ddcommon-ffi", default-features = false }

[dev-dependencies]
11 changes: 11 additions & 0 deletions ddsketch-ffi/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/
// SPDX-License-Identifier: Apache-2.0

extern crate build_common;

use build_common::generate_and_configure_header;

fn main() {
let header_name = "ddsketch.h";
generate_and_configure_header(header_name);
}
35 changes: 35 additions & 0 deletions ddsketch-ffi/cbindgen.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/
# SPDX-License-Identifier: Apache-2.0

language = "C"
cpp_compat = true
tab_width = 2
header = """// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/
// SPDX-License-Identifier: Apache-2.0
"""
include_guard = "DDOG_DDSKETCH_H"
style = "both"
pragma_once = true
no_includes = true
sys_includes = ["stdbool.h", "stddef.h", "stdint.h"]
includes = ["common.h"]

[parse]
parse_deps = true
include = ["ddcommon-ffi", "datadog-ddsketch"]

[export]
include = ["ddsketch-ffi"]
prefix = "ddog_"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be ddog_ddsketch by default?

Suggested change
prefix = "ddog_"
prefix = "ddog_ddsketch"

Copy link
Contributor

Choose a reason for hiding this comment

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

Or possibly just ddsketch. Like if we were greenfielding this thing I'd say ddog_sketch but ddsketch has been used for years as a name and I believe that includes papers and such.

renaming_overrides_prefixing = true

[export.rename]
"DDSketchError" = "DDSketchError"
"DDSketchErrorCode" = "DDSketchErrorCode"

[fn]
must_use = "DDOG_CHECK_RETURN"

[enum]
prefix_with_name = true
rename_variants = "ScreamingSnakeCase"
97 changes: 97 additions & 0 deletions ddsketch-ffi/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/
// SPDX-License-Identifier: Apache-2.0

use ddcommon_ffi::CString;
use std::fmt::Display;

/// Represent error codes that `DDSketchError` struct can hold
#[repr(C)]
#[derive(Copy, Clone, Debug, PartialEq)]
pub enum DDSketchErrorCode {
InvalidArgument,
InvalidInput,
Internal,
}

impl Display for DDSketchErrorCode {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::InvalidArgument => write!(f, "Invalid argument provided"),
Self::InvalidInput => write!(f, "Invalid input"),
Self::Internal => write!(f, "Internal error"),
}
}
}

/// Structure that contains error information that DDSketch FFI API can return.
#[repr(C)]
#[derive(Debug)]
pub struct DDSketchError {
pub code: DDSketchErrorCode,
pub msg: CString,
}
Comment on lines +29 to +32
Copy link
Member

Choose a reason for hiding this comment

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

If we do keep DDSketchError (see my note below), I believe it should be renamed ddog_ddsketch_DDSketchError / ddog_ddsketch_DDSketchErrorCode, right now it's missing the prefixes in the .h files 👀


impl DDSketchError {
pub fn new(code: DDSketchErrorCode, msg: &str) -> Self {
Self {
code,
msg: CString::new_or_empty(msg),
}
}
}
Comment on lines +26 to +41
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about the need for this -- is ddcommon_ffi::Result and ddcommon_ffi::Error not usable for the same purpose?


impl From<Box<dyn std::error::Error>> for DDSketchError {
fn from(value: Box<dyn std::error::Error>) -> Self {
DDSketchError::new(DDSketchErrorCode::Internal, &value.to_string())
}
}

/// Frees `error` and all its contents. After being called error will not point to a valid memory
/// address so any further actions on it could lead to undefined behavior.
///
/// # Safety
///
/// Only pass null or a pointer to a valid DDSketchError created by this library.
#[no_mangle]
pub unsafe extern "C" fn ddog_ddsketch_error_free(error: Option<Box<DDSketchError>>) {
drop(error)
}
Comment on lines +56 to +58
Copy link
Member

Choose a reason for hiding this comment

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

I'm hoping we may be able to move away from the ddsketch-specific error code; but if not, I think this should be _drop, not _free, for consistency with other apis?


#[cfg(test)]
mod tests {
use super::*;

#[test]
fn constructor_test() {
let code = DDSketchErrorCode::InvalidArgument;
let error = Box::new(DDSketchError::new(code, &code.to_string()));

assert_eq!(error.code, DDSketchErrorCode::InvalidArgument);
let msg = error.msg.as_cstr().into_std().to_str().unwrap();
assert_eq!(msg, DDSketchErrorCode::InvalidArgument.to_string());
}

#[test]
fn destructor_test() {
let code = DDSketchErrorCode::InvalidArgument;
let error = Box::new(DDSketchError::new(code, &code.to_string()));

assert_eq!(error.code, DDSketchErrorCode::InvalidArgument);
let msg = error.msg.as_cstr().into_std().to_str().unwrap();
assert_eq!(msg, DDSketchErrorCode::InvalidArgument.to_string());

unsafe { ddog_ddsketch_error_free(Some(error)) };
}

#[test]
fn test_error_with_null_bytes() {
let code = DDSketchErrorCode::InvalidInput;
let error = Box::new(DDSketchError::new(code, "Error with\0null bytes"));

assert_eq!(error.code, DDSketchErrorCode::InvalidInput);
let msg = error.msg.as_cstr().into_std().to_str().unwrap();
assert_eq!(msg, ""); // Should fall back to empty string

unsafe { ddog_ddsketch_error_free(Some(error)) };
}
Comment on lines +86 to +96
Copy link
Member

Choose a reason for hiding this comment

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

Re: my comment in ddcommon for the null bytes in the middle of the string, I don't quite understand how this would happen in normal code? 👀

}
Loading
Loading