Skip to content

feat: implement capacity estimate (workers → supportable jobs)#29

Open
lukas-frystak-sonarsource wants to merge 12 commits into
mainfrom
feat/capacity-estimate
Open

feat: implement capacity estimate (workers → supportable jobs)#29
lukas-frystak-sonarsource wants to merge 12 commits into
mainfrom
feat/capacity-estimate

Conversation

@lukas-frystak-sonarsource

@lukas-frystak-sonarsource lukas-frystak-sonarsource commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Implements spec 031-capacity-estimate.md: an experimental --estimate-workers flag on analyze bgtasks (and run bgtasks for parity)
  • Given a comma-separated list of worker counts, the estimator computes expected project-analysis throughput per hour (±20%) broken down by 8 size categories (XXS…XXXL)
  • The estimate is log-only — the HTML report is byte-for-byte identical when the flag is omitted

Key changes

File Change
internal/analyzer/bgtasks/estimate.go New: pure 5-step estimation algorithm. Named constants for rounding granularity (0.1s), cost floor (0.1s), margin (20%), seconds-per-hour (3600).
internal/analyzer/bgtasks/estimate_test.go New: 14 unit tests covering every acceptance criterion — happy path, ISSUE_SYNC exclusion, category shares summing to 1, mode floor, tie-breaking, duplicate/unsorted inputs, XXXL label, empty-category div-by-zero safety, proportional scaling.
internal/analyzer/bgtasks.go AnalyzeBgTasks gains estimateWorkers []int; logCapacityEstimate is called after the report is written — report generation is never affected. DEBUG for intermediate arithmetic, INFO for headline results.
cmd/analyze.go --estimate-workers IntSlice flag with >=1 validation; withEstimateWorkers option threads it through.
cmd/run.go Same flag for parity.

Notable decisions

  • EstimateCapacity is pure (no I/O, no globals); intermediate values are returned in the result struct and logged at the call site in logCapacityEstimate.
  • XXXL upper bound uses ceiling division (maxObservedMs + 999) / 1000 so the label always shows a whole-second value ≥ the actual max.
  • Mode tie-breaking picks the smaller rounded value for determinism, as specified.

Testing notes

  • All 70+ existing tests continue to pass
  • 14 new unit tests in estimate_test.go directly cover each acceptance criterion
  • sonar analyze agentic --staged and golangci-lint run both report 0 issues
  • Smoke test requiring a live SonarQube instance (sonar-insights -v analyze bgtasks --estimate-workers 4,8,16) was not run — no live instance available in CI; the flag wiring and logging logic are covered by the unit and integration tests in bgtasks_test.go

🤖 Generated with Claude Code


Summary by Gitar

  • New specification:
    • Added specs/032-IMP-capacity-estimate-mean-and-demand.md to refine the capacity estimation logic, introducing mean-based costing and a demand-profile analysis.
  • Refined estimation logic:
    • Updated internal/analyzer/bgtasks/estimate.go to use the mean of execution times as the representative cost, removing arbitrary ±20% margins and rounding-based mode selection.
    • Added DemandProfile calculation to compare throughput against peak and average analysis arrival rates.
  • Updated logging:
    • Modified internal/analyzer/bgtasks.go to report per-worker verdict lines based on demand vs. capacity, replacing uncertainty margins with operational guidance.
  • Regression testing:
    • Replaced existing mode-based unit tests in internal/analyzer/bgtasks/estimate_test.go with tests covering the new mean-based calculations, demand profiles, and verdict logic.

This will update automatically on new commits.

Sonar CAG skill added by the sonar CLI
Spec 031 introduces an experimental --estimate-workers flag on
`analyze bgtasks` (and `run bgtasks` for parity) that inverts the
existing capacity-demand analysis. Given a planned worker count, it
estimates how many project analyses per hour that capacity can support,
broken down by size category (XXS–XXXL) with a ±20% error margin.

Key design decisions captured in the spec:
- Pure estimation functions in estimate.go; no new data sources
- ISSUE_SYNC tasks excluded from all sums to avoid reindex skew
- Mode on 0.1s-rounded execution times as the representative per-job cost
- Every calculation step logged at DEBUG; headline results at INFO
- Report output is byte-for-byte identical when the flag is omitted
- Algorithm fully specified with guards for empty categories and zero totals
Adds an experimental --estimate-workers flag to `analyze bgtasks` (and
`run bgtasks` for parity). Given a comma-separated list of worker counts
the estimator computes expected project-analysis throughput per hour
(with ±20% margin) broken down by size category (XXS…XXXL).

