[PROF-4779] Refactor in preparation for use of libddprof from profiling native extension by ivoanjo · Pull Request #1885 · DataDog/dd-trace-rb

ivoanjo · 2022-02-07T11:49:26Z

libddprof is a Rust-based library that implements common functionality used by our profilers:

aggregation of profiling data
serialization of profiling data
reporting of profiling data to backend

This PR adds the scaffolding to enable usage of libddprof from dd-trace-rb, but does not yet make use of any of its functionality.

Currently, libddprof works for all supported Rubies (2.1 to 3.2 dev) but only on x86-64 and 64-bit ARM Linux. The current integration has been set up in such a way that on other platforms we print a nice banner telling customers that they won't have profiling but do not otherwise impact the customer in using all other Datadog products (tracing, ci, appsec, ...).

This PR includes a nice refactoring + addition of tests to make sure this safe installation behavior is working correctly.

Note: To allow this PR to be merged in more quickly, I've commented out the actual libddprof linking bits in extconf.rb; I'll re-enable that in a later PR.

lloeki

LGTM, probably worth another pair of eyes as to how this behaves overall WRT general ddtrace user usage.

ivoanjo · 2022-02-28T15:52:25Z

This PR was opened to target the 1.0 branch. I've just rebased it now on top of current master, and moved it to target that branch, since 1.0 is no more. No further changes :)

codecov-commenter · 2022-02-28T16:19:05Z

Codecov Report

Merging #1885 (73863f8) into master (b5bae3c) will decrease coverage by 0.00%.
The diff coverage is 97.90%.

@@            Coverage Diff             @@
##           master    #1885      +/-   ##
==========================================
- Coverage   97.51%   97.51%   -0.01%     
==========================================
  Files         997      999       +2     
  Lines       48739    48858     +119     
==========================================
+ Hits        47528    47642     +114     
- Misses       1211     1216       +5

Impacted Files	Coverage Δ
spec/datadog/profiling/native_extension_spec.rb	`94.50% <0.00%> (-2.13%)`	⬇️
...iling_native_extension/native_extension_helpers.rb	`100.00% <100.00%> (ø)`
spec/datadog/core/configuration/components_spec.rb	`99.68% <100.00%> (-0.01%)`	⬇️
spec/datadog/core/configuration_spec.rb	`100.00% <100.00%> (ø)`
spec/datadog/profiling/integration_spec.rb	`97.29% <100.00%> (-0.02%)`	⬇️
...datadog/profiling/native_extension_helpers_spec.rb	`100.00% <100.00%> (ø)`
spec/datadog/profiling/pprof/builder_spec.rb	`100.00% <100.00%> (ø)`
spec/datadog/profiling/pprof/stack_sample_spec.rb	`100.00% <100.00%> (ø)`
spec/datadog/profiling/pprof/template_spec.rb	`100.00% <100.00%> (ø)`
spec/datadog/profiling/spec_helper.rb	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5bae3c...73863f8. Read the comment docs.

ivoanjo · 2022-03-02T08:49:53Z

@delner care to chime in?

ivoanjo · 2022-04-25T14:43:05Z

I've just did another rebase of this PR, fixing a conflict and adding a few cleanups for things that had since became a bit stale.

Before this PR gets merged in we must release libddprof 0.6.0 on rubygems.org: DataDog/libddprof#59 since older versions will break some of our integration tests (see da925a0 for details).

ivoanjo · 2022-05-05T10:13:39Z

Converted to draft as we're still waiting on 1.1.0 to go out, and then possible a rebase and maybe pull in a fix or two :)

Also reviewed some of the wording in the user-visible banners.

These validations mean that the profiler native extension will no longer be built unless libddprof is avaliable. The extension itself does not yet get compiled using libddprof.

Because we're not yet using any of the functions, libddprof will actually be thrown away at linking time, but everything is ready for us to use them.

…ment in docker volume This avoids a massive performance hit when this folder is shared with a macOS host (seconds to minutes, I kid you not!).

I was not liking how this was looking. This is much nicer!

… on unsupported platforms

…on is available Now that libddprof is in the picture, the profiling native extension will usually not be built for macOS, so we allow these tests to fail when no extension is available.

…ailable there

This is a much nicer approach than just adding them and hoping for the best! Also, it means we can enable more warnings, even if they get ignored on older compilers.

This is a temporary measure to simplify PR <#1885>. A later PR will re-enable all of this.

ivoanjo · 2022-05-16T09:57:10Z

I've rebased this PR, did a few cleanups, and disabled actually linking to libddprof so we can merge it in ASAP.

marcotc · 2022-05-16T23:54:12Z

  before do
-    skip 'Profiling is not supported on JRuby' if PlatformHelpers.jruby?
-    skip 'Profiling is not supported on TruffleRuby' if PlatformHelpers.truffleruby?
+    skip_if_profiling_not_supported(self)


ivoanjo · 2022-05-17T08:41:22Z

Thanks y'all! Let's get this show on the road :)

ivoanjo requested a review from a team February 7, 2022 11:49

lloeki approved these changes Feb 25, 2022

View reviewed changes

ivoanjo force-pushed the ivoanjo/prof-4779-enable-libddprof branch from b68274a to 1090d71 Compare February 28, 2022 15:51

ivoanjo changed the base branch from 1.0 to master February 28, 2022 15:51

ivoanjo mentioned this pull request Mar 1, 2022

[PROF-4780] Add HttpTransport class to report profiles through libddprof #1923

