Remove unused Exporter APIs and adjust Slices#35
Conversation
CharSlice -> intended to be valid utf-8 string. ByteSlice -> not intended to be valid utf-8 string (like binary data).
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
Other than the broken example, LGTM 👍
| pub struct Tag<'a> { | ||
| name: ByteSlice<'a>, | ||
| name: CharSlice<'a>, | ||
| value: ByteSlice<'a>, | ||
| } |
There was a problem hiding this comment.
I think the same reasoning for a tag name would apply to the value as well, e.g. both should probably be CharSlice.
| #[repr(C)] | ||
| pub enum EndpointV3<'a> { | ||
| Agent(ByteSlice<'a>), | ||
| Agentless(ByteSlice<'a>, ByteSlice<'a>), | ||
| Agent(CharSlice<'a>), | ||
| Agentless(CharSlice<'a>, CharSlice<'a>), | ||
| } |
There was a problem hiding this comment.
I noticed this broke one of the FFI examples:
/Users/ivo.anjo/datadog/x-second-libddprof/examples/ffi/exporter.cpp:109:38: error: no matching function for call to
'ddprof_ffi_EndpointV3_agentless'
ddprof_ffi_EndpointV3 endpoint = ddprof_ffi_EndpointV3_agentless(to_byteslice("datad0g.com"), to_byteslice(api_key));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ivo.anjo/datadog/x-second-libddprof/build-macos/include/ddprof/ffi.h:329:30: note: candidate function not viable: no known
conversion from 'ddprof_ffi_ByteSlice' (aka 'ddprof_ffi_Slice_u8') to 'ddprof_ffi_CharSlice' (aka 'ddprof_ffi_Slice_c_char')
for 1st argument
struct ddprof_ffi_EndpointV3 ddprof_ffi_EndpointV3_agentless(ddprof_ffi_CharSlice site,
^
/Users/ivo.anjo/datadog/x-second-libddprof/examples/ffi/exporter.cpp:110:31: error: no viable conversion from
'ddprof_ffi_ByteSlice' (aka 'ddprof_ffi_Slice_u8') to 'const char *'
ddprof_ffi_Tag tags[] = {{to_byteslice("service"), to_byteslice(service)}};
^~~~~~~~~~~~~~~~~~~~~~~
/Users/ivo.anjo/datadog/x-second-libddprof/examples/ffi/exporter.cpp:110:56: error: no viable conversion from
'ddprof_ffi_ByteSlice' (aka 'ddprof_ffi_Slice_u8') to 'size_t' (aka 'unsigned long')
ddprof_ffi_Tag tags[] = {{to_byteslice("service"), to_byteslice(service)}};
^~~~~~~~~~~~~~~~~~~~~
/Users/ivo.anjo/datadog/x-second-libddprof/examples/ffi/exporter.cpp:124:17: error: no viable conversion from
'ddprof_ffi_ByteSlice' (aka 'ddprof_ffi_Slice_u8') to 'const char *'
.name = to_byteslice("auto.pprof"),
One thing I've been thinking is that it would perhaps make sense to build the examples when we run ffi-build.sh, to avoid bitrotting them (and as an extra "test" that API changes are deliberate).
There was a problem hiding this comment.
Interesting; I built this example and it was fine. Now I need to figure out where I screwed up ^_^
| // Use to represent strings -- should be valid UTF-8. | ||
| type CharSlice<'a> = crate::Slice<'a, c_char>; | ||
|
|
||
| /// Use to represent bytes -- does not need to be valid UTF-8. | ||
| type ByteSlice<'a> = crate::Slice<'a, u8>; |
There was a problem hiding this comment.
I'm not against it per se, but -- why should libddprof care if it is given non-utf-8 data? What are we gaining by distinguishing both types?
There was a problem hiding this comment.
Datadog cares about UTF-8 in certain cases, like tag names. Generally we need UTF-8 for string-like things or else it's probably garbage/error. However, certain things are not UTF-8, like many binary data formats. I think it's important that we separate things that are bytes and not characters in the human sense of things.
There was a problem hiding this comment.
That's fair enough I guess. I think we could probably handle non-utf8-ness at other places downstream, but I think either way is fine, and we can always revisit as needed. 👍
…PIs, and adjust Slices (#34) * Fix clippy lints and expose dependencies of API hyper::Uri and chrono::{DateTime, Utc} are used in the public Rust API, so we need to `pub use` them so consumers can use these types and for them to be compatible. * Update for Rust 1.56 * Downgrade ux to 0.1.3 to avoid const new 0.1.4 uses const new, which requires Rust 1.57+ * Fix 3rdparty license paths and clippy lint * Fix license file * Remove unused Exporter APIs and adjust Slices (#35) * Remove unused Exporter APIs and adjust Slices CharSlice -> intended to be valid utf-8 string. ByteSlice -> not intended to be valid utf-8 string (like binary data). * Fix quoting for CMake * Fix exporter example, address CR
2 participants
What does this PR do?
Removes unused legacy Exporter APIs.
Adds CharSlice (which is
Slice<_, c_char>) to FFI. Moves ByteSliceinto
ddprof-ffi/src/lib.rsas well. Adjust some structs and APIs touse CharSlice instead of ByteSlice.
Adjust quoting for CMake project.
Motivation
Nobody is using the legacy FFI Exporter APIs so let's remove them,
which reduces the size a bit and helps avoid confusion.
Ivo was rightfully complaining about how we have
Slice<_, c_char>andByteSlice<_>and aren't consistent about where to use what. This PRadds guidance:
CharSlice -> intended to be valid utf-8 string.
ByteSlice -> not intended to be valid utf-8 string (like binary data).
Additional Notes
When seeing some kind of char or byte slice, think whether it
could/should be binary data. If so, then pick ByteSlice; if it should
be validated UTF-8 or is a "string", then pick CharSlice.
How to test the change?
This is going to break things, I'm sure. Edit: Turns out, the FFI
examples were not using a struct name and didn't require any changes!
Users may not get quite so lucky, but the changes shouldn't be too bad.