[PROF-10241] Extract libdatadog crashtracker telemetry into separate extension #3824

ivoanjo · 2024-08-02T16:11:06Z

What does this PR do?

In #3384 we added support for the libdatadog crashtracker telemetry, and in #3816 we enabled it by default.

But so far the crashtracker telemetry support has been tied to the profiler:

It interfaces with libdatadog via the profiler native extension
It's only turned on when the profiler is turned on

This PR is the first step into allowing the crashtracker to be used completely independently of the profiler. Specifically, it addresses only 1) above, and does not yet address 2. (See additional notes for more details).

This PR introduces a new, separate native extension (libdatadog_api) that links to libdatadog separate from the profiler native extension, and moves the crashtracker.c bindings for the crashtracker to it.

This new extension borrows the same strategy as the profiler native extension for building and linking to libdatadog. This led to a bunch of code being extracted (and a bit being copy-pasted) from the profiler native extension.

The below diff looks "scarier" than it is, because most of the diff is things being moved around. After opening the PR, I plan to do a quick pass of comments to facilitate review and indicate more clearly when things just moved around.

Note that the crashtracker is working with this PR -- it's just still depending on profiling being enabled as well.

Motivation:

A big motivation for this work is to enable the crashtracker to work for customers not using profiling (or even in setups where the profiling doesn't work).

Additional Notes:

There's some copy-pasta 🍝:

I chose to copy-paste the datadog_ruby_common.h/datadog_ruby_common.c as it's kinda hard (and I did spend some time on it) to get extconf.rb to read .c and .h files from other directories, and to add them as correct dependencies to the generated Makefile. If you do figure out how to do it, please share!

What's missing for later PRs:

The crashtracker is still only being turned on when the profiler is turned on. We'll need to extract it to be a top-level component, outside Datadog::Profiling.
The native bits still place the crashtracker in Datadog::Profiling::Crashtracker, it should be moved as well.
The crashtracker extension is built in development only when running the profiler test suite. This does not affect an actual regular installation.
The crashtracker is controlled by the DD_PROFILING_EXPERIMENTAL_CRASH_TRACKING_ENABLED / c.profiling.advanced.experimental_crash_tracking_enabled setting. We need to deprecate this setting and rename it to something more appropriate.
Configuration of the endpoint is currently reusing some helper code in profiling/http_transport.rb that was not yet extracted.

Things to keep in mind about libdatadog/this integration:

Libdatadog only has binary builds for linux. This means, like the profiler native extension, this is a "soft" integration that is built to gracefully work with this:
1. If, during installation, the extension detects that it cannot compile, it will just skip that step and still allow a successful installation.
2. You can force the extension to not be built with DD_NO_EXTENSION=true.
3. For testing, you can set DD_FAIL_INSTALL_IF_MISSING_EXTENSION=true to force installation to fail immediately. This is useful when testing/debugging if the installation is working or not.
4. We wrap extension loading with a begin/rescue, and the code gracefully handles situations where the extension is not present.
I've added a document showing how you can develop natively on macOS with libdatadog.

How to test the change?

This snippet will enable profiling and crash Ruby:

DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddprofrb exec ruby -e 'Process.kill("SEGV", Process.pid)'

You should see logging indicating that the crashtracker has been turned on, and if you have an active Datadog agent running, you'll see a crash report being submitted.

This extension will be used to call libdatadog APIs without needing to go through profiling.

There's a lot of copy-pasta and I'm not entirely happy. Let's go with it for now, to get it moving.

The specs still pass! :)

…nsions This cleans up the wad of copy-pasted code I did for my first attempt.

These new files just moved things around from where they were in the profiler, and other than minimal cleanups, they are the same stuff.

We need to use these features in extconf, so let's tell rubocop to stop annoying us about it.

…extensions Let's reduce the copy-pasta!

This also included adding a `skip_building_extension!` to `libdatadog_api/extconf.rb` so we can properly handle pkg-config failures like the profiler extension does.

This was the last bit that had been copy-pasta'd from the profiler native extension into the `libdatadog_api/extconf_helpers` and thus we are now sharing most of the extconf helpers.

…g profiling

…tracker

This makes it easy to use the same prebuilt extension across multiple Ruby patch versions.

One less file, and we can always extract it again later.

datadog-datadog-staging-us1-all · 2024-08-02T16:11:40Z

ext/libdatadog_api/extconf.rb

+require 'rubygems'
+require_relative '../libdatadog_extconf_helpers'
+
+def skip_building_extension!(reason)


🔵 Code Quality Violation

Avoid top-level methods definition. Organize methods in modules/classes. (...read more)

This rule emphasizes the importance of organizing methods within modules or classes in Ruby. In Ruby, it's considered a best practice to wrap methods within classes or modules. This is because it helps in grouping related methods together, which in turn makes the code easier to understand, maintain, and reuse.

Not adhering to this rule can lead to a disorganized codebase, making it hard for other developers to understand and maintain the code. It can also lead to potential name clashes if a method is defined in the global scope.

To avoid violating this rule, always define your methods within a class or a module. For example, instead of writing def some_method; end, you should write class SomeClass def some_method; end end. This not only adheres to the rule but also improves the readability and maintainability of your code.

datadog-datadog-staging-us1-all · 2024-08-02T16:11:41Z

ext/libdatadog_api/extconf.rb

+require 'rubygems'
+require_relative '../libdatadog_extconf_helpers'
+
+def skip_building_extension!(reason)


🔵 Code Quality Violation

Avoid top-level methods definition. Organize methods in modules/classes. (...read more)

This rule emphasizes the importance of organizing methods within modules or classes in Ruby. In Ruby, it's considered a best practice to wrap methods within classes or modules. This is because it helps in grouping related methods together, which in turn makes the code easier to understand, maintain, and reuse.

Not adhering to this rule can lead to a disorganized codebase, making it hard for other developers to understand and maintain the code. It can also lead to potential name clashes if a method is defined in the global scope.

To avoid violating this rule, always define your methods within a class or a module. For example, instead of writing def some_method; end, you should write class SomeClass def some_method; end end. This not only adheres to the rule but also improves the readability and maintainability of your code.

datadog-staging-us1-crawler-test · 2024-08-02T16:11:42Z

ext/libdatadog_api/extconf.rb

+require 'rubygems'
+require_relative '../libdatadog_extconf_helpers'
+
+def skip_building_extension!(reason)


🔵 Code Quality Violation

Avoid top-level methods definition. Organize methods in modules/classes. (...read more)

This rule emphasizes the importance of organizing methods within modules or classes in Ruby. In Ruby, it's considered a best practice to wrap methods within classes or modules. This is because it helps in grouping related methods together, which in turn makes the code easier to understand, maintain, and reuse.

Not adhering to this rule can lead to a disorganized codebase, making it hard for other developers to understand and maintain the code. It can also lead to potential name clashes if a method is defined in the global scope.

To avoid violating this rule, always define your methods within a class or a module. For example, instead of writing def some_method; end, you should write class SomeClass def some_method; end end. This not only adheres to the rule but also improves the readability and maintainability of your code.

ivoanjo · 2024-08-02T16:11:58Z

ext/datadog_profiling_native_extension/datadog_ruby_common.c

These things got moved over from other helper files and put together here, they're mostly unchanged.

ivoanjo · 2024-08-02T16:12:19Z

ext/datadog_profiling_native_extension/datadog_ruby_common.h

These things got moved over from other helper files and put together here, they're mostly unchanged.

ivoanjo · 2024-08-02T16:12:56Z

ext/datadog_profiling_native_extension/helpers.h

-// Used to mark symbols to be exported to the outside of the extension.
-// Consider very carefully before tagging a function with this.
-#define DDTRACE_EXPORT __attribute__ ((visibility ("default")))
-
-// Used to mark function arguments that are deliberately left unused
-#ifdef __GNUC__
-  #define DDTRACE_UNUSED  __attribute__((unused))
-#else
-  #define DDTRACE_UNUSED
-#endif
-


Moved to datadog_ruby_common.h

ivoanjo · 2024-08-02T16:13:09Z

ext/datadog_profiling_native_extension/libdatadog_helpers.c

Moved to datadog_ruby_common.c

ivoanjo · 2024-08-02T16:13:20Z

ext/datadog_profiling_native_extension/libdatadog_helpers.h

Moved to datadog_ruby_common.h

ivoanjo · 2024-08-02T16:13:40Z

ext/datadog_profiling_native_extension/native_extension_helpers.rb

Moved to libdatadog_extconf_helpers.rb

ivoanjo · 2024-08-02T16:13:54Z

ext/datadog_profiling_native_extension/ruby_helpers.c

Moved to datadog_ruby_common.c

ivoanjo · 2024-08-02T16:14:20Z

ext/datadog_profiling_native_extension/ruby_helpers.h

Moved to datadog_ruby_common.h

ivoanjo · 2024-08-02T16:14:55Z

ext/libdatadog_api/datadog_ruby_common.c

