Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROF-8289] Upgrade to libdatadog 5 #3169

Merged
merged 9 commits into from
Oct 4, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Sep 28, 2023

What does this PR do?

This PR upgrades dd-trace-rb to use libdatadog 5. This release includes BIG 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:

I recommend reviewing this PR commit-by-commit, as it makes more sense for what changed why instead of the whole diff.

The backwards incompatible changes were the following:

  • The value of the end_timestamp_ns label is now provided as a regular argument to ddog_prof_Profile_add
  • The libdatadog 5 serializer outputs compressed pprof files:
    • A number of tests needed to be adjusted to take that into account
    • The HttpTransport needed to be adjusted to the new API that allows us to tell libdatadog which files to compress and which to assume are compressed when exporting
  • The libdatadog 5 serializer now resets profiles as part of serializing them
  • The ddog_prof_Profile_new now returns a result structure

I'm opening this PR on top of #3162 because I'm making use of the sample_labels struct that was introduced in that PR. Otherwise the two PRs are independant.

Furthermore, I'm opening it as a draft since libdatadog 5 is not yet available on rubygems.org. Once a release is out, I'll mark this PR as non-draft.

How to test the change?

Our existing test coverage also includes a lot of libdatadog integration testing.

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.

@github-actions github-actions bot added the profiling Involves Datadog profiling label Sep 28, 2023
Base automatically changed from ivoanjo/prof-7440-timeline-states-v1 to master October 3, 2023 11:35
ivoanjo added a commit to DataDog/libdatadog that referenced this pull request Oct 3, 2023
**What does this PR do?**

This PR bumps the libdatadog version to 5.0.0. This PR looks a bit
different from previous PRs to bump libdatadog version
[(here's the 4.0.0 PR as an example)](#236)
because in [this PR](#247)
we've centralized the version on the single `Cargo.toml` file.

Furthermore, we're going from 4.0.0 to 5.0.0 because there were a number
of backwards-incompatible changes to the profiling APIs.

**Motivation:**

Release libdatadog 5.5.0.

**Additional Notes:**

If I haven't missed anything, the backwards incompatible API changes
were the following:

* The value of the `end_timestamp_ns` label is now provided as a
  regular argument to `ddog_prof_Profile_add`
* The libdatadog 5 serializer outputs compressed pprof files
* The exporter has a new API that takes two lists, a list of
  files to compress and a list of files to assume are compressed when
  exporting
* The libdatadog 5 serializer now resets profiles as part of serializing them
* The `ddog_prof_Profile_new` now returns a result structure

**How to test the change?**

I've tested the libdatadog 5 releases using the Ruby profiler, see
DataDog/dd-trace-rb#3169 for my draft PR.
…og_prof_Profile_add` directly

(Haven't fully tested it since other things aren't compiling yet
either).
This is why the `start_time` argument from `reset` is also needed for
`_serialize`.
In libdatadog 5, the exporter gets two arguments:
* files to be compressed
* files that are already assumed to be compressed

This change matches up with the fact that the libdatadog 5 serializer
now produces compressed pprofs, but some other files (e.g. code
provenance) are still handed over uncompressed.
ivoanjo added a commit to DataDog/libdatadog that referenced this pull request Oct 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>

(It's also exactly the same as
[the v4.0.0 release PR](#238)).

**Motivation:**

Enable Ruby to use libdatadog v5.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#3169 .

As a reminder, new libdatadog releases don't get automatically picked up
by dd-trace-rb, so the PR that bumps the Ruby profiler will also test this
release against all supported Ruby versions.
@ivoanjo ivoanjo force-pushed the ivoanjo/prof-8289-libdatadog5-upgrade branch from 8b5031f to f3f0dbd Compare October 4, 2023 10:07
@ivoanjo ivoanjo force-pushed the ivoanjo/prof-8289-libdatadog5-upgrade branch from f3f0dbd to df15be8 Compare October 4, 2023 10:48
@ivoanjo ivoanjo marked this pull request as ready for review October 4, 2023 10:49
@ivoanjo ivoanjo requested review from a team as code owners October 4, 2023 10:49
Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder to run bundle exec appraisal:lock before merging.

@ivoanjo
Copy link
Member Author

ivoanjo commented Oct 4, 2023

Just a reminder to run bundle exec appraisal:lock before merging.

Right! Thanks for the reminder, I totally forgot about it.

@codecov-commenter
Copy link

Codecov Report

Merging #3169 (87eeec2) into master (5147774) will decrease coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3169      +/-   ##
==========================================
- Coverage   98.21%   98.20%   -0.01%     
==========================================
  Files        1247     1247              
  Lines       71663    71666       +3     
  Branches     3329     3330       +1     
==========================================
+ Hits        70381    70383       +2     
- Misses       1282     1283       +1     
Files Coverage Δ
...iling_native_extension/native_extension_helpers.rb 96.73% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

@ivoanjo ivoanjo merged commit 9c0c820 into master Oct 4, 2023
176 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/prof-8289-libdatadog5-upgrade branch October 4, 2023 14:42
@github-actions github-actions bot added this to the 1.15.0 milestone Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants