Repository navigation
feat(metrics): show network storage usage - #164
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 (the 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
Resolve conflicts with the --latest fix (#165) and the observability entrypoint lookup (#118): - MetricsCommandBase: keep both STORAGE_MOUNTPOINT and LATEST_SETTLE_TIME. - Move the storage tests to metrics_storage_test.go, next to the --latest tests in metrics_test.go. - Serve the observability entrypoint (_links.resources_overview) in both tests' mocks, as the resources overview URL is now discovered there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 2 minor points
🔍 Full review · 6 files reviewed
🔵 Minor points
legacy/src/Command/Metrics/MetricsCommandBase.php:362— In table (non-machine-readable) output the storage columns are shown only if some point in$values['data']has amountpoints.storagekey. With--latestthe data has already been reduced to a single point inprocessQuery(), so on a project that does use network storage, a latest point in which no service has yet reported itsstoragemountpoint makesmetrics:all -1/disk -1drop the Storage columns entirely — the user sees a table with no storage information and no indication that the data was incomplete, rather than the blank cells the PR description predicts. Running the same command without-1shows them again, so the column set flickers between invocations.legacy/src/Command/Metrics/DiskUsageCommand.php:154—disktreats the two volumes asymmetrically: the /mnt defaults includeipercent(inode usage %), but the storage default set passed tostorageColumns()is['storage_used', 'storage_limit', 'storage_percent']and omitsstorage_ipercent, even though the command fetchesinodesmetrics for the storage mountpoint and the header defines the column. A network storage volume that is running out of inodes therefore shows nothing in the default table, while the same condition on /mnt is visible; the user must know to pass-c storage_ipercent.
Verification
- Both commands' inode prefixes ('storage_inodes_' and 'storage_i') produce keys that exactly match the storage entries added to their TABLE_HEADER constants.
- extractValue()'s new storage branch returns null instead of throwing when the storage mountpoint omits inodes_used, matching the partial 'db' fixture that expects a blank cell.
- getValueFromSource()'s
$value !== null && $limit > 0guard makes a limit-only storage entry render blank rather than 0.0%, and leaves the limit-missing case unchanged. - storageColumns()'s array_filter with ARRAY_FILTER_USE_KEY strips every storage_* key (including storage_inodes_*) from both headers when api.metrics_storage is false.
- Config::__construct merges config-defaults.yaml under the wrapper-written config.yaml, so getBool('api.metrics_storage') cannot throw 'Configuration not defined' for existing configs.
New integration tests in integration-tests/metrics_storage_test.go cover both commands, the enabled/disabled config, csv stability and the partial-inode-data case, and metrics_test.go was adapted to the merged-in observability entrypoint lookup; they run in the CI 'integration-test' job (builds the phar, then make test on integration-tests). No PHPUnit test covers storageColumns()/storageFields()/extractValue() directly; the 'legacy-php' job only runs phpstan, php-cs-fixer and the existing unit suite.
Review details
- Commit: 5a9cfe2
- Model: claude-opus-5
Review 4 of 10 for this pull request · View the full run
Matches the /mnt defaults, which include inode usage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re the latest review:
🤖 Addressed by Claude Code |
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 2 files reviewed
Outstanding from earlier reviews:
- 🔵
legacy/src/Command/Metrics/MetricsCommandBase.php:362: Storage usage silently vanishes from the default table on partial data points. — The table branch still decides from $values['data'], so with --latest a point lacking a storage mountpoint drops the columns silently. (first raised)
Verification
storage_ipercentis a real key:storageFields($bytes, 'storage_i')emitsstorage_iused/ilimit/ipercent, and DiskUsageCommand::TABLE_HEADER declaresstorage_ipercent.- Adding
storage_ipercentto the machine-readable default set keeps CSV order... tmp_percent, storage_used, storage_limit, storage_percent, storage_ipercent, matching the new test's expected header. - With
api.metrics_storage=false, storageColumns() strips allstorage_*keys from the header and leaves defaultColumns untouched, so the extra default name cannot reach columnsToDisplay(). - With
--tmp,columnsis set explicitly to tmpReportColumns, so specifiedColumns() wins and the new storage default column does not appear.
The diff adds an integration test case ('disk-usage csv always includes storage') in integration-tests/metrics_storage_test.go, run by the integration-test CI job (make integration-test, .github/workflows/ci.yml:140); no PHP-side unit test covers storageColumns() directly.
Review 5 of 10 for this pull request · View the full run
Tables previously showed storage columns only if a data point reported the storage mountpoint. Also check for mounts with "source: storage" in the current deployment, so the columns do not depend on the data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 🔵 1 minor point
🔁 Incremental · 4 files reviewed
Verification
storageColumns()now receives$environmentfrom both call sites (AllMetricsCommand.php:219, DiskUsageCommand.php:154), matching the new required parameter.Environmentis already imported in MetricsCommandBase (use Platformsh\Client\Model\Environment), so the new type hint resolves.usesStorage()adds no extra API round-trip:Api::getCurrentDeployment()is memoised inself::$deploymentsCacheand is already called bygetServices()duringprocessQuery().($mount['source'] ?? null)is safe for legacy string mount definitions ('shared:files/x'), since??suppresses the non-numeric string-offset error and yields null.- The machine-readable short-circuit in
storageColumns()meansusesStorage()is never reached for csv/json output.
Verified by the Go integration tests in integration-tests/metrics_storage_test.go, including the new 'all table shows storage when the deployment has storage mounts' case, run by the make test step of the CI workflow's Go job; no PHPUnit test covers usesStorage(), and no test exercises a deployment whose apps lack a mounts property.
Review 6 of 10 for this pull request · View the full run
- Ignore apps and workers excluded by --service or --type, so filtered tables do not gain an always-empty storage column. - Tolerate a missing or null "mounts" deployment property. - Filter by services[] in the test mock, as the API does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Take main's TestMetricsLatest (#167), which fixes the same missing observability entrypoint mock as this branch did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 1 minor point
🔍 Full review · 5 files reviewed
🔵 Minor point
legacy/src/Command/Metrics/AllMetricsCommand.php:219—metrics:alladds onlystorage_percentto the default table columns, while the /mnt and /tmp volumes each contribute both a disk-percent and an inode-percent default column (inodes_percent,tmp_inodes_percent). The command does fetchMetricKind::API_TYPE_INODESfor the storage mountpoint and definesstorage_inodes_percentin TABLE_HEADER, so a network-storage volume exhausting its inodes shows nothing in the defaultmetrics:alltable, whereas the same condition on /mnt or /tmp is visible.metrics:disk-usagealready passesstorage_ipercentin its default set (DiskUsageCommand.php:154), so the two commands disagree about the same volume.
Verification
storageFields()key names line up with both headers: 'storage_inodes_' exists in AllMetricsCommand::TABLE_HEADER and 'storage_i' in DiskUsageCommand::TABLE_HEADER.- The new
extractValue()storage branch returns null instead of throwing, so the test's partialdbstorage entry (noinodes_used) renders blank rather than aborting. getValueFromSource()'s added$value !== nullguard makes a missing used-value render blank instead of 0.0%, and still shows 0.0% for a genuine zero.usesStorage()reads mounts viagetProperty('mounts', false) ?: [], so an app whose deployment payload omits or nullsmountsdoes not throw.storageColumns()strips onlystorage_-prefixed header keys whenapi.metrics_storageis false, and neither command's$defaultColumnscontains such a key, so no default column becomes unresolvable.
Verified by the new Go integration test integration-tests/metrics_storage_test.go (table/CSV column sets, partial storage data, the disabled flag), run by the integration-test CI job; no PHP unit test covers storageColumns()/usesStorage() directly, though the legacy-php job runs phpstan, php-cs-fixer and PHPUnit over the changed classes.
Review details
- Commit: e036946
- Model: claude-opus-5
Review 8 of 10 for this pull request · View the full run
Matches the /mnt and /tmp defaults, and the disk command. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Added 🤖 Addressed by Claude Code |
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_percentandstorage_inodes_percentare default columns.metrics:disk-usage: addstorage_used,storage_limit,storage_percentandstorage_i*columns.storage_used,storage_limit,storage_percentandstorage_ipercentare default columns.storagemounts or a service reports storage. Machine-readable formats always include them (after the existing columns), for stable output.disk_*columns are not renamed.api.metrics_storageconfig key (defaulttrue) turns this off.🤖 Generated with Claude Code