Repository navigation
Report the ledger as metrics, with an optional ServiceMonitor and alerts - #12
Timofei Larkin (lllamnyp) wants to merge 1 commit into
Conversation
Two gauges count IPAddress objects by class and phase and claims by namespace, resolved class and phase, read from the cache at scrape time. The chart's metrics.enabled serves metrics over authenticated HTTPS on 8443 behind a Service, with the TokenReview and SubjectAccessReview RBAC this needs and a reader ClusterRole for scrapers; a ServiceMonitor and a PrometheusRule alerting on Conflict, Lost and long-Pending claims are optional. Assisted-By: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
IvanHunters
left a comment
There was a problem hiding this comment.
Review: report the ledger as metrics
The collector mechanics are sound: labels are bounded (class, the six-value phase enum, namespace by tenant count), so no cardinality blow-up; counts are recomputed from a cached list at scrape time so deleted objects drop out and nothing leaks; a failed list reports an invalid metric rather than a false zero; registration is single and race-free; the PromQL in the alerts is valid and the ServiceMonitor selectors line up. Good.
[MAJOR] The metrics cannot attribute held addresses to a tenant, which is the stated purpose
The design's User-facing-changes section makes the IPAddress ledger the unit of accounting: "Metering follows the claim ... the IPAddress ledger is the unit of accounting: claimRef says whose." The metrics do not deliver that:
address_controller_ipaddressesis labelled{class, phase}only (internal/controller/metrics.go:33); it dropsclaimRef.Namespace, so the ledger metric cannot answer "how many addresses does tenant X hold."address_controller_ipaddressclaimscarriesnamespace, but it counts claims, not addresses. ADualclaim holds a v4 and a v6IPAddress, so a per-namespace claim count understates the addresses actually held and billable.
So neither metric answers the per-tenant accounting the design calls for. Adding namespace (from claimRef.Namespace) as a label on address_controller_ipaddresses fixes it, and the cardinality stays bounded by the tenant count.
[MINOR] ServiceMonitor and alerts are not turnkey
Scraping needs the Prometheus ServiceAccount bound to address-controller-metrics-reader, which the chart cannot create (it does not know the SA), so with serviceMonitor.enabled alone the scrape returns 403. The ServiceMonitor also carries no label for a specific Prometheus serviceMonitorSelector and no namespaceSelector. Worth documenting the required RoleBinding and label next to the values.
NOT LGTM on the MAJOR; the rest is clean.
Adds metrics for the address ledger, raised in a review of cozystack/cozystack#3382: "Nothing reports
Conflict,LostorReleasedaddresses. Turning metrics on, adding a ServiceMonitor and alerting on those phases would make the feature usable in production." Stacked on #11.Changes
Gauges.
InventoryCollector, registered on the controller-runtime registry, counts from the informer cache at scrape time:address_controller_ipaddresses{class, phase}. This includes the driver-ownedConflictandLostphases.address_controller_ipaddressclaims{namespace, class, phase}, whereclassis the resolvedstatus.className.A failed list reports an invalid metric, so the scrape fails visibly rather than reading zero.
Chart,
metrics.enabled(default off)::8443, served over HTTPS and authorized through the controller-runtime filter already wired incmd/main.go;address-controller-metrics;address-controller-metrics-readerClusterRole (geton/metrics) to bind the scraper to.It overrides
controller.metricsBindAddress.metrics.serviceMonitor.enabled: aServiceMonitorscraping that Service with its bearer token. The certificate is self-signed, so TLS verification is skipped.metrics.prometheusRule.enabled: alerts on addresses inConflictorLostfor 5m and on claimsPendingfor 15m.Both are rendered only where the
monitoring.coreos.comAPI exists.README. A new Metrics section.
Not covered
Releasedaddresses get no alert: underRetainthey are expected, and #11'sreleaseDelayreturns them to the pool. The gauge still counts them.Testing
A unit test checks the collector's output against a fake cache.
helm lintpasses, and the templates render with and without the monitoring APIs. I have not run the scrape path against a live Prometheus.