[NO-TICKET] Fix Ruby 3.3 CI being broken in master due to profiler #3356
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 is not optimal BUT these tests are all related to the in-development and off-by-default heap profiling feature so the blast radius is small.
… 3.3 **What does this PR do?** This PR changes the profiler to use the new `rb_postponed_job_preregister`/`rb_postponed_job_trigger` API instead of `rb_postponed_job_register_one` in Ruby 3.3. The old API is deprecated. **Motivation:** Usage of the old API was causing CI to fail, since we turn every warning into an error in CI, and Ruby 3.3 includes a nice warning when the deprecated API gets used. **Additional Notes:** See also https://bugs.ruby-lang.org/issues/19991 and ruby/ruby#8949 if you're curious on why this new API was introduced. **How to test the change?** The code path for calling `sample_from_postponed_job` is already covered by existing tests. I discovered while working on this that the code path for calling `after_gc_from_postponed_job` from `on_gc_event` is missing coverage since we exercise that behavior in a different way in the specs. I've taken a note to circle back on this, but I didn't want to delay the PR since CI is broken in master currently.
AlexJF
approved these changes
Jan 2, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3356 +/- ##
=======================================
Coverage 98.23% 98.23%
=======================================
Files 1253 1253
Lines 73035 73039 +4
Branches 3428 3431 +3
=======================================
+ Hits 71748 71752 +4
Misses 1287 1287 ☔ View full report in Codecov by Sentry. |
ivoanjo
added a commit
that referenced
this pull request
Jan 11, 2024
**What does this PR do?** This PR re-enables a garbage collection profiling spec in the `CpuAndWallTimeWorker` that was disabled on Ruby 3. This test was disabled in #2354 as this caused flaky tests due to a Ruby VM bug related to Ractor GC. Recently, this "flaky when combined with Ractors" behavior started showing up in other places and @AlexJF came up with a better solution in #3320 of separating out the Ractor tests. Thus, we can now run this spec on Ruby 3. **Motivation**: Recently, while working on #3356, I noticed that if I commented out the calls to `rb_postponed_job_[trigger|register_one]` in `on_gc_event`, no specs failed. This was pretty surprising to me -- as it was weird that we didn't have test coverage for it. After looking into it, I realized we did have specs for it -- but we weren't running them on Ruby 3, which I was using for local development. The re-enabled spec correctly covers that situation. **Additional Notes:** N/A **How to test the change?** Validate that test now runs on Ruby 3 and test suite is still green.
2 tasks
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.
What does this PR do?
This PR includes two changes:
See individual commit messages for details.
Motivation:
These fixes were needed to bring CI back to green now that it's running with the stable Ruby 3.3.0.
Additional Notes:
N/A
How to test the change?
Existing test coverage should mostly be enough, or in the cases it's not, the feature affected is off by default and still experimental.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.