Key changes:
- internal/analyzer/bgtasks/estimate.go: pure estimation functions;
  EstimateCapacity performs the 5-step algorithm (ISSUE_SYNC exclusion,
  REPORT isolation, report-share, per-category mode cost, per-worker
  projection). Named constants for rounding granularity, cost floor,
  margin, and seconds-per-hour.
- internal/analyzer/bgtasks/estimate_test.go: unit tests for every
  acceptance criterion — happy path, ISSUE_SYNC exclusion, category
  shares summing to 1, mode floor, tie-breaking, duplicate/unsorted
  worker inputs, XXXL label, empty-category div-by-zero safety, and
  proportional scaling across worker counts.
- internal/analyzer/bgtasks.go: AnalyzeBgTasks gains an
  estimateWorkers []int parameter; logCapacityEstimate is called after
  the report is written — report generation is never affected.
  All algorithm steps are logged: DEBUG for intermediate arithmetic,
  INFO for headline results.
- cmd/analyze.go: --estimate-workers IntSlice flag with validation
  (values must be >= 1); withEstimateWorkers analyzeOption threads it
  through to AnalyzeBgTasks.
- cmd/run.go: same flag for parity.

The estimate result is not added to AnalysisResults and is not passed
to report.go — HTML output is byte-for-byte identical when the flag is
omitted.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Comment thread internal/analyzer/bgtasks/estimate.go
Comment thread internal/analyzer/bgtasks/estimate.go
Comment thread cmd/analyze.go
gitar-bot and others added 4 commits June 26, 2026 16:22
…ng in logger

Co-authored-by: Lukas Frystak <95630751+lukas-frystak-sonarsource@users.noreply.github.com>
…lower bound

Co-authored-by: Lukas Frystak <95630751+lukas-frystak-sonarsource@users.noreply.github.com>
Co-authored-by: Lukas Frystak <95630751+lukas-frystak-sonarsource@users.noreply.github.com>
Adds two integration tests in bgtasks_test.go that exercise the
logCapacityEstimate code path which had zero coverage:

- TestAnalyzeBgTasks_WithEstimateWorkers: runs AnalyzeBgTasks with
  --estimate-workers 4,8 and task data that includes a REPORT task
  (M bucket), a sub-50ms REPORT task (XXS, triggers the mode-floor
  branch), and an ISSUE_SYNC task (verifies exclusion). Uses a
  DEBUG-level logger so all step-by-step log branches execute.
- TestAnalyzeBgTasks_EstimateSkipped_NoReport: runs with only a
  non-REPORT task so the estimator skips gracefully; verifies the
  report is still written.

