Chart Disk Perf Example Results in the Docs - #3983
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3983 +/- ##
==========================================
+ Coverage 96.44% 96.45% +0.01%
==========================================
Files 437 437
Lines 27417 27417
Branches 631 627 -4
==========================================
+ Hits 26441 26445 +4
+ Misses 470 469 -1
+ Partials 506 503 -3 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
DiskPerfChart.vue is essentially a copy of LineChart.vue. Diffing the two templates, all 140 lines of the new one already exist in LineChart.vue's 161. Only three things really differ: the y tick label goes through label(t) for the
thousands suffix, the curve and legend line carry blaze-chart-line-dashed, and there's no y2 axis.
The script blocks duplicate as much. MARKER_RADIUS and its comment are byte-identical, and xAxis, yAxis, plot, x, y, curves (including the ${i === 0 ? "M" : "L"} path builder) and description are the same computations with different inputs. BarChart.vue repeats the same frame scaffolding a third time: the SVG wrapper, title, gridlines, tick groups, rotated axis labels, legend block, and the (yLabel ? 26 : 8) + maxTickWidth + 8 gutter.
I'd suggest pulling the frame into a ChartFrame.vue (title, grid, ticks, axes, labels, legend; marks via slot) plus a small LineSeries.vue, with Tick/Curve types and ticks()/gutter()/linePath() in plot.ts. The label(t) vs tick(t) difference then disappears: the formatter is passed in where the ticks are built.
alexanderkiel
left a comment
There was a problem hiding this comment.
Review
Replaces the stale hand-written "Example Results" table in docs/performance/disk-perf.md with charts and stat tiles rendered at build time from the Parameters resources of the $disk-perf operation, committed under docs/performance/disk-perf/. Adds a third system (A5N46) and per-system prose.
Verification
I built the site (make -C docs build) and checked the output:
- Build exits 0.
performance/disk-perf.htmlrenders 6 SVGs and 3 stat blocks with no SSR errors. (TheCannot read properties of undefined (reading 'substring')errors in the build log come fromterminology-service.mdandvalidation_external-validator.md— pre-existing, unrelated to this PR.) - The parser in
disk-perf.tsmatches the producer injob-disk-perf/src/blaze/job/disk_perf.cljexactly —seq-write-throughput,rand-read/concurrency/iops/throughput/latency-p50|95|99|max,fsync-rate,fsync-latency-*,direct-io,score,rating. - I extracted every value from the three JSONs and checked all 15 numbers in the new prose. All correct: A5N46 18.8 k @ 55 µs / 386 k @ 32 / 178 fsync/s @ 5.4 ms; LEA47 2.2 k @ 441 µs / 460 MiB/s / 484 fsync/s @ 1.9 ms / score 33.5; LEA79 440 k @ 32 (ref 320 k) / p50 65.5–71.5 µs / 10.7 GiB/s / 122 k fsync/s @ 8.2 µs. "Staying above the reference curve at every level" holds for A5N46 at all six levels.
- The defaults sentence (4 GiB, 30 s, max concurrency 32) matches
default-file-size/default-phase-duration-millis/default-max-concurrency. - The cross-references to
load-testing.md#transactionresolve, and both throughput claims check out (A5N46 plateaus at 90–100 req/s, LEA79 at 5300–5600). The fsync explanation is nicely consistent: 177.6 fsync/s over two WALs ≈ 89/s, right at the observed plateau. - Legend placement doesn't collide with any curve on any of the six charts (legend occupies x≈68–130, where all curves sit well below it).
Good work overall — the mechanism does what #3981 asked for, and the prose is a real improvement over the old table.
Main point: DiskPerfChart.vue duplicates LineChart.vue
93% of DiskPerfChart.vue's template (124 of 133 non-blank lines) is identical to LineChart.vue's — title, grid, tick labels, axis lines, curve/marker rendering, axis labels and the legend group are copied verbatim. MARKER_RADIUS and its two-line comment are copied word for word, and plot, description, xAxis/yAxis and the path-building in curves are near-identical.
AGENTS.md says "Avoid code duplication. Use existing functions if possible. Create a function if code is used more than two times." This is now the third chart component, and it's the point where the SVG frame should be extracted rather than copied a third time. Suggestion: a Plot.vue taking {title, xAxis, yAxis, xLabel, yLabel, curves, tickLabel} and rendering the frame, leaving LineChart and DiskPerfChart to do only what actually differs — building curves from their respective data sources, the optional right axis, and the dashed-reference styling.
That keeps a real bonus: the next chart type is cheap, and a fix to the frame (say, tick collision) lands everywhere at once instead of in two places.
Smaller points
1. bestReadRun throws an opaque error on an empty sweep (disk-perf.ts:139-143)
diskPerf() validates every scalar with a named error, but readRuns is the one field it doesn't: filter(...).map(...) on a resource without rand-read parameters silently yields []. Then reduce with no seed throws TypeError: Reduce of empty array with no initial value, and Math.min(...[]) in xAxis returns Infinity, so the chart path becomes NaN. A build failure on a bad results file would point at the reducer rather than at the file.
Cheap fix, consistent with the rest of the module — validate in diskPerf():
const readRuns = parameters
.filter((parameter) => parameter.name === "rand-read")
.map((parameter) => readRun(parameter.part ?? []))
.sort((a, b) => a.concurrency - b.concurrency);
if (readRuns.length === 0) {
throw new Error(`missing parameter: rand-read`);
}Related edge case in the same area: a sweep with a single level makes logAxis(1, 1, [1]) and scale(..., log = true) divide by a zero span, again yielding NaN. Only reachable with max-concurrency 1, so it's fine to leave, but it's the same missing guard.
2. Reference-curve constants now live in a third place (disk-perf.ts:128-129)
REFERENCE_IOPS_PER_READER/REFERENCE_MAX_CONCURRENCY duplicate reference-iops-per-reader/reference-max-concurrency in blaze.job.disk-perf.score, which modules/frontend/src/lib/jobs/disk-perf/read-iops-chart.svelte:19-20 already duplicates too — plus a fourth restatement in the prose of the "Score" section on this same page. If the reference ever changes, the docs chart silently plots the wrong target while claiming it's "the reference curve the score is computed against". Can't be shared across the three build units, but a comment pointing at blaze.job.disk-perf.score as the source of truth (in both TS copies) would at least make the coupling greppable.
3. Binary vs. decimal units on the same page
The stat tiles use binary units (460 MiB/s), matching the admin UI's prettyBytes({binary: true}) — good. But the "Score" section a few paragraphs up says sequential writes are "normalized against 1 GB/s", which is decimal (1.0e9 in score.clj). A reader comparing LEA47's 460 MiB/s against 1 GB/s reads the ratio as 0.46 when it's actually 0.48. Minor, and the old table said MB/s for the same quantity, so the page got less consistent here rather than more. Either note the unit convention once or state the write reference in binary.
4. A5N46's SSD column is just "4 TB"
The other two rows name the drive, and the prose makes a drive-specific claim about A5N46 ("the drive acknowledges a sync only once the data has reached the flash instead of from a write cache"). Naming the model would let a reader act on that. (Same blank exists in load-testing.md, so this may be deliberate.)
5. Prose numbers are still hand-written
The charts and tiles can't drift, which is what #3981 asked for, but the fifteen numbers in the surrounding paragraphs can. They're all correct today; just worth being aware that a re-measurement means re-reading the prose too.
6. Shadowing in parameter() (disk-perf.ts:59-65)
const parameter shadows the enclosing function parameter, and the find callback parameter shadows it again. Renaming the local to found and the callback arg to p would read more easily.
7. Bundle (informational)
The three JSONs are pretty-printed and get inlined verbatim into the shared theme chunk that every docs page loads — ~31 KB raw, about a quarter of that chunk. Gzipped it's only ~2.1 KB, so the real cost is small; I mention it only because data.ts's own header comment treats bundle size as a design constraint. Compacting them would cut it to ~1.7 KB gzipped, at the cost of the "taken as they came out of the server" property, which is probably not worth trading away.
Summary
No correctness bugs in the rendered output — the data pipeline is sound and every number checks out. The one thing I'd want addressed before merge is the template duplication (main point); the rest are nits, of which #1 is the most worthwhile because it's three lines.
Generated by Claude Code
022c49a to
c8f6ff7
Compare
Closes: #3981 Signed-off-by: Alexander Kiel <alexanderkiel@gmx.net>
c8f6ff7 to
23cd77c
Compare
|
I worked on your points @knoppiks and the points from Claude. |
Closes: #3981