[profiling] Use the new FFI macros and types #797
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danielsn
force-pushed
the
dsn/profiler-use-macros
branch
from
31cd819 to
1374f98
Compare
BenchmarksComparisonBenchmark execution time: 2024-12-10 22:50:54 Comparing candidate commit ef10d90 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 51 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #797 +/- ##
==========================================
+ Coverage 70.95% 71.04% +0.09%
==========================================
Files 313 314 +1
Lines 45747 45665 -82
==========================================
- Hits 32458 32441 -17
+ Misses 13289 13224 -65
|
danielsn
force-pushed
the
dsn/profiler-use-macros
branch
from
6680602 to
4a92b26
Compare
danielsn
force-pushed
the
dsn/profiler-use-macros
branch
from
2ffb63d to
ef10d90
Compare
There was a problem hiding this comment.
This has breaking FFI changes, which I didn't expect with a "refactor" title, but that's okay. Just wanted to call a bit of attention there, and we should write something about in this PR description.
I think I need to open this up in an editor to see some of it, diff-view isn't great sometimes.
There was a problem hiding this comment.
I don't care about the C++ changes, if someone thinks it's better, it's fine to me.
danielsn
changed the title
Updated the PR description |
Comment on lines
+17
to
+18
| 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) { |
There was a problem hiding this comment.
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_NewResultaliased toddog_prof_Result_HandleProfileddog_prof_Profilealiased toddog_prof_Handle_Profile
Not sure what to do about the enum. Maybe @ivoanjo has thoughts?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like these being moved into their own file 👍🏻
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Uses the macros introduced in #752 in the profiler-ffi
Motivation
DRY: Don't Repeat Yourself ;)
Additional Notes
This has breaking FFI changes (changes some types and type names) but the upgrade should be pretty mechanical clientside.
How to test the change?
This is a refactor change