Both tests verify that the HTML report is produced unchanged regardless
of the estimate path taken.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Comment on lines +283 to +297
func TestAnalyzeBgTasks_WithEstimateWorkers(t *testing.T) {
dir := t.TempDir()
bgtasksDir := writeBgtasksDir(t, dir)
if err := os.WriteFile(filepath.Join(bgtasksDir, "page-0001.json"), []byte(estimateTaskJSON), 0o644); err != nil {
t.Fatal(err)
}
reportDir := t.TempDir()
// Use a debug logger to exercise all log branches in logCapacityEstimate.
debugLogger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug}))
if err := AnalyzeBgTasks(dir, reportDir, "myreport", nil, nil, debugLogger, []int{4, 8}); err != nil {
t.Fatalf("unexpected error: %v", err)
}
// Report must still be written despite the estimate running.
if _, err := os.Stat(filepath.Join(reportDir, "myreport.html")); os.IsNotExist(err) {
t.Error("expected report file myreport.html to be created")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Estimate integration tests assert only report creation

TestAnalyzeBgTasks_WithEstimateWorkers and TestAnalyzeBgTasks_EstimateSkipped_NoReport only assert that myreport.html is created — the same assertion already made by TestAnalyzeBgTasks_HappyPath. Neither test captures or inspects the logger output, so they don't actually verify that the capacity estimate ran, produced expected per-category throughput, applied the cost floor (the comment claims '20ms → floor applied'), excluded the ISSUE_SYNC task, or skipped gracefully with the expected reason. As written, these tests would still pass even if logCapacityEstimate did nothing. The pure algorithm is covered in estimate_test.go, so this is low impact, but capturing the debug logger output (e.g. into a bytes.Buffer via slog.NewTextHandler) and asserting on key lines would make these tests meaningfully exercise the wiring rather than duplicating the happy-path check.

Capture logger output and assert the estimate actually ran.:

var buf bytes.Buffer
debugLogger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))
if err := AnalyzeBgTasks(dir, reportDir, "myreport", nil, nil, debugLogger, []int{4, 8}); err != nil {
	t.Fatalf("unexpected error: %v", err)
}
if !strings.Contains(buf.String(), "capacity estimate") {
	t.Errorf("expected capacity estimate to be logged, got:
%s", buf.String())
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

Amends shipped spec 031. Replaces two of its design choices that this
refinement supersedes:

- Representative per-job cost switches from mode to mean, removing the
  0.1s rounding and tie-break. The total then reduces to the provable
  identity totalJobs = reportCapacity / averageCost, so the size-category
  split is unbiased. (Mode sat below the mean for right-skewed execution
  times and over-estimated throughput, worst for the wide buckets.)
- The flat +/-20% band is replaced by a demand profile: REPORT arrivals
  are binned by clock-hour (from SubmittedAt) to report average and peak
  (p95) hourly demand, and each worker count gets a plain-language verdict
  on whether its capacity covers average and peak load.

Also specifies always listing all eight size categories (empties marked
"no tasks") and a doc comment for the capacity -> jobs chain. Log-only,
report output unchanged. draft-status: draft.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread specs/032-IMP-capacity-estimate-mean-and-demand.md Outdated
Review fixes to spec 032 (capacity estimate mean + demand profile):

- Fix illustrative output: empty XXXL shows "> 180s" label, not a
  numeric upper bound (a numeric max implies a task in XXXL, which
  contradicts "no tasks"). Matches unchanged 031 labeling + the
  backwards-range guard.
- Point percentile reuse at mathutil.CalculatePercentile (already used
  by metrics.go/capacity.go); note it interpolates linearly, not
  nearest-rank, and takes 0.95.
- Define averageReportCost explicitly where the mean identity is stated.
- Qualify the mean-identity acceptance criterion: exact only when no
  category hits the cost floor; update matching test scenario.
- Set draft-status: ready (no blockers).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Lukas Frystak <95630751+lukas-frystak-sonarsource@users.noreply.github.com>
@gitar-bot

gitar-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 4 resolved / 5 findings

Implements capacity estimation for background task workers using mean-based costing and demand profiles. Consider adding specific assertions for estimate output values in the integration tests to move beyond verifying report creation only.

💡 Quality: Estimate integration tests assert only report creation

📄 internal/analyzer/bgtasks_test.go:283-297

TestAnalyzeBgTasks_WithEstimateWorkers and TestAnalyzeBgTasks_EstimateSkipped_NoReport only assert that myreport.html is created — the same assertion already made by TestAnalyzeBgTasks_HappyPath. Neither test captures or inspects the logger output, so they don't actually verify that the capacity estimate ran, produced expected per-category throughput, applied the cost floor (the comment claims '20ms → floor applied'), excluded the ISSUE_SYNC task, or skipped gracefully with the expected reason. As written, these tests would still pass even if logCapacityEstimate did nothing. The pure algorithm is covered in estimate_test.go, so this is low impact, but capturing the debug logger output (e.g. into a bytes.Buffer via slog.NewTextHandler) and asserting on key lines would make these tests meaningfully exercise the wiring rather than duplicating the happy-path check.

Capture logger output and assert the estimate actually ran.
var buf bytes.Buffer
debugLogger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))
if err := AnalyzeBgTasks(dir, reportDir, "myreport", nil, nil, debugLogger, []int{4, 8}); err != nil {
	t.Fatalf("unexpected error: %v", err)
}
if !strings.Contains(buf.String(), "capacity estimate") {
	t.Errorf("expected capacity estimate to be logged, got:
%s", buf.String())
}
✅ 4 resolved
Quality: Step-5 arithmetic duplicated between estimator and logger

📄 internal/analyzer/bgtasks/estimate.go:258-272 📄 internal/analyzer/bgtasks.go:129-139 📄 internal/analyzer/bgtasks.go:157
computeWorkerEstimate (estimate.go) computes capacitySec, reportCapacitySec, and per-category catCapacitySec to derive jobs, while logCapacityEstimate (bgtasks.go:131-138) re-derives the exact same intermediate values independently for DEBUG logging using a separately-defined secondsPerHour constant in the analyzer package. If the estimation formula or the constant ever changes in one place, the DEBUG trace will silently diverge from the numbers actually used — which undermines the spec's stated goal that 'a reader must be able to reconstruct each result from the logs alone.' Consider returning these intermediate values in CategoryEstimate/WorkerEstimate (e.g. a CatCapacitySec field) and logging those, so there is a single source of truth.

Quality: XXXL label is nonsensical when no task exceeds 180s

📄 internal/analyzer/bgtasks/estimate.go:196-201
In buildCategoryEstimates, the XXXL label uses upperBound = (maxObservedMs + 999) / 1000 where maxObservedMs is the maximum across ALL REPORT tasks (not just the XXXL bucket). When no REPORT task exceeds 180s, maxObservedMs can be small (e.g. 2000ms → upperBound=2), producing a backwards/misleading label like XXXL (180-2s). The bucket is empty in that case so it is skipped from INFO output, but it is still emitted at DEBUG for steps 3 and 4. Consider clamping the displayed upper bound to at least 180 (or labeling it > 180s when the bucket is empty / max is below 180).

Quality: CLI validateEstimateWorkers has no unit test

📄 cmd/analyze.go:75-82 📄 cmd/run.go:58-61
validateEstimateWorkers enforces the >= 1 rule for --estimate-workers in both cmd/analyze.go and cmd/run.go, but the diff adds no test exercising it (the 14 new tests are all on the pure estimator). A regression that drops or weakens this validation (e.g. allowing 0, which would make capacitySec 0 and produce all-zero estimates) would go unnoticed. Consider a small table test for validateEstimateWorkers covering the <1, empty, and valid cases.

Quality: Stray tags at end of spec file

📄 specs/032-IMP-capacity-estimate-mean-and-demand.md:342-343
The spec markdown file ends with two stray tags — </content> (line 342) and </invoke> (line 343) — that appear to be accidental artifacts left over from the file-creation tooling rather than intended content. They will render as literal text at the bottom of the rendered document and have no semantic meaning in the spec. Remove both lines so the file ends cleanly after line 341 (the × 24 note).

🤖 Prompt for agents
Code Review: Implements capacity estimation for background task workers using mean-based costing and demand profiles. Consider adding specific assertions for estimate output values in the integration tests to move beyond verifying report creation only.

1. 💡 Quality: Estimate integration tests assert only report creation
   Files: internal/analyzer/bgtasks_test.go:283-297

   `TestAnalyzeBgTasks_WithEstimateWorkers` and `TestAnalyzeBgTasks_EstimateSkipped_NoReport` only assert that `myreport.html` is created — the same assertion already made by `TestAnalyzeBgTasks_HappyPath`. Neither test captures or inspects the logger output, so they don't actually verify that the capacity estimate ran, produced expected per-category throughput, applied the cost floor (the comment claims '20ms → floor applied'), excluded the ISSUE_SYNC task, or skipped gracefully with the expected reason. As written, these tests would still pass even if `logCapacityEstimate` did nothing. The pure algorithm is covered in `estimate_test.go`, so this is low impact, but capturing the debug logger output (e.g. into a `bytes.Buffer` via `slog.NewTextHandler`) and asserting on key lines would make these tests meaningfully exercise the wiring rather than duplicating the happy-path check.

   Fix (Capture logger output and assert the estimate actually ran.):
   var buf bytes.Buffer
   debugLogger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))
   if err := AnalyzeBgTasks(dir, reportDir, "myreport", nil, nil, debugLogger, []int{4, 8}); err != nil {
   	t.Fatalf("unexpected error: %v", err)
   }
   if !strings.Contains(buf.String(), "capacity estimate") {
   	t.Errorf("expected capacity estimate to be logged, got:
   %s", buf.String())
   }

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud

Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants