Skip to content

ci: account for unit batch setup costs - #6439

Open
cgcote wants to merge 2 commits into
codex/ci-rebalance-unit-batchesfrom
codex/ci-gemfile-aware-batches
Open

cgcote wants to merge 2 commits into
codex/ci-rebalance-unit-batchesfrom
codex/ci-gemfile-aware-batches

Conversation

@cgcote

@cgcote cgcote commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

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.rb and compare repeated Unit Tests workflow durations and shard spreads with #6429.

@cgcote cgcote added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Oct 7, 2026
@datadog-official

datadog-official Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Tests

✅ All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 94.47% (-0.63%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: ebec889 | Docs | View more details | Give us feedback!

@cgcote

cgcote commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

First Gemfile-aware Unit Tests run: https://github.com/DataDog/dd-trace-rb/actions/runs/37613186521

  • Workflow: 12m32s, down 2m32s from ci: rebalance unit test batches #6429's 15m04s duration-weighted run.
  • Slowest standard shard: 9m41s, down from 11m56s.
  • Original PR baseline: 18m25s p50 and 20m46s p90.
  • Improvement from the original baseline: 5m53s below p50 and 8m14s below p90.
  • All Unit Tests jobs passed.

@cgcote

cgcote commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Post-master repeat completed successfully.

  • Unit workflow: 12m44s (previous experiment run: 12m32s)
  • Slowest standard shard: 9m42s (previous: 9m41s)
  • bundler-audit: passed

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.

@pr-commenter

pr-commenter Bot commented Oct 7, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-10-07 12:30:45

Comparing candidate commit ebec889 in PR branch codex/ci-gemfile-aware-batches with baseline commit f38a9e3 in branch codex/ci-rebalance-unit-batches.

📊 Benchmarking dashboard

Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 0 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

@TonyCTHsu

Copy link
Copy Markdown
Collaborator

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?

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T09:10:51.602401Z ebec889 Draft marked ready
🔒 Security Review ✅ Completed 2026-10-09T09:11:56.268325Z ebec889 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Strech Strech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tasks/github_batching.rb
Comment on lines +11 to +16
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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can turn that into each_with_object calls instead and avoid to_h and multiline block

Comment thread tasks/github_batching.rb
end.max

task = entry.fetch(:task)
candidate = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be plural form? candidates

@cgcote
cgcote marked this pull request as draft October 9, 2026 09:06
@cgcote
cgcote marked this pull request as ready for review October 9, 2026 09:07

@datadog-official datadog-official Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

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.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit ebec889 · @DataDog review to ask questions

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants