[PROF-4780] Add libddprof dependency to power the new Ruby profiler #2028
Conversation
This file contains 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
This reverts commit be6fb26.
Profiler is currently not supported on macOS due to missing libddprof binaries for it, although if you locally compile libddprof it actually works. Thus, I've added a `DD_PROFILING_MACOS_TESTING` environment variable that can be used to allow testing on macOS, but is disabled by default. I also added a check that the profiler is properly available, when we expect it to be available. Without this check, specs that run with a profiler that is not available would fail later with confusing error messages.
…ative extension The new approach factors out some of the repetition in the banners, as well as enables us to later reuse the same messages without the banner styling. I also slightly reworded a few messages to hopefully make them more clear.
…rying to load profiler There are many reasons (see `native_extension_helpers.rb`) why we may not be able to compile the profiling native extension. During gem installation, we check them, and instead of failing to install dd-trace-rb, we print a nice banner explaining what went wrong. Unfortunately, the nice banner is not shown during `gem install` or `bundle install` by default. Instead, customers would run into a cryptic failure regarding the native extension not being able to be loaded. To solve this, whenever we skip compilation, we record that info on a file (`skipped_reason.txt`). Then, at runtime, we check if that file exists, and if it does, we use that as a nice explanation of what went wrong. It's somewhat "icky" to generate files during installation and put them in our source tree, but I couldn't think of a better approach.
Previously, the profiler would start on macOS, but only get wall time and be missing CPU time data. If you're a customer reading this, and this impacts you, please get in touch! We need to do this because with the move to `libddprof` we can't support macOS until we also have `libddprof` binaries for macOS. In practice, the profiler does work on macOS if one manually compiles and packages a `libddprof` version for macOS. So I've added an override that is expected to be used only during development.
…oken libddprof This is a bit of a corner case, but the outcome of tests on macOS depends on all validations (like on Linux), we can't just assume "macOS is OK" when `DD_PROFILING_MACOS_TESTING` is set. Also moved the generic "operating system is not supported" test outside of the test group to simplify testing.
Changes included: * Remove testing for libddprof with missing binaries: From libddprof 0.5.0 on, we will no longer have a fallback version of the gem with no binaries, so `Libddprof.binaries?` was removed.
The missing mocking meant this test would only pass on macOS or Linux, whereas the intent of the spec is to simulate the platform so the spec can run on any Ruby and platform.
…pilers In platforms where `-std=gnu99` is not supported this should still be safe because the `add_compiler_flag` helper actually only adds a flag after checking the compiler accepts it.
Ran into jruby/jruby#7218 but the workaround is easy enough :)
Codecov Report
@@ Coverage Diff @@
## master #2028 +/- ##
=======================================
Coverage 97.44% 97.45%
=======================================
Files 1021 1021
Lines 51750 51802 +52
=======================================
+ Hits 50429 50482 +53
+ Misses 1321 1320 -1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
marcotc
approved these changes
May 19, 2022
| @@ -55,5 +55,8 @@ Gem::Specification.new do |spec| | |||
| # Used by appsec | |||
| spec.add_dependency 'libddwaf', '~> 1.3.0.1.0' | |||
|
|
|||
| # Used by profiling | |||
| spec.add_dependency 'libddprof', '~> 0.6.0.1.0' | |||
Make it prettier as suggested by Marco! Co-authored-by: Marco Costa <marco.costa@datadoghq.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In #1885 we bootstrapped support for libddprof in ddtrace. This PR adds the final bits needed to make it an actual dependency of ddtrace.
This PR:
Relevant Q&A:
Q: What's the impact on ddtrace customers that aren't on the supported libddprof platforms?
A: ddtrace will still install and be usable. When those customers try to turn on profiling, they'll get a nice log message explaining why it can't be turned on, but otherwise their application will work, including using all other Datadog products supported by ddtrace.
Q: What if compilation fails?
A: Users will be presented with an error message that tells them that they can use the
DD_PROFILING_NO_EXTENSIONenvironment variable set totrueto skip any attempts to compile the code.Q: What's the impact of merging this PR?
A: Only customers on x86-64 and 64-bit ARM Linux will be able to use the profiler (no more macOS; windows wasn't supported anyway). The profiling native extension will no longer be optional. Other than that, the current almost-100%-pure-Ruby codepaths will continue to be in use for the time being, this PR is only about enabling later work.
Final note: Some of the commits in this PR were extracted from #1923. I felt that PR had accumulated too many changes, so I've decided to break it down into more focused PRs.