[fix] Skip storage capacity/utilization gauges when capacity is None#138
[fix] Skip storage capacity/utilization gauges when capacity is None#138huniu20 wants to merge 1 commit into
Conversation
CLA Signature Passhuniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
65daf9d to
e0386b0
Compare
CLA Signature Passhuniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
There was a problem hiding this comment.
Pull request overview
This PR fixes Prometheus metrics collection for “unlimited capacity” storage units by avoiding publishing capacity/utilization gauge values when the storage unit reports capacity=None, preventing periodic float(None) errors and log flooding.
Changes:
- Skip setting
tq_storage_capacity_totalwhencapacity is None. - Skip setting
tq_storage_utilization_ratiowhencapacity is None(while continuing to export other storage metrics).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ``capacity`` is ``None`` when the storage unit is configured with | ||
| # unlimited capacity (``total_storage_size=None``). | ||
| if capacity is not None: | ||
| self.storage_capacity.labels(storage_unit_id=label).set(capacity) | ||
| self.storage_utilization.labels(storage_unit_id=label).set( | ||
| active / capacity if capacity > 0 else 0.0 | ||
| ) | ||
| self.storage_active_keys.labels(storage_unit_id=label).set(active) |
| # ``capacity`` is ``None`` when the storage unit is configured with | ||
| # unlimited capacity (``total_storage_size=None``). | ||
| if capacity is not None: | ||
| self.storage_capacity.labels(storage_unit_id=label).set(capacity) | ||
| self.storage_utilization.labels(storage_unit_id=label).set( |
| self.storage_capacity.labels(storage_unit_id=label).set(capacity) | ||
| self.storage_utilization.labels(storage_unit_id=label).set( | ||
| active / capacity if capacity > 0 else 0.0 | ||
| ) |
There was a problem hiding this comment.
We can have an else here to deal with the remove logic. For example
else:
for gauge in (self.storage_capacity, self.storage_utilization):
try:
gauge.remove(label)
except (KeyError, ValueError):
pass
There was a problem hiding this comment.
Done. Added an else branch to prune the stale storage_capacity / storage_utilization series when a unit transitions to unlimited, mirroring the existing partition-level pruning. Also covered Copilot's suggestion with two unit tests in test_metrics.py.
Follow-up of Ascend#130, which made ``total_storage_size`` optional. When a storage unit is configured with unlimited capacity (``total_storage_size=None``), ``SimpleStorageUnit._handle_get_metrics`` reports ``capacity=None``. The ``storage_capacity`` and ``storage_utilization`` ``prometheus_client.Gauge`` objects defined in ``transfer_queue/metrics.py`` cannot accept ``None`` (``float(None)`` raises ``TypeError``), which triggers a ``Failed to collect metrics from storage unit ...`` warning every ``TQ_METRICS_COLLECT_INTERVAL`` seconds for every unlimited-capacity storage unit, polluting the logs. Skip the capacity and utilization gauges in that case; the remaining metrics (``active_keys``, ``memory_rss``, per-op stats) are still reported correctly. If a storage unit previously reported a numeric capacity and later transitions to unlimited (e.g. after re-registration), also remove the stale ``storage_capacity`` / ``storage_utilization`` series so dashboards do not keep serving outdated values, mirroring the stale-label pruning already applied to partition-level gauges. Signed-off-by: nexhu <nexhu@tencent.com>
e0386b0 to
a43cb6d
Compare
CLA Signature Passhuniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
Background
Follow-up of #130, which made
total_storage_sizeoptional so that a storage unit can be configured with unlimited capacity.Problem
When a storage unit is created with
total_storage_size=None(unlimited),SimpleStorageUnit._handle_get_metricsreportscapacity=Nonein its/metricspayload.The metrics exporter in
transfer_queue/metrics.pydefines twoprometheus_client.Gaugeobjects that consume this value:self.storage_capacity = Gauge("tq_storage_capacity_total", ...)— around L164self.storage_utilization = Gauge("tq_storage_utilization_ratio", ...)— around L170Inside
TQMetricsExporter._collect_storage_metrics, both gauges are updated unconditionally:When
capacity is None,Gauge.set()internally doesfloat(value), andfloat(None)raisesTypeError. Because the whole block is wrapped intry/except, this is caught by the surrounding handler and turned into:This warning is emitted every
TQ_METRICS_COLLECT_INTERVALfor every unlimited-capacity storage unit, flooding the logs and hiding real issues.Fix
In
TQMetricsExporter._collect_storage_metrics, treatcapacity is Noneas "unlimited" and skip only thestorage_capacityandstorage_utilizationgauges for that storage unit. All other metrics (storage_active_keys,storage_memory_rss, per-op request stats such as count / latency avg / p50 / p99) are still reported normally, so dashboards for unlimited-capacity units keep working — they simply no longer publish a (meaningless) capacity/utilization value.Related
total_storage_sizeoptional to support unlimited storage capacity #130 Maketotal_storage_sizeoptional to support unlimited storage capacity