fix(metrics): distinguish unavailable disk stats from a real zero - #91
Open
strausmann wants to merge 1 commit into
Open
fix(metrics): distinguish unavailable disk stats from a real zero#91strausmann wants to merge 1 commit into
strausmann wants to merge 1 commit into
Conversation
Collect() reported DiskTotal/DiskUsed/DiskFree as a plain 0 whenever the Docker data-root statfs call failed (e.g. the path isn't visible in the agent's mount namespace), because HostMetrics used bare uint64 fields with no omitempty. On the wire, "disk stat failed" was indistinguishable from "disk is full" or "0 bytes total". Change the three fields to *uint64 with omitempty: a nil pointer omits the field entirely on a stat failure, a non-nil pointer marshals the real value -- including a legitimate 0 -- so success and failure no longer look alike. This is wire-additive: an unchanged JSON consumer that still expects a plain "number" gets undefined/absent instead of 0, which is a strictly more useful signal and does not break decoding. collectDisk() is split into a standalone, exported DiskUsage(path) that no longer resolves the data root itself, so the statfs call is unit-testable without a live Docker client. Collect() now resolves the data root and calls DiskUsage() directly, applying the result via a small applyDiskMetrics() helper that is also independently tested.
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
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.
Problem
Collector.Collect()reportsDiskTotal/DiskUsed/DiskFreeas a plain0whenever thestatfscall on the Docker data root fails (e.g. the path isn't visible in the agent's mountnamespace, or
SKIP_DF_COLLECTIONisn't the reason).HostMetricsuses bareuint64fieldswith no
omitempty, so the JSON always includes"diskTotal":0-- indistinguishable from adisk that is genuinely full or genuinely reports 0 bytes.
Related: Finsys/dockhand#976, Finsys/dockhand#1397
Fix
Change
DiskTotal/DiskUsed/DiskFreeto*uint64withomitempty:nil→ omitted from the outgoing JSON entirely.diskFreeon a full disk), so success is never silently dropped by
omitempty.This is additive on the wire: a consumer still decoding into a plain, required
numberfield(as
strausmann/dockhand's current TSMetricsMessagetype does) getsundefinedinstead of0when the field is absent -- not a decode error, and today nothing in Dockhand actually readsthese three fields yet (verified against the current
hawser.ts), so there's no existingconsumer to break.
collectDisk()is promoted to an exported, package-levelDiskUsage(path string)that nolonger resolves the Docker data root itself --
Collect()now does that and passes theresolved path in. This makes the
statfscall directly unit-testable without a live Dockerclient, and lets a later change reuse it (see the follow-up PR extending Standard mode's
/_hawser/info).Tests
New tests in
internal/metrics/collector_test.goandinternal/protocol/messages_test.go(neither package had tests before):
DiskUsage()success/error paths (real vs. missing filesystem path)applyDiskMetrics()success/error, including the "legitimate 0" caseCollect()withSKIP_DF_COLLECTIONset0for an absent fieldgo build ./...,go vet ./...,go test ./... -raceall pass.