-
Notifications
You must be signed in to change notification settings - Fork 373
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-4780] Add libddprof dependency to power the new Ruby profiler #2028
Conversation
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 |
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's real now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it prettier as suggested by Marco! Co-authored-by: Marco Costa <marco.costa@datadoghq.com>
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_EXTENSION
environment variable set totrue
to 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.