[NO-TICKET] Profiling: Add integration test with GC.stress - #6302
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aca274441
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 8631f6a | Docs | View more details | Give us feedback! |
|
I saw this fail in CI with
I actually had an older WIP fix for that in #6021 which I haven't yet finished, so I'll hold on merging this one a few days until I can fix that one. |
|
Update: Flaky issue I mentioned above will be fixed by #6364 . I'll rebase this PR and stack it on top of that one, to avoid landing a new test that triggers flaky behavior before the PR that fixes the flaky behavior ;) |
**What does this PR do?** This PR adds a new "integration" test to the `cpu_and_wall_time_worker_spec.rb` where we run the profiler for a brief period with all features enabled under Ruby's `GC.stress` setting. **Motivation:** In #6242 and #6245 we were able to reproduce those bugs using `GC.stress`. To try to catch possible similar issues in the future, it seemed useful to have an integration test where we ran the profiler with `GC.stress`. **Additional Notes:** The big downside of `GC.stress` is how slow it makes the test suite. On my workspace, it takes around 20-30s to run just this one new testcase, which is why I've made it only run in CI as I really value quick iteration on our testsuite and having mega-slow tests breaks that. I also evaluated how feasible it would be to run the entire `cpu_and_wall_time_worker_spec.rb` with `GC.stress` and unfortunately the answer isn't great -- it would take hours AND in particular it would need modifications since we have a bunch of timeouts for cross-thread stuff that would need heavy tweaking. Ideas on how we could get more coverage without a lot of work are welcome ;) **How to test the change?** Validate CI is green and this test passes!
Co-authored-by: Benoit Daloze <eregontp@gmail.com>
0b77e23 to
0857e9c
Compare
|
Regarding this spec being slow, maybe we should look at the serialized profile to find out which lines take longest? |
Yeap, I didn't mention it, the PR already incorporates some of that -- I added the pre-create instances thing for instance. A very big one is https://github.com/DataDog/dd-trace-rb/blob/master/spec/spec_helper.rb#L330 (since we start a new thread) but there's also a few others so I hesitated in trying too hard in that direction. |
...otherwise we might (as just happened in CI) end up in a situation where e.g. no allocation samples are taken.
… due to GC **What does this PR do?** This PR fixes the profiler stopping with "BUG: Unexpected CPU time going backwards between samples" if we get very unlucky and a GC gets triggered by our `thread_list()` call. The issue happened because of: 1. We recorded the current thread's CPU-time 2. `thread_list()` triggered GC 3. GC profiling hook advances the current thread's CPU-time 4. After `thread_list()` returned, we tried to `update_cpu_time_since_previous_sample` using old timestamp from step 1, resulting in a negative delta, triggering the assert **Motivation:** Fix bug that leads to profiler stopping. **Additional Notes:** This has flown under the radar for a long time because it's quite rare that `thread_list()` actually triggers GC. It did show up as a flaky issue in DataDog#6302 because that PR adds a test that uses `GC.stress` to trigger GC more often. **How to test the change?** I've chosen not to add more coverage as the one that DataDog#6302 will add has already proven to trigger the issue. (I still decided to keep this as a separate, small PR since in the issue already existed independent of that PR)
3 participants
What does this PR do?
This PR adds a new "integration" test to the
cpu_and_wall_time_worker_spec.rbwhere we run the profiler for a brief period with all features enabled under Ruby'sGC.stresssetting.Motivation:
In #6242 and #6245 we were able to reproduce those bugs using
GC.stress.To try to catch possible similar issues in the future, it seemed useful to have an integration test where we ran the profiler with
GC.stress.Change log entry
None.
Additional Notes:
The big downside of
GC.stressis how slow it makes the test suite. On my workspace, it takes around 20-30s to run just this one new testcase, which is why I've made it only run in CI as I really value quick iteration on our testsuite and having mega-slow tests breaks that.I also evaluated how feasible it would be to run the entire
cpu_and_wall_time_worker_spec.rbwithGC.stressand unfortunately the answer isn't great -- it would take hours AND in particular it would need modifications since we have a bunch of timeouts for cross-thread stuff that would need heavy tweaking.Ideas on how we could get more coverage without a lot of work are welcome ;)
How to test the change?
Validate CI is green and this test passes!