[PROF-8139] Upgrade to libdatadog 4 #3104

ivoanjo · 2023-09-04T09:45:57Z

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_Location be able to include multiple ddog_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_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.

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

**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-commenter · 2023-09-04T11:07:52Z

Codecov Report

Merging #3104 (80d8cec) into master (3e3ad39) will increase coverage by 0.00%.
Report is 15 commits behind head on master.
The diff coverage is 100.00%.

@@           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

Files Changed	Coverage Δ
...iling_native_extension/native_extension_helpers.rb	`96.90% <100.00%> (ø)`

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ekump

My C is a bit rusty, but LGTM.

GustavoCaso · 2023-09-05T14:53:31Z

ext/ddtrace_profiling_native_extension/stack_recorder.c

+  ddog_prof_Profile_Result reset_result = ddog_prof_Profile_reset(args.profile, NULL /* start_time is optional */ );
+  if (reset_result.tag == DDOG_PROF_PROFILE_RESULT_ERR) {
+    return rb_ary_new_from_args(2, error_symbol, rb_sprintf("Failed to reset profile: %"PRIsVALUE, get_error_details_and_drop(&reset_result.err)));
  }


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

Simplified as 80d8cec

GustavoCaso · 2023-09-05T14:54:34Z

ext/ddtrace_profiling_native_extension/stack_recorder.c

@@ -428,7 +430,7 @@ void record_sample(VALUE recorder_instance, ddog_prof_Slice_Location locations,

  sampler_unlock_active_profile(active_slot);

-  if (result.tag == DDOG_PROF_PROFILE_ADD_RESULT_ERR) {
+  if (result.tag == DDOG_PROF_PROFILE_RESULT_ERR) {


This was not mentioned in the breaking changes on the PR description. Is this expected?

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 · 2023-09-06T08:38:34Z

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.

github-actions bot added the profiling Involves Datadog profiling label Sep 4, 2023

ivoanjo mentioned this pull request Sep 4, 2023

[PROF-8139] Package libdatadog v4.0.0 for Ruby DataDog/libdatadog#238

Merged

2 tasks

ivoanjo marked this pull request as ready for review September 4, 2023 13:00

ivoanjo requested a review from a team September 4, 2023 13:00

ekump approved these changes Sep 5, 2023

View reviewed changes

GustavoCaso reviewed Sep 5, 2023

View reviewed changes

ivoanjo added 2 commits September 6, 2023 09:38

Bump appraisals to use new libdatadog

4d837ef

ivoanjo force-pushed the ivoanjo/prof-8139-upgrade-libdatadog4 branch from b187f48 to 4d837ef Compare September 6, 2023 08:38

Use reset_profile to raise exception when reset fails

80d8cec

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

View reviewed changes

ivoanjo merged commit 24c162b into master Sep 6, 2023
166 checks passed

ivoanjo deleted the ivoanjo/prof-8139-upgrade-libdatadog4 branch September 6, 2023 13:49

github-actions bot added this to the 1.15.0 milestone Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROF-8139] Upgrade to libdatadog 4 #3104

[PROF-8139] Upgrade to libdatadog 4 #3104

ivoanjo commented Sep 4, 2023 •

edited

Loading

codecov-commenter commented Sep 4, 2023 •

edited

Loading

ekump left a comment

GustavoCaso Sep 5, 2023

ivoanjo Sep 6, 2023

GustavoCaso Sep 5, 2023

ivoanjo Sep 6, 2023

ivoanjo commented Sep 6, 2023

[PROF-8139] Upgrade to libdatadog 4 #3104

[PROF-8139] Upgrade to libdatadog 4 #3104

Conversation

ivoanjo commented Sep 4, 2023 • edited Loading

codecov-commenter commented Sep 4, 2023 • edited Loading

Codecov Report

ekump left a comment

Choose a reason for hiding this comment

GustavoCaso Sep 5, 2023

Choose a reason for hiding this comment

ivoanjo Sep 6, 2023

Choose a reason for hiding this comment

GustavoCaso Sep 5, 2023

Choose a reason for hiding this comment

ivoanjo Sep 6, 2023

Choose a reason for hiding this comment

ivoanjo commented Sep 6, 2023

ivoanjo commented Sep 4, 2023 •

edited

Loading

codecov-commenter commented Sep 4, 2023 •

edited

Loading