Merged

ivoanjo requested a review from delner March 2, 2022 08:50

ivoanjo mentioned this pull request Mar 3, 2022

[PROF-4780] Report profiles using libddprof, drop old codepath #1924

Closed

ivoanjo force-pushed the ivoanjo/prof-4779-enable-libddprof branch from 8ae9ea1 to da925a0 Compare April 25, 2022 14:40

ivoanjo changed the title ~~[PROF-4779] [Not for 1.0.0] Enable use of libddprof from profiling native extension~~ [PROF-4779] [Not for 1.0.0/1.1.0] Enable use of libddprof from profiling native extension May 5, 2022

ivoanjo marked this pull request as draft May 5, 2022 10:13

luhenry approved these changes May 10, 2022

View reviewed changes

ivoanjo changed the title ~~[PROF-4779] [Not for 1.0.0/1.1.0] Enable use of libddprof from profiling native extension~~ [PROF-4779] Enable use of libddprof from profiling native extension May 16, 2022

ivoanjo added 13 commits May 16, 2022 09:38

Extract profiling native extension support detection to separate file

e0cdea9

Also reviewed some of the wording in the user-visible banners.

Add libddprof validations to profiler native extension support helper

f5288fe

These validations mean that the profiler native extension will no longer be built unless libddprof is avaliable. The extension itself does not yet get compiled using libddprof.

Enable linking libddprof into profiling native extension

c3447c9

Because we're not yet using any of the functions, libddprof will actually be thrown away at linking time, but everything is ready for us to use them.

Place tmp folder used for native extension compilation during develop…

6183c09

…ment in docker volume This avoids a massive performance hit when this folder is shared with a macOS host (seconds to minutes, I kid you not!).

Add validation that native extension is only built on macOS and Linux

03cafeb

Add validation that native extension is only built for x86-64 cpus

fc01268

Refactor banners in native extension helpers

eedb97a

I was not liking how this was looking. This is much nicer!

Avoid compilation of profiling native extension before test suite run…

40c4f97

… on unsupported platforms

Update native extension design document to reflect libddprof usage

14fdcbb

Do not fail profiling native extension specs on macOS when no extensi…

a552427

…on is available Now that libddprof is in the picture, the profiling native extension will usually not be built for macOS, so we allow these tests to fail when no extension is available.

Skip running some profiling testcases on macOS as profiling is not av…

bc69c45

…ailable there

Update validate_benchmarks_spec to not run on macOS

6c99e69

Also compile native extension before running specs on 64-bit ARM Linux

47a5ffc

ivoanjo added 2 commits May 16, 2022 10:22

Test compiler flags before using them

1146f38

This is a much nicer approach than just adding them and hoping for the best! Also, it means we can enable more warnings, even if they get ignored on older compilers.

Temporarily disable linking to libddprof

be6fb26

This is a temporary measure to simplify PR <#1885>. A later PR will re-enable all of this.

ivoanjo force-pushed the ivoanjo/prof-4779-enable-libddprof branch from da925a0 to be6fb26 Compare May 16, 2022 09:48

ivoanjo changed the title ~~[PROF-4779] Enable use of libddprof from profiling native extension~~ [PROF-4779] Refactor in prepraration for use of libddprof from profiling native extension May 16, 2022

ivoanjo marked this pull request as ready for review May 16, 2022 09:56

marcotc approved these changes May 16, 2022

View reviewed changes

ivoanjo merged commit b8801d1 into master May 17, 2022

ivoanjo deleted the ivoanjo/prof-4779-enable-libddprof branch May 17, 2022 08:41

github-actions Bot added this to the 1.1.0 milestone May 17, 2022

ivoanjo mentioned this pull request May 18, 2022

[PROF-4780] Add libddprof dependency to power the new Ruby profiler #2028

Merged

ivoanjo changed the title ~~[PROF-4779] Refactor in prepraration for use of libddprof from profiling native extension~~ [PROF-4779] Refactor in preparation for use of libddprof from profiling native extension May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[PROF-4779] Refactor in preparation for use of libddprof from profiling native extension#1885

[PROF-4779] Refactor in preparation for use of libddprof from profiling native extension#1885
ivoanjo merged 15 commits into
masterfrom
ivoanjo/prof-4779-enable-libddprof

ivoanjo commented Feb 7, 2022 •

edited

Loading

Uh oh!

lloeki left a comment

Uh oh!

ivoanjo commented Feb 28, 2022

Uh oh!

codecov-commenter commented Feb 28, 2022

Uh oh!

ivoanjo commented Mar 2, 2022

Uh oh!

ivoanjo commented Apr 25, 2022

Uh oh!

ivoanjo commented May 5, 2022

Uh oh!

ivoanjo commented May 16, 2022

Uh oh!

marcotc May 16, 2022

Uh oh!

ivoanjo commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ivoanjo commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lloeki left a comment

Choose a reason for hiding this comment

Uh oh!

ivoanjo commented Feb 28, 2022

Uh oh!

codecov-commenter commented Feb 28, 2022

Codecov Report

Uh oh!

ivoanjo commented Mar 2, 2022

Uh oh!

ivoanjo commented Apr 25, 2022

Uh oh!

ivoanjo commented May 5, 2022

Uh oh!

ivoanjo commented May 16, 2022

Uh oh!

marcotc May 16, 2022

Choose a reason for hiding this comment

Uh oh!

ivoanjo commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ivoanjo commented Feb 7, 2022 •

edited

Loading