fix: Skip extra header columns in iostat parsing#4676
fix: Skip extra header columns in iostat parsing#4676InoMurko wants to merge 1 commit intoOffchainLabs:masterfrom
Conversation
Handle cases where header has extra columns in iostat.
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4676 +/- ##
==========================================
+ Coverage 52.38% 59.09% +6.70%
==========================================
Files 506 506
Lines 73739 60417 -13322
==========================================
- Hits 38630 35703 -2927
+ Misses 29951 19452 -10499
- Partials 5158 5262 +104 |
| if i >= len(data) { | ||
| // Some iostat builds emit a header with more columns than a data row | ||
| // (e.g. trailing header tokens without values). Skip the rest of the header. | ||
| break | ||
| } |
There was a problem hiding this comment.
Are we okay with "skipping" the missing field? The break plus partial send means any field whose column index >= len(data) gets published as 0.0 and overwrites the gauge. I think it would be overkill to track which fields are missing so maybe, a good medium, we could add a log.Warn so the failure is at least observable? Something like:
if i >= len(data) {
log.Warn("iostat row has fewer columns than header",
"headerCols", len(fields), "dataCols", len(data),
"header", fields, "row", data)
break
}| stat := DeviceStats{} | ||
| var err error | ||
| for i, field := range fields { | ||
| if i >= len(data) { |
There was a problem hiding this comment.
Given this fixes a production panic, it'd be great to add a regression test so a future refactor can't silently reintroduce the bug. The package has no _test.go file today so maybe we can introduce something like util/iostat/iostat_test.go. The current Run is awkward to unit-test because it shells out to iostat directly. One option is to extract the scanner/parse loop (roughly the body from var fields []string through the scanner.Err() check) into a helper:
func parseStream(r io.Reader, receiver chan<- DeviceStats)Then Run becomes process-setup + parseStream(stdout, receiver) + teardown. A test can drive parseStream with strings.NewReader(...) against different inputs. A few cases worth covering:
- Well-formed row (baseline)
- The regression: header has more columns than the data row (the exact panic from the stack trace)
- A header that starts with
Device:(older sysstat prints the trailing colon)
Happy to push a sketch if helpful.
| // Some iostat builds emit a header with more columns than a data row | ||
| // (e.g. trailing header tokens without values). Skip the rest of the header. |
There was a problem hiding this comment.
nitpick: reword comment:
| // Some iostat builds emit a header with more columns than a data row | |
| // (e.g. trailing header tokens without values). Skip the rest of the header. | |
| // Some iostat builds emit a header with more columns than a data row. | |
| // Stop matching schema fields once the data row is exhausted. |
3 participants
Skip parsing when a stats row has fewer fields than the last Device header row.
It breaks when i >= len(data), i.e. when this data row has fewer whitespace-separated fields than the header has columns (or than we’ve consumed so far).