[PROF-4780] WIP: libddprof integration improvements and tweaks #1936
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
ivoanjo
added a commit
to DataDog/libddprof
that referenced
this pull request
Apr 25, 2022
I'm following the instructions in <ruby/README.md> on how to package newer versions of libddprof for release. I haven't actually been pushing every release to rubygems.org, since the PR on the Ruby side that will pull it in is still unmerged (<DataDog/dd-trace-rb#1936>) and thus there's no need to litter up the releases list. Nevertheless, I like being ready to release, so I decided to bump this version anyway.
ivoanjo
added a commit
to DataDog/libddprof
that referenced
this pull request
May 3, 2022
I'm following the instructions in <ruby/README.md> on how to package newer versions of libddprof for release. I haven't actually been pushing every release to rubygems.org, since the PR on the Ruby side that will pull it in is still unmerged (<DataDog/dd-trace-rb#1936>) and thus there's no need to litter up the releases list. Nevertheless, I like being ready to release, so I decided to bump this version anyway.
r1viollet
pushed a commit
to DataDog/libdatadog
that referenced
this pull request
May 5, 2022
I'm following the instructions in <ruby/README.md> on how to package newer versions of libddprof for release. I haven't actually been pushing every release to rubygems.org, since the PR on the Ruby side that will pull it in is still unmerged (<DataDog/dd-trace-rb#1936>) and thus there's no need to litter up the releases list. Nevertheless, I like being ready to release, so I decided to bump this version anyway.
luhenry
approved these changes
May 10, 2022
…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. * Use the new `DDPROF_FFI_CHARSLICE_C` to replace `byte_slice_from_literal` * Introduce `char_slice_from_ruby_string` and update APIs that now take a char slice in 0.5.0 * Remove pending from Unix Domain Socket tests: they work now (yay!!!)
Changes included: * Use new tag handling API * Minor updates to match API changes (renames, added arguments, etc)
Up until now, `ddprof_ffi_ProfileExporterV3_send` was blocking and nothing on the Ruby side could interrupt it. This meant that if reporting a profile was slow/hung, then pressing ctrl+c or calling `Thread#kill` would do nothing -- send would only return after it hit the configured timeout (which by default is 30 seconds). This was very annoying if we're talking about an app that is trying to shut down and is being delayed by a report that is taking longer than expected. To fix this, we make use of the `CancellationToken` added in <DataDog/libddprof#45> to enable the Ruby VM to signal to libddprof that it should cancel an in-progress `send`. Thus, as the added spec shows, we can make `send` return early by interrupting the thread, rather than needing to wait until the timeout was hit.
Because we are using `rb_thread_call_without_gvl2`, we need to be careful not to assume that our code ran at all after it returns.
ivoanjo
force-pushed
the
ivoanjo/prof-4780-add-libddprof-httptransport
branch
from
c85d4e3 to
59637a9
Compare
ivoanjo
force-pushed
the
ivoanjo/spike-http-transport-v2
branch
from
dea0426 to
00280c5
Compare
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.
This PR depends on #1923 . I'll come back to clean it up, rebase it, and write an actually decent PR description once that's merged.