Copy/pasta of file from profiling native extension. See PR description for why.

ivoanjo · 2024-08-02T16:15:01Z

ext/libdatadog_api/datadog_ruby_common.h

Copy/pasta of file from profiling native extension. See PR description for why.

ivoanjo · 2024-08-02T16:15:45Z

ext/libdatadog_api/extconf.rb

This is a simplified version of datadog_profiling_native_extension/extconf.rb

ivoanjo · 2024-08-02T16:16:21Z

ext/libdatadog_extconf_helpers.rb

Extracted from datadog_profiling_native_extension/native_extension_helpers.rb with minimal changes

ivoanjo · 2024-08-02T16:17:09Z

lib/datadog/profiling/crashtracker.rb

    class Crashtracker
+      LIBDATADOG_API_FAILURE =
+        begin
+          require "libdatadog_api.#{RUBY_VERSION[/\d+.\d+/]}_#{RUBY_PLATFORM}"
+          nil
+        rescue LoadError => e
+          e.message
+        end


This is needed for gracefully handling the extension being missing. Before the extraction, we were relying on the profiler loading code taking care of the graceful loading (which it does)

codecov-commenter · 2024-08-02T16:27:51Z

Codecov Report

Attention: Patch coverage is 96.15385% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.87%. Comparing base (d3125b9) to head (5f6580d).

Files	Patch %	Lines
ext/libdatadog_extconf_helpers.rb	90.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3824      +/-   ##
==========================================
- Coverage   97.88%   97.87%   -0.01%     
==========================================
  Files        1261     1262       +1     
  Lines       75624    75653      +29     
  Branches     3706     3710       +4     
==========================================
+ Hits        74025    74048      +23     
- Misses       1599     1605       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2024-08-02T16:49:44Z

Benchmarks

Benchmark execution time: 2024-08-02 16:49:37

Comparing candidate commit 5f6580d in PR branch ivoanjo/prof-10241-libdatadog-crashtracker-without-profiling-v2 with baseline commit d3125b9 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 2 unstable metrics.

AlexJF

LGTM!

ivoanjo added 17 commits August 2, 2024 15:25

Bootstrap libdatadog_api native extension

94fc941

This extension will be used to call libdatadog APIs without needing to go through profiling.

Setup libdatadog_api linking based on profiling stuff

bea50c9

There's a lot of copy-pasta and I'm not entirely happy. Let's go with it for now, to get it moving.

Successful experimental extraction of crashtracker to new module

ed05c7b

The specs still pass! :)

Introduce new datadog_ruby_common.h/.c for common bits between exte…

4f8f500

…nsions This cleans up the wad of copy-pasted code I did for my first attempt.

Adopt new datadog_ruby_common.h/.c for profiling native extension

5a8bec6

These new files just moved things around from where they were in the profiler, and other than minimal cleanups, they are the same stuff.

Minor: Actually check value of DDTRACE_DEBUG env variable

0bf005d

Rubocop tweaks for extconf

3342c52

We need to use these features in extconf, so let's tell rubocop to stop annoying us about it.

Extract libdatadog_folder_* into helper that can be shared between …

eabcb38

…extensions Let's reduce the copy-pasta!

Extract pkg_config_missing? to libdatadog_extconf_helpers

587d535

This also included adding a `skip_building_extension!` to `libdatadog_api/extconf.rb` so we can properly handle pkg-config failures like the profiler extension does.

Extract try_loading_libdatadog to libdatadog_extconf_helpers

28ebaa7

This was the last bit that had been copy-pasta'd from the profiler native extension into the `libdatadog_api/extconf_helpers` and thus we are now sharing most of the extconf helpers.

Add env flags to skip extension and to force early failures, mirrorin…

85b1d8b

…g profiling

Gracefully handle errors when failing to load libdatadog_api in crash…

f051a0f

…tracker

Tag extension builds with only major.minor, skipping Ruby patch version

f0a17d2

This makes it easy to use the same prebuilt extension across multiple Ruby patch versions.

Inline Init_libdatadog_api into crashtracker.c

7037bdb

One less file, and we can always extract it again later.

Add tiny doc explaining how to build on macOS

b7dc64d

Add TODO about spec extraction

7448a21

Update type signature

5f6580d

ivoanjo requested review from a team as code owners August 2, 2024 16:11

github-actions bot added the profiling Involves Datadog profiling label Aug 2, 2024

datadog-datadog-staging-us1-all bot reviewed Aug 2, 2024

View reviewed changes

datadog-staging-us1-crawler-test bot reviewed Aug 2, 2024

View reviewed changes