This code was missing a check for the ID field before truncating it to a
shorter length for presentation. This would result in a panic if an event
would either have an empty ID field or a shorter length ID;
panic: runtime error: slice bounds out of range [:12] with length 0
goroutine 82 [running]:
github.com/docker/cli/cli/command/container.RunStats.func2({{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x40001fcba0, 0x9}, {0x40001fcba9, 0x5}, ...})
/go/src/github.com/docker/cli/cli/command/container/stats.go:146 +0x1d0
created by github.com/docker/cli/cli/command/container.(*eventHandler).watch in goroutine 6
/go/src/github.com/docker/cli/cli/command/container/stats.go:363 +0x1c8
We need to look at this code in general; the truncated ID is passed to
NewStats, which uses the ID to propagate the `Container` field in the
`StatsEntry` struct. which is not used in the default format used by
`docker stats` and, having the same content as the `ID` field on the
same struct, doesn't make it very useful, other than being able to
present it under a `CONTAINER` column (instead of `CONTAINER ID`);
we should consider deprecating it; there may be some subtle things
to look into here; the `Container` field originally held the container
name. This was changed in [moby@ef915fd], which introduced separate
`ID` and `Name` fields, renaming the old `Name` field to container.
Looking at [`Stats.SetStatistics()`] and related code in [stats_helpers.go],
the `Container` field is used as the "canonical" reference for the stats
record; this allows the stats _data_ to be refreshed when a new stats
sample arrives for the same container (also see [moby@929a77b], which
moved locking to the `Stats` wrapper struct). This construct allows to
account for intermediate states, where a stats sample was incomplete
or could produce an error; in that case, the reference to the container
for which the stats were sampled is kept to allow removing a container
from the list once the container was removed. We should consider removing
`Container` as a formatting option, and moving the `Container` field to
the outer struct; this makes the outer struct responsible for keeping a
reference to the container, allowing the `StatsEntry` as a whole to be
replaced atomically.
This patch only addresses the panic;
- It changes the logic to preserve the container ID verbatim instead
of truncating. This allows stats samples to be matched against the
`Actor.ID` as-is.
- Truncating the `Container` is moved to the presentation logic;
currently this does not take `--no-trunc` into account to keep
the existing behavior, but we can (should) consider adding this.
- Logging is improved to use structured logs, and an extra check is
added to prevent empty IDs from being added as watcher.
[`Stats.SetStatistics()`]: 82281087e3/cli/command/container/formatter_stats.go (L88-L94)
[moby@ef915fd]: ef915fd036
[moby@929a77b]: 929a77b814
[stats_helpers.go]: 82281087e3/cli/command/container/stats_helpers.go (L26-L51)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
We were depending on pkg/stringid to truncate IDs for presentation. While
traditionally, we used a fixed length for "truncated" IDs, this is not
a strict requirement (any ID-prefix should work, but conflicts may
happen on shorter IDs).
This patch adds a local `TruncateID()` utility in the formatter package;
it's currently using the same implementation and length as the
`stringid.TruncateID` function, but may diverge in future.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Formatting stats runs in a loop to refresh the stats for each container. This
patch makes some small performance improvments by reducing the use of Sprintf
in favor of concatenating strings, and using strconv directly where possible.
Benchmark can be run with:
GO111MODULE=off go test -test.v -test.bench '^BenchmarkStatsFormat' -test.run '^$' ./cli/command/container/
Before/after:
BenchmarkStatsFormatOld-8 2655 428064 ns/op 62432 B/op 5600 allocs/op
BenchmarkStatsFormat-8 3338 335822 ns/op 52832 B/op 4700 allocs/op
Average of 5 runs;
benchstat old.txt new.txt
name old time/op new time/op delta
StatsFormat-8 432µs ± 1% 344µs ± 5% -20.42% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
StatsFormat-8 62.4kB ± 0% 52.8kB ± 0% -15.38% (p=0.000 n=5+4)
name old allocs/op new allocs/op delta
StatsFormat-8 5.60k ± 0% 4.70k ± 0% -16.07% (p=0.008 n=5+5)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- make it possible to extract the formatter implementation from the
"common" code, that way, the formatter package stays small
- extract some formatter into their own packages
This is essentially moving the "formatter" implementation of each type
in their respective packages. The *main* reason to do that, is to be
able to depend on `cli/command/formatter` without depending of the
implementation detail of the formatter. As of now, depending on
`cli/command/formatter` means we depend on `docker/docker/api/types`,
`docker/licensing`, … — that should not be the case. `formatter`
should hold the common code (or helpers) to easily create formatter,
not all formatter implementations.
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
