[PROF-8139] Upgrade to libdatadog 4#3104
Merged
Merged
Conversation
ivoanjo
added a commit
to DataDog/libdatadog
that referenced
this pull request
Sep 4, 2023
**What does this PR do?** This PR includes the changes documented in the "Releasing a new version to rubygems.org" part of the README: <https://github.com/datadog/libdatadog/tree/main/ruby#releasing-a-new-version-to-rubygemsorg> **Motivation:** Enable Ruby to use libdatadog v4.0.0. **Additional Notes:** N/A **How to test the change?** I've tested this release locally against the changes in DataDog/dd-trace-rb#3104 . As a reminder, new libdatadog releases don't get automatically picked up by Ruby, so the PR that bumps the Ruby profiler will also test this release against all supported Ruby versions.
2 tasks
ivoanjo
added a commit
to DataDog/libdatadog
that referenced
this pull request
Sep 4, 2023
**What does this PR do?** This PR includes the changes documented in the "Releasing a new version to rubygems.org" part of the README: <https://github.com/datadog/libdatadog/tree/main/ruby#releasing-a-new-version-to-rubygemsorg> **Motivation:** Enable Ruby to use libdatadog v4.0.0. **Additional Notes:** N/A **How to test the change?** I've tested this release locally against the changes in DataDog/dd-trace-rb#3104 . As a reminder, new libdatadog releases don't get automatically picked up by Ruby, so the PR that bumps the Ruby profiler will also test this release against all supported Ruby versions.
Codecov Report
@@ Coverage Diff @@
## master #3104 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 1323 1323
Lines 75187 75232 +45
Branches 3430 3430
=======================================
+ Hits 73804 73850 +46
+ Misses 1383 1382 -1
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ivoanjo
marked this pull request as ready for review
September 4, 2023 13:00
ekump
approved these changes
Sep 5, 2023
ekump
left a comment
ekump
left a comment
Contributor
There was a problem hiding this comment.
My C is a bit rusty, but LGTM.
GustavoCaso
reviewed
Sep 5, 2023
Comment on lines
390
to
393
Contributor
There was a problem hiding this comment.
Could we use reset_profile here?
Maybe the function reset_profile could return if the profiler successfully rest and if not the caller can choose what to do
GustavoCaso
reviewed
Sep 5, 2023
Contributor
There was a problem hiding this comment.
This was not mentioned in the breaking changes on the PR description. Is this expected?
Member
Author
There was a problem hiding this comment.
Yes! This was slightly folded under
A bunch of APIs started returning ddog_prof_Profile_Result.
The API changed to no longer return an AddResult and use a regular Result (diff of profiling.h header):
-struct ddog_prof_Profile_AddResult ddog_prof_Profile_add(struct ddog_prof_Profile *profile,
+struct ddog_prof_Profile_Result ddog_prof_Profile_add(struct ddog_prof_Profile *profile,
struct ddog_prof_Sample sample);Error handling on result structure is similar, but the constant to indicate the error is different.
**What does this PR do?** This PR upgrades dd-trace-rb to use libdatadog 4. This release includes a number of memory footprint improvements, but there were also a few breaking API changes, which needed adjustments in our code. **Motivation:** Use the latest libdatadog release. **Additional Notes:** The backwards incompatible changes were mainly these 3: * Instead of having a `ddog_prof_Location` be able to include multiple `ddog_prof_Line`s, now a location can only include a single line. In our code we already always used 1-line-per-location only, so we only had to adjust to the `ddog_pprof_Line` getting removed. * The `ddog_prof_Profile_new` now returns a `ddog_prof_Profile`, instead of a pointer; conversely, a bunch of other APIs now want a pointer to a `ddog_prof_Profile` instead of the `ddog_prof_Profile` directly. * A bunch of APIs started returning `ddog_prof_Profile_Result`. **How to test the change?** Our existing test coverage already exercises all of these code paths.
ivoanjo
force-pushed
the
ivoanjo/prof-8139-upgrade-libdatadog4
branch
from
September 6, 2023 08:38
b187f48 to
4d837ef
Compare
Member
Author
|
I've rebased this on top of current master without any other changes so as to incorporate the new CI job which is a requirement for merging. |
This is slightly inconsistent with the error result when profile serialization fails, but a reset failing is way less likely, and this allows us to simplify the code.
GustavoCaso
approved these changes
Sep 6, 2023
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
This PR upgrades dd-trace-rb to use libdatadog 4. This release includes a number of memory footprint improvements, but there were also a few breaking API changes, which needed adjustments in our code.
Motivation:
Use the latest libdatadog release.
Additional Notes:
✅ Done
I'm opening this PR as a draft until the libdatadog 4 release is up on rubygems.org, but I've already tested it locally.✅ Done
After libdatadog 4 gets released on rubygems.org, we should update the appraisals and then mark the PR as non-draft.The backwards incompatible changes were mainly these 3:
Instead of having a
ddog_prof_Locationbe able to include multipleddog_prof_Lines, now a location can only include a single line.In our code we already always used 1-line-per-location only, so we only had to adjust to the
ddog_pprof_Linegetting removed.The
ddog_prof_Profile_newnow returns addog_prof_Profile, instead of a pointer; conversely, a bunch of other APIs now want a pointer to addog_prof_Profileinstead of theddog_prof_Profiledirectly.A bunch of APIs started returning
ddog_prof_Profile_Result.How to test the change?
Our existing test coverage already exercises all of these code paths.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.