feat(byoo-perf): measure a baseline and report results - #637
Open
shobham-nv wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe performance suite now executes repeated load and measurement cycles. It parses collector and sink metrics, computes baseline reports with pod health, records partial or invalid results, and optionally persists JSON output. ChangesPerformance baseline measurement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PerfRunner
participant DeployClient
participant Collector
participant Sink
participant ReportBuilder
participant ResultsDir
PerfRunner->>DeployClient: StartLoad jobs
PerfRunner->>Collector: Scrape collector metrics
PerfRunner->>Sink: Scrape sink metrics
PerfRunner->>DeployClient: WaitLoad jobs
PerfRunner->>DeployClient: Collect pod health
PerfRunner->>ReportBuilder: Build baseline report
ReportBuilder->>ResultsDir: Persist JSON result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Comment |
sbaum1994
reviewed
Aug 5, 2026
sbaum1994
left a comment
sbaum1994
left a comment
Contributor
There was a problem hiding this comment.
Go review findings from the performance-reporting changes.
shobham-nv
force-pushed
the
shobham/419-byoo-perf-loadgen-sink
branch
from
August 10, 2026 15:51
89ae4fa to
833b809
Compare
run now measures the collector over the profile's window and emits a baseline (there are no pass/fail thresholds yet). - pkg/report: a small Prometheus text parser plus baseline computation. It matches candidate metric names per concept (suffixes vary across collector-contrib versions), computes per-signal throughput, drops, end-to-end delivery (sink accepted / collector accepted), collector CPU (avg cores) and RSS, and records pod restart/OOM health. Missing series are noted, not fatal. Emits a human summary and JSON. - deploy: ScrapePodMetrics via the API-server proxy (no metrics-server or port-forward needed), PodHealth, and a StartLoad/WaitLoad split so metrics can be sampled while load is in flight. - run: after warmup, snapshots the collector + sink at the start and end of the measurement window, builds the report, prints the summary, and writes <results-dir>/<shape>.json when --results-dir is set. Signed-off-by: shobham <shobham@nvidia.com>
shobham-nv
force-pushed
the
shobham/420-byoo-perf-measure
branch
from
August 11, 2026 04:53
c64a39d to
01f5652
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.go`:
- Line 162: Update the help text for the results-dir flag in the command’s flag
definition to state that structured JSON output includes one file per shape and
repetition, matching the filenames generated by the multi-repetition write path.
- Around line 372-383: Update the repetition flow around StartLoad and measure
so it waits for every load-generator pod to reach started/running state before
measurement warmup begins. Use a bounded timeout context for this readiness
wait, propagate timeout or startup errors, and add a delayed-start test covering
pods that become ready after job creation.
- Around line 441-453: Update the flow around PodHealth and report.Build to
record health-query errors in Notes and mark the report partial when health is
unavailable, instead of passing an indistinguishable zero-value health result.
Ensure a later load-completion failure still takes precedence and preserves
invalid status, and add a test covering PodHealth failure.
- Around line 419-439: Update the snap function so Snapshot.At reflects when the
metric samples are captured, not when scraping begins. Perform the collector and
sink scrapes concurrently, then apply an explicit common timestamp after
successful responses are received; preserve warning handling for scrape errors.
Add coverage for delayed successful scrapes verifying the resulting window
duration and throughput denominator use the aligned snapshot timestamps.
In `@src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report.go`:
- Around line 164-176: Update Build in
src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report.go (lines
164-176) to capture and pass the ok result from counterDelta to note for every
refused, sent, and failed logs and metrics counter. In
src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report_test.go
(lines 69-112), add all required reported counters to the complete fixture or
assert the resulting notes and partial status; in lines 129-140, require every
reported counter for completeness.
In `@src/compute-plane-services/byoo-otel-collector/perf/README.md`:
- Around line 10-12: Replace the em dash in the README sentence describing
pass/fail thresholds with ASCII punctuation, such as a period or semicolon,
while preserving the sentence’s meaning.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e8e3159-f345-4146-bff0-ed433eb4e2a4
📒 Files selected for processing (11)
src/compute-plane-services/byoo-otel-collector/VERSIONsrc/compute-plane-services/byoo-otel-collector/perf/README.mdsrc/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.gosrc/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main_test.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy_test.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/report/prom.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/report/prom_test.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/report/report.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/report/report_test.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/sink/sink.go
| cmd.Flags().BoolVar(&cfg.skipLoad, "skip-load", false, "deploy the collector and sink but do not drive load") | ||
| cmd.Flags().StringVar(&cfg.k3dCluster, "k3d-cluster", "byoo-perf", "name of the managed k3d cluster (k3d mode)") | ||
| cmd.Flags().BoolVar(&cfg.importImages, "import-images", false, "import the collector/sink/loadgen images from local Docker into the k3d cluster (k3d mode)") | ||
| cmd.Flags().StringVar(&cfg.resultsDir, "results-dir", "", "directory to write structured JSON results to (one <shape>.json per shape)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the --results-dir help text.
Line 162 promises one <shape>.json file per shape. Lines 494-501 write <shape>-run<N>.json for multi-repetition profiles. State that the command writes one file per shape and repetition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.go` at line
162, Update the help text for the results-dir flag in the command’s flag
definition to state that structured JSON output includes one file per shape and
repetition, matching the filenames generated by the multi-repetition write path.
Comment on lines
+372
to
+383
| reports, err := runRepetitions(stdout, prof, shape, | ||
| func(run int) error { | ||
| jobs := loadgen.Jobs(ns, dep.PodName, lgOpts) | ||
| fmt.Fprintf(stdout, "[%s] driving load for %s (logs=%d/s metrics=%d/s) ...\n", shape, loadDuration, lgOpts.LogsPerSec, lgOpts.MetricsPerSec) | ||
| if err := client.StartLoad(ctx, ns, jobs); err != nil { | ||
| return fmt.Errorf("start load for %s shape (run %d): %w", shape, run, err) | ||
| } | ||
| return nil | ||
| }, | ||
| func(run int) report.ShapeReport { | ||
| return measure(ctx, stdout, client, cfg, prof, shape, ns, dep.PodName, collectorMetricsPort) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wait for load Jobs to start before warmup.
StartLoad only creates the Jobs. measure starts warmup immediately after creation. Pod scheduling or image pulls can consume part of the measurement window, while Build still treats the full window as generated load. This produces false throughput and delivery results.
Wait until all load generator pods have started before warmup. Use a bounded context. Add a delayed-start test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.go` around
lines 372 - 383, Update the repetition flow around StartLoad and measure so it
waits for every load-generator pod to reach started/running state before
measurement warmup begins. Use a bounded timeout context for this readiness
wait, propagate timeout or startup errors, and add a delayed-start test covering
pods that become ready after job creation.
Comment on lines
+419
to
+439
| snap := func(label string) report.Snapshot { | ||
| s := report.Snapshot{At: time.Now()} | ||
| if raw, err := client.ScrapePodMetrics(ctx, ns, collectorPod, collectorMetricsPort, "/metrics"); err != nil { | ||
| fmt.Fprintf(stdout, "[%s] warning: %s collector scrape failed: %v\n", shape, label, err) | ||
| } else { | ||
| s.Collector = report.Parse(string(raw)) | ||
| } | ||
| if raw, err := client.ScrapePodMetrics(ctx, ns, sink.Name, strconv.Itoa(sink.MetricsPort), "/metrics"); err != nil { | ||
| fmt.Fprintf(stdout, "[%s] warning: %s sink scrape failed: %v\n", shape, label, err) | ||
| } else { | ||
| s.Sink = report.Parse(string(raw)) | ||
| } | ||
| return s | ||
| } | ||
|
|
||
| fmt.Fprintf(stdout, "[%s] warmup %s ...\n", shape, prof.Warmup) | ||
| sleep(ctx, prof.Warmup) | ||
| start := snap("start") | ||
| fmt.Fprintf(stdout, "[%s] measuring for %s ...\n", shape, prof.MeasurementWindow) | ||
| sleep(ctx, prof.MeasurementWindow) | ||
| end := snap("end") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Timestamp snapshots when samples are taken.
Line 420 assigns Snapshot.At before two synchronous proxy requests. The counter values arrive later, but Window.Seconds() uses the earlier timestamps. Successful slow scrapes can therefore produce an incorrect throughput denominator.
Capture timestamps aligned with the sampled values. Prefer concurrent scrapes with an explicit snapshot time policy. Add a test with delayed successful scrapes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.go` around
lines 419 - 439, Update the snap function so Snapshot.At reflects when the
metric samples are captured, not when scraping begins. Perform the collector and
sink scrapes concurrently, then apply an explicit common timestamp after
successful responses are received; preserve warning handling for scrape errors.
Add coverage for delayed successful scrapes verifying the resulting window
duration and throughput denominator use the aligned snapshot timestamps.
Comment on lines
+441
to
+453
| health, err := client.PodHealth(ctx, ns, collectorPod) | ||
| if err != nil { | ||
| fmt.Fprintf(stdout, "[%s] warning: could not read pod health: %v\n", shape, err) | ||
| } | ||
|
|
||
| return report.Build(report.Inputs{ | ||
| Shape: string(shape), | ||
| Profile: prof.Name, | ||
| LogsPerSec: prof.LogRecordsPerSec, | ||
| MetricsPerSec: prof.MetricDataPointsPerSec, | ||
| Window: report.Window{Start: start, End: end}, | ||
| Health: health, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark unavailable pod health as partial.
If PodHealth fails, this code logs a warning and passes a zero-value PodHealth to Build. A report can then have status="ok" with phase="", restarts=0, and oom_killed=false even though health was not observed.
Record the health collection failure in Notes and set the report to partial. Preserve invalid status if load completion later fails. Add a health-query failure test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.go` around
lines 441 - 453, Update the flow around PodHealth and report.Build to record
health-query errors in Notes and mark the report partial when health is
unavailable, instead of passing an indistinguishable zero-value health result.
Ensure a later load-completion failure still takes precedence and preserves
invalid status, and add a test covering PodHealth failure.
Comment on lines
+164
to
+176
| r.Logs.CollectorRefused, _ = counterDelta(start.Collector, end.Collector, refusedLogs...) | ||
| r.Logs.ExporterSent, _ = counterDelta(start.Collector, end.Collector, sentLogs...) | ||
| r.Logs.ExporterFailed, _ = counterDelta(start.Collector, end.Collector, failedLogs...) | ||
| r.Logs.SinkAccepted, ok = counterDelta(start.Sink, end.Sink, acceptedLogs...) | ||
| note("sink accepted logs", ok) | ||
|
|
||
| // Metrics. | ||
| r.Metrics.GeneratedExpected = float64(in.MetricsPerSec) * win | ||
| r.Metrics.CollectorAccepted, ok = counterDelta(start.Collector, end.Collector, acceptedMetrics...) | ||
| note("collector accepted metric points", ok) | ||
| r.Metrics.CollectorRefused, _ = counterDelta(start.Collector, end.Collector, refusedMetrics...) | ||
| r.Metrics.ExporterSent, _ = counterDelta(start.Collector, end.Collector, sentMetrics...) | ||
| r.Metrics.ExporterFailed, _ = counterDelta(start.Collector, end.Collector, failedMetrics...) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark every unavailable reported counter as missing.
Build ignores the ok result for refused, sent, and failed counters. Missing series then appear as zero values in an ok baseline. This makes absent drop and delivery data indistinguishable from observed zero values.
src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report.go#L164-L176: callnotefor every reported refused, sent, and failed counter.src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report_test.go#L69-L112: include all required counters in the complete fixture, or expect notes and partial status.src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report_test.go#L129-L140: update completeness expectations to require every reported counter.
📍 Affects 2 files
src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report.go#L164-L176(this comment)src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report_test.go#L69-L112src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report_test.go#L129-L140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report.go`
around lines 164 - 176, Update Build in
src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report.go (lines
164-176) to capture and pass the ok result from counterDelta to note for every
refused, sent, and failed logs and metrics counter. In
src/compute-plane-services/byoo-otel-collector/perf/pkg/report/report_test.go
(lines 69-112), add all required reported counters to the complete fixture or
assert the resulting notes and partial status; in lines 129-140, require every
reported counter for completeness.
Comment on lines
+10
to
+12
| > authentic collector pointed at it, drive telemetrygen load, and measure a | ||
| > baseline), and `cleanup` are implemented. There are no pass/fail thresholds | ||
| > yet — `run` establishes a reproducible baseline. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the em dash with ASCII punctuation.
Line 12 adds —. Use a period or semicolon instead.
As per coding guidelines, "Documentation and committed text must avoid Markdown bold emphasis, emojis, em dashes, and non-ASCII characters."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compute-plane-services/byoo-otel-collector/perf/README.md` around lines
10 - 12, Replace the em dash in the README sentence describing pass/fail
thresholds with ASCII punctuation, such as a period or semicolon, while
preserving the sentence’s meaning.
Source: Coding guidelines
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
TL;DR
Completes the performance suite's measurement milestone:
runnow measures the collector over the profile's window and emits a baseline (per-signal throughput, drops, end-to-end delivery, collector CPU/memory, and pod health) as a human summary and JSON. No pass/fail thresholds yet — the goal is a reproducible baseline. Stacked on #624.Additional Details
pkg/report— a small Prometheus text-exposition parser plus baseline computation. Metric-name suffixes (notably_total) and spellings vary across collector-contrib versions, so it matches a list of candidate names per concept and records any missing series as notes rather than failing. It computes, per signal (logs/metrics): collector accepted/refused, exporter sent/failed, sink accepted, throughput (sink/window), and delivery ratio (sink/collector); plus collector CPU (avg cores fromprocess_cpu_seconds) and RSS, and pod restart/OOM health. Emits a summary and indented JSON.pkg/deploy—ScrapePodMetricsreads a pod's Prometheus endpoint through the API-server proxy, so it works on k3d without a metrics-server, ingress, or port-forward. AddsPodHealth, and splitsRunLoadintoStartLoad/WaitLoadso metrics can be sampled while load is in flight.run— after warmup, snapshots the collector (metricsport) and sink (8888) at the start and end of the window, builds the report, prints the summary, and writes<results-dir>/<shape>.jsonwhen--results-diris set. New flag:--results-dir.Limitations / to validate on a cluster
warmup + window), so adev-profile run takes ~40s per shape.For the Reviewer
pkg/report/report.go(candidate metric names + derived stats) andcmd/perf/main.gomeasure.ScrapePodMetrics) can't be exercised by the fake clientset; it is covered by the parser/report unit tests plus manual cluster runs.For QA
make perf-testpasses (parser + report computation unit-tested, incl. counter-reset guard and missing-metric handling).perf run --shape container --results-dir ./results) should be run against a cluster.Issues
NO-REF
Checklist
Summary by CodeRabbit
New Features
ok,partial, andinvalidstatuses.Documentation
Maintenance
0.157.16.