feat(metrics): show network storage usage - #164
pjcdawkins wants to merge 3 commits into
Conversation
The metrics API reports network storage volumes (used by "storage" mounts) under the "storage" mountpoint, but metrics:all and metrics:disk-usage only read "/mnt" and "/tmp". On projects using storage mounts, the only disk the customer sizes could not be monitored from the CLI, and resources:get "Disk" could not be compared to any metrics column. - metrics:all: add storage_used, storage_limit, storage_percent and storage_inodes_* columns. - metrics:disk-usage: add storage_used, storage_limit, storage_percent and storage_i* columns, and a --storage report option. - storage_percent is a default column only when a service reports storage, so output is unchanged for other projects. - Existing disk_* columns are not renamed. - The api.metrics_storage config key (default true) turns this off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Boolean environment parsing must be fixed so api.metrics_storage=0 actually disables storage metrics.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds network storage usage metrics to legacy metrics commands, including configurable output and integration coverage.
Changes:
- Adds storage usage and inode columns to
metrics:allandmetrics:disk-usage. - Adds the
--storagereport option. - Adds the
api.metrics_storageconfiguration flag. - Adds integration tests for storage output and disabling behavior.
The environment override currently fails to disable the feature because "0" is cast to true; this requires correction.
| File | Summary |
|---|---|
legacy/src/Command/Metrics/MetricsCommandBase.php |
Storage detection and configuration handling |
legacy/src/Command/Metrics/DiskUsageCommand.php |
Storage columns and --storage option |
legacy/src/Command/Metrics/AllMetricsCommand.php |
Storage metrics in combined output |
legacy/config-defaults.yaml |
Enables storage metrics by default |
integration-tests/metrics_test.go |
Tests storage output and configuration disabling |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 🔵 2 minor points · ⚪ 1 nitpick
🔍 Full review · 5 files reviewed
⚪ Nitpick
legacy/src/Command/Metrics/DiskUsageCommand.php:183—storageFields()is duplicated verbatim in DiskUsageCommand and AllMetricsCommand (AllMetricsCommand.php:245), differing only in the key names (storage_iusedvsstorage_inodes_used). Both construct the same six SourceField/SourceFieldPercentage pairs overself::STORAGE_MOUNTPOINT; a future change (e.g. a different aggregation) has to be made twice or the two commands silently diverge.
Verification
$fields += $this->storageFields($bytes)cannot clobber existing entries: nostorage_*key collides with the existing disk_/tmp_/inodes_ keys in either command.- The new
api.metrics_storage: truelives in legacy/config-defaults.yaml, which Config always merges under the user config, sogetBool('api.metrics_storage')cannot throw 'Configuration not defined'. TEST_CLI_API_METRICS_STORAGE=0is picked up by Config::applyEnvironmentOverrides' generic second-level scalar map and cast to boolean false, so the disabled test really exercises the disabled path.- Expected test values match the code: 920/1000 and 30/100 render as
92.0%and30.0%via Format::Percent, and-Brenders storage_used as920via Format::Rounded. --tmpand--storageare rejected together before either setscolumns, so the two report column lists can never be merged.
The diff adds integration-tests/metrics_test.go covering the storage columns, the --storage report and the api.metrics_storage=0 path; it runs in the CI integration-test job (make integration-test, after building the phar), while the legacy-php job runs php-cs-fixer, phpstan (level 8) and PHPUnit over the changed PHP. No PHP unit test covers MetricsCommandBase::storageColumns() directly, and nothing covers a storage mountpoint that omits inode metrics.
Review details
- Commit: 0efbcec
- Model: claude-opus-5
Review 1 of 10 for this pull request · View the full run
- Remove the disk --storage option. Instead, disk shows storage_used, storage_limit and storage_percent by default (metrics:all shows storage_percent). - In tables, storage columns are shown only if a service reports storage. Machine-readable formats always include them, for stable output. - Return empty values instead of failing when the storage mountpoint lacks a metric (e.g. inodes). - Share storageFields() between both commands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
📋 PR Summary Adds network storage usage reporting to the metrics commands. Changes
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 🔵 2 minor points
🔁 Incremental · 4 files reviewed
Verification
extractValue()'s storage early return makes the db fixture's inode-lessstoragemountpoint yield blank cells instead of the previous RuntimeException.storageFields($bytes, $inodesPrefix)producesstorage_inodes_*for metrics:all andstorage_i*for disk, matching each command's TABLE_HEADER keys.$fields += $this->storageFields(...)adds no key that collides with an existing field in either command.- The disabled branch of
storageColumns()filtersstorage_*out of the header while the fields array is also skipped, so no row key is rendered without a header.
Covered by the rewritten integration-tests/metrics_test.go table test (storage present/absent, csv determinism, TEST_CLI_API_METRICS_STORAGE=0), run by the integration-test job in .github/workflows/ci.yml via make integration-test; no test covers a storage mountpoint that reports a limit without the matching used value.
Review 2 of 10 for this pull request · View the full run
A limit without a matching used value was shown as 0%. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 2 files reviewed
Outstanding from earlier reviews:
- 🔵 #4086400234 —
legacy/src/Command/Metrics/MetricsCommandBase.php:326: Column-count change silently breaks positional parsers of csv/tsv output. — Code unchanged: storageColumns() still appends the storage columns for machine-readable formats regardless of whether any service reports a storage mountpoint; author states this is intentional for stable column sets.
Verification
- getValueFromSource now returns null when the percentage numerator is missing, so a storage volume reporting only disk_limit renders blank instead of 0.0%.
- The mirror case (used present, limit absent) still returns null via
$limit > 0, so both halves of the partial-data case now render identically blank. - Format::format/formatPercent return '' for null, so the new null propagation renders an empty cell rather than erroring.
- The updated fixture (db storage with inodes_limit but no inodes_used) exercises the new branch, matching the expected trailing-blank column in 'all storage columns' and 'disk-usage storage columns'.
- extractValue's storage-mountpoint branch returns null rather than throwing, so the added inodes_limit-only fixture cannot abort buildRows.
integration-tests/metrics_test.go covers the changed branch via the db fixture with partial inode metrics (TestMetricsStorage), run by the integration-test job in .github/workflows/ci.yml (make integration-test); no PHPUnit test in the diff covers getValueFromSource directly, though the legacy-php job runs phpstan/php-cs-fixer over the changed file.
Review 3 of 10 for this pull request · View the full run
The metrics API reports network storage volumes (used by
storagemounts) under astoragemountpoint, butmetrics:allandmetrics:disk-usageonly read/mntand/tmp. On projects using storage mounts, storage usage was not visible in the CLI, and theDiskallocation inresources:gethad no matching metrics column (disk_*refers to/mnt).Changes:
metrics:all: addstorage_used,storage_limit,storage_percentandstorage_inodes_*columns.storage_percentis a default column.metrics:disk-usage: addstorage_used,storage_limit,storage_percentandstorage_i*columns.storage_used,storage_limitandstorage_percentare default columns.disk_*columns are not renamed.api.metrics_storageconfig key (defaulttrue) turns this off.Note:
--latestcan pick a partial data point, leaving storage (and disk) columns blank; #49 addresses that separately.🤖 Generated with Claude Code