Repository navigation
feat(stats): report cpuUtilization in the machine-readable formats - #2146
Conversation
container stats already samples twice, sleeping two seconds between samples, and computes a CPU percentage from the pair for its table. Only the second sample was rendered for json, yaml and toml, so a consumer paid that latency and received neither the percentage nor the earlier sample needed to derive it. Getting a CPU percentage from the CLI meant invoking the command twice and diffing cpuUsageUsec by hand. Adds a StatsReport payload for the non-table formats: the latest sample plus the derived percentage. ContainerResource.ContainerStats models a single sample and a rate is not a property of one sample, so the derived value lives in the command rather than widening that type. The two-second interval is now a single constant. collectStats sleeps for it and the percentage divides by it, so the two must agree; they were separate literals before.
kavix
left a comment
There was a problem hiding this comment.
Thanks for working on this, @guanchzhou! Adding cpuPercent to the machine-readable output is a useful improvement, and consolidating the hardcoded 2-second sleep into sampleInterval also helps prevent sampling drift.
A few small suggestions that could improve maintainability
- Deduplicate CPU calculation
Consider moving cpuPercent into a computed property on StatsSnapshot. This would allow both StatsReport and statsTable to share the same calculation logic and avoid duplication. - Avoid field duplication in StatsReport
Rather than manually mirroring all properties from ContainerResource.ContainerStats (and potentially missing fields added in the future), consider wrapping stats2 directly and implementing a custom encode(to:). This would make the implementation more resilient to future changes. - Add test coverage
Please consider adding or updating unit/snapshot tests to verify that --format json includes the expected cpuPercent field.
|
Thanks @kavix — took all three.
|
Review asked for one calculation, no field mirroring on StatsReport, and tests that json carries cpuPercent. TOMLEncoder cannot merge two keyed containers, so encode flattens the wrapped sample in one pass.
9746cab to
0a12f00
Compare
|
@katiewasnothere, could you take a look at this one when you have time? It addresses the earlier review feedback and has been unchanged since 30 August. Happy to rebase if needed. |
jglogan
left a comment
There was a problem hiding this comment.
@guanchzhou Thanks! This seems reasonable; only thing I'm suggestion is to report fractional utilization in JSON, and do the percentage presentation for the tabular result only.
| /// - cpuUsage2: CPU usage from the second sample | ||
| /// - timeInterval: Wall-clock interval between samples | ||
| /// - Returns: CPU percentage where 100% = one fully utilized core | ||
| static func calculateCPUPercent( |
There was a problem hiding this comment.
Let's call this calculateCPUUtilization that returns the fractional utilization of the CPU, with 1.0 representing full utilization of one core.
Percentages are a presentation concern - compute those for tabular output but the JSON output should only have cpuUtilization, and the caller can decide how to format the raw data.
There was a problem hiding this comment.
Please update the StatsReport docc and the PR text for the modification as well.
There was a problem hiding this comment.
Done in 9b43c1d. calculateCPUUtilization returns the fraction of one core (1.0 = one fully used core). JSON, YAML and TOML carry only cpuUtilization; the table multiplies by 100 for Cpu %.
There was a problem hiding this comment.
Updated the StatsReport doc comment, the PR title and description, and docs/resource-usage.md, which now shows cpuUtilization in the JSON example and explains the unit.
| return ContainerStats.calculateCPUPercent( | ||
| cpuUsage1: .microseconds(first), | ||
| cpuUsage2: .microseconds(second), | ||
| timeInterval: ContainerStats.sampleInterval |
There was a problem hiding this comment.
We could make this a little more accurate by recording ContinuousClock.Instant times for both samples and using the delta instead of the constant here.
I wouldn't require it for this PR; might be more suitable for a follow-on issue.
There was a problem hiding this comment.
Folded this in rather than leaving a follow-on. Each container's two samples are timestamped with ContinuousClock and the utilization divides by the measured delta. If the second sample fails there is no interval, so the value is reported as unavailable instead of 0.
…vals Per review, the machine-readable formats carry `cpuUtilization`, a fraction of one core (1.0 is one fully used core), and only the table renders a percentage. `calculateCPUPercent` becomes `calculateCPUUtilization`. Each container's two samples are now timestamped with ContinuousClock, and the utilization divides by the measured delta instead of the nominal two-second sleep. When the second sample fails there is no interval, so the value is reported as unavailable rather than 0. Update the StatsReport documentation and docs/resource-usage.md. Signed-off-by: Andrey Maltsev <maltsev.andrey@gmail.com>
jglogan
left a comment
There was a problem hiding this comment.
The elapsed time enhancement looks good. Could you add a test case to cover the nil path?
| @@ -156,48 +156,57 @@ extension Application { | |||
| fileprivate struct StatsSnapshot { | |||
There was a problem hiding this comment.
Can you give this internal visibility and add a unit test for the nil case?
There was a problem hiding this comment.
Done in 111396e. StatsSnapshot and StatsReport.init(snapshot:) are internal now, and the new StatsSnapshotTests suite covers the nil cases: no second sample (zero interval), and missing cpuUsageUsec in either sample. It also checks that the measured interval is used and that StatsReport(snapshot:) leaves cpuUtilization out of the JSON when it is nil.
Give StatsSnapshot internal visibility and add unit tests for the cases where cpuUtilization is nil. Signed-off-by: Andrey Maltsev <maltsev.andrey@gmail.com>
testStatsAllContainers reads stats for every running container, so the cpuUtilization assertion also covered containers from tests running in parallel. One of those can stop between the two samples and correctly report no utilization. Limit the assertion to the two containers the test creates. Signed-off-by: Andrey Maltsev <maltsev.andrey@gmail.com>
Code Coverage
|
Fixes #2144
container statsalready samples twice, sleeping two seconds between samples, and computes CPU usage from the pair for its table.Output.renderreceived onlystats2, sojson,yamlandtomlconsumers paid that latency and got back neither the derived value nor the earlier sample needed to compute it. Getting CPU usage from the CLI meant invoking the command twice and diffingcpuUsageUsecby hand.Change
A
StatsReportpayload for the non-table formats: the latest sample pluscpuUtilization, the fraction of one core used between the two samples (1.0is one fully used core,2.5is two and a half). Percentages are a presentation concern, so only the table renders one (cpuUtilization * 100); machine-readable consumers get the raw fraction and format it themselves.ContainerResource.ContainerStatsis documented as modelling a single sample, and a rate is not a property of one sample, so the derived value lives in the command rather than widening that type. The wire shape of the existing fields is unchanged;cpuUtilizationis additive.calculateCPUUtilization(cpuUsage1:cpuUsage2:timeInterval:)replacescalculateCPUPercentand returns the fraction. Each container's two samples are timestamped withContinuousClock, and the utilization divides by the measured delta rather than the nominal two-second sleep, so request latency between samples no longer skews the result. If the second sample fails there is no interval to divide by, and the value is reported as unavailable (--in the table, omitted from json) rather than as 0.The
StatsReportdocumentation anddocs/resource-usage.mddescribe the new field.Verification, end to end with this build
A container allocated two CPUs running two busy loops:
Unit tests cover the calculation (one core, four cores, a non-nominal interval, no change, counter decrease) and the json, yaml and toml encodings, including TOML's
[[items]]wrapper for top-level arrays.swift test --filter 'CalculateCPUUtilizationTests|StatsReportEncodingTests'passes 10 tests in 2 suites, andmake swift-fmt-checkis clean.Left for your call
stats1is still dropped. Emitting both samples would let a consumer compute other rates (network and block I/O per second) the same way. I kept this PR to the one value the table already shows; say the word if the fuller shape is preferable.Environment: macOS 27.0 (Tahoe), Apple silicon, Swift 6.4.
Per CONTRIBUTING: I used AI assistance while investigating and drafting this, and I can explain and justify every line.