Return SerializeResult for serializing a profile#42
Conversation
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
Thanks for improving this!
I can't speak too much for the C++ black magic being switched for slightly different C++ black magic but it seems reasonable as well.
| [[package]] | ||
| name = "ddprof" | ||
| version = "0.4.0-rc.1" | ||
| version = "0.5.0-rc.1" | ||
| dependencies = [ |
There was a problem hiding this comment.
Minor: I already merged this change to master in #39 so I think this is going to conflict.
| #[export_name = "ddprof_ffi_NewProfileExporterV3Result_drop"] | ||
| pub unsafe extern "C" fn new_profile_exporter_v3_result_drop(result: NewProfileExporterV3Result) { |
There was a problem hiding this comment.
Minor: As we're going to rename this for consistency anyway (thanks!) may I suggest calling it ddprof_ffi_ProfileExporterV3Result_drop?
Indeed the type is called NewProfileExporterV3Result but that's a bit of a mouthful and we could rename it later as well :) (probably not in this PR, to avoid getting too off-topic). I actually think that since we're not too worried about backwards-compatibility right now we could even just rename it back to ProfileExporter without New or V3 suffixes.
There was a problem hiding this comment.
I'll leave it for now -- we can shorten and clean this up later (like next PR, possibly).
2e39741 to
76aad26
Compare
The Vec type is a generalization of Buffer. In particular, this PR exposes an API to create and manage Vec<Tag> and replaces existing uses of Buffer with Vec<u8>. It includes a helper to parse DD_TAGS as well. One of the APIs, ddprof_ffi_Buffer_reset, was removed. Previously this was required when using the failure case of the SendResult type, but now there is ddprof_ffi_SendResult_drop, which should be used instead.
1183de2 to
7c0ed24
Compare
Previously it couldn't handle a tags like: env:staging:east value Tag messages on failure have improved. As part of adding tests, added more trait support to Slice and CharSlice so they can be used in equivalence statements. As part of doing that, I realized Slice could implement IntoIterator and that it cleaned up 3+ places where we call .into_slice().iter(). I also removed unsafe from `into_slice`. Technically, it could still fail such as if the user creates a slice of length 4 but only has 2 elements, but this is a user error that cannot really be checked.
7c0ed24 to
3c4e4eb
Compare
This removes the direct `eprintln` usage and allows the caller to choose what to do with it instead. Adds these FFI functions: - `ddprof_ffi_SerializeResult_drop` - `ddprof_ffi_Vec_u8_as_slice`. The EncodedProfile has a Vec<u8> but the struct ddprof_ffi_File requires a Slice<u8>. And removes `ddprof_ffi_EncodedProfile_delete`.
3c4e4eb to
f7dd75b
Compare
2 participants
What does this PR do?
Changes:
ddprof_ffi_Profile_serializereturnsddprof_ffi_SerializeResultAdds:
ddprof_ffi_SerializeResult_dropddprof_ffi_Vec_u8_as_sliceRemoves
ddprof_ffi_EncodedProfile_delete.Cleans up exporter example. With the newer APIs, fewer helpers are
needed.
Motivation
Ivo wrote a doc for API changes he wants and I had a moment and was
already In The Groove. These changes make it easier for the caller to
choose what to do with errors, and make it easier to convert the
EncodeProfile's buffer field into a slice.
Additional Notes
I like working with Ivo.
How to test the change?
Observe that existing code breaks, adjust code paths around the above
changes, and be on your way. I fixed up the exporter example so you can
refer to that too.