Repository navigation
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ebec889 | Docs | View more details | Give us feedback! |
|
First Gemfile-aware Unit Tests run: https://github.com/DataDog/dd-trace-rb/actions/runs/37613186521
|
|
Post-
All Unit jobs passed. The 12-second workflow difference and 1-second slowest-shard difference indicate that the improvement is repeatable rather than a favorable one-off run. |
BenchmarksBenchmark execution time: 2026-10-07 12:30:45 Comparing candidate commit ebec889 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 0 unstable metrics.
|
|
This improves the slowest shard by a few minutes per run, which is real, but I want to pressure-test the approach before we commit to maintaining it. The timing manifest is the part that concerns me. It's a large checked-in file that needs a manual refresh process, and I'd like to understand what we're signing up for: when do the weights need recalculating, what is the actual procedure to do it, and what happens when someone adds or removes a test task — how long does the schedule run on stale or missing data, and who notices? On whether the built-in approach is worth it at all: tools like Knapsack Pro already sell dynamic test distribution, so I don't think this is a problem we have to solve by hand anymore. More importantly, we already send all the runtime data this feature needs to Datadog CI Visibility. It feels like test distribution belongs there as a product feature — and dd-trace-rb would be the natural place to dogfood it. If we build this by hand now, are we rebuilding something the product side should own, and does that change what the minimal version here should look like? |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Strech
left a comment
There was a problem hiding this comment.
I'm with Tony on being dynamic, Knapsack Pro evolution (I was in first 50 clients 10y ago, lol) started with this and already went this path from local file with timing to the dynamic re-distribution on server due to ever changing results from each run. I don't think we want to re-invent the bicycle here and step on the same rake twice.
| tasks: entries.fetch("tasks", []).map do |entry| | ||
| [[entry.fetch("task"), entry.fetch("group")], entry.fetch("p90_seconds")] | ||
| end.to_h, | ||
| gemfiles: entries.fetch("gemfiles", []).map do |entry| | ||
| [entry.fetch("gemfile"), entry.fetch("p90_seconds")] | ||
| end.to_h, |
There was a problem hiding this comment.
I think you can turn that into each_with_object calls instead and avoid to_h and multiline block
| end.max | ||
|
|
||
| task = entry.fetch(:task) | ||
| candidate = [ |
There was a problem hiding this comment.
Should it be plural form? candidates
There was a problem hiding this comment.
More details
Generated plans preserve every task and remain deterministic across all configured Ruby versions. The specialist’s focused checks supported correct accounting for Gemfile setup costs when moving tasks between shards.
🤖 Bits Code Review · Commit ebec889 · @DataDog review to ask questions
What does this PR do?
Separates per-task test duration from per-Gemfile dependency setup duration when estimating unit-test batches.
Groups tasks sharing a Gemfile before balancing the seven shards, then deterministically splits a group only when the duplicated setup cost still lowers the slowest estimated shard.
Stores p50 and p90 estimates from the two timing runs collected by #6429 and schedules against p90.
Motivation:
#6429 reduced the Unit Tests workflow to 15m04s, but its combined task weights move one-time Gemfile setup costs with whichever task happened to run first.
The Ruby 3.2 generated plan now has 69 Gemfile placements for 69 unique Gemfiles, with estimated shard loads between 441.7s and 446.9s.
Change log entry
Not required because this only changes internal CI scheduling.
Additional Notes:
This is a follow-up experiment based on #6429 and should be reviewed after that PR.
The first Unit Tests run completed successfully in 12m32s, 2m32s faster than #6429's 15m04s duration-weighted run.
The slowest standard shard completed in 9m41s, down from 11m56s on #6429.
How to test the change?
Run
bundle exec rspec spec/tasks/github_batching_spec.rband compare repeated Unit Tests workflow durations and shard spreads with #6429.