INC-1252: Clean up adapter metrics keyed by clusters - #38640
Conversation
### Motivation Three adapter metrics carry a cluster id label: mz_time_to_first_row_seconds, mz_determine_timestamp, and mz_timestamp_difference_for_bounded_staleness_ms. Their series were never removed when the cluster was dropped, so every dropped cluster left dead series behind and the metrics endpoint response grew without bound. ### Description Group the three metrics into `ClusterLabeledMetrics` with a `remove_cluster` method, and call it from the catalog implications finalize step next to the controller's own cluster drop. `remove_cluster` scans each metric vec for children whose cluster label matches and removes them, through a new `mz_ore::metrics::remove_children_with_label` helper. Nothing is tracked on the record path, so observations stay lock-free and the drop pays a one-off scan of each vec's children. A recording that lands after the sweep, for example a frontend peek planned against a catalog snapshot that still contained the cluster, recreates the series, which then lingers. Preventing that would mean synchronizing the record path with the drop. Metrics are best effort and the window is small, so this accepts the stale series instead. After DROP CLUSTER, the metrics endpoint no longer reports series for the dropped cluster in the three metrics above. ### Verification Unit tests for the ore helper and for `remove_cluster`, plus an environmentd test that peeks on a cluster, drops it, and checks the series are gone.
Given we don't care about metrics for clusters with 0 replicas, we can prune the metrics registry for these cases too.
03ced9e to
595b01f
Compare
|
|
||
| #[mz_ore::test] | ||
| #[allow(clippy::disallowed_methods)] | ||
| fn test_cluster_labeled_metrics_are_removed_at_zero_replicas() { |
| .try_get_cluster(cluster_id) | ||
| .is_some_and(|cluster| cluster.replicas().next().is_some()); | ||
| if !has_replicas { | ||
| self.metrics.by_cluster.remove_cluster(cluster_id); |
There was a problem hiding this comment.
This is where we remove ... but I'm curious is it possible that we drop all the replicas and then add them back.. where does the metric get registered again?
There was a problem hiding this comment.
Yup it's still possible. This can be done by setting the replication factor to 0 then back to >=1. However everything's still on the same cluster! We always key by cluster ID.
| .iter() | ||
| .map(|label| labels.get(label.as_str()).copied().unwrap_or_default()) | ||
| .collect(); | ||
| // `Err` means a concurrent removal got there first. The only other |
There was a problem hiding this comment.
What is concurrent here ... and if it is concurrent what is guarding from concurrent modification. I guess I am just confused by the Err comment
There was a problem hiding this comment.
Ah so concurrent meaning when we do vec.collect above, we acquire a lock to create a snapshot of the children. However if something deletes the child beforehand (e.g. something calls this function before), then on remove_label_values, we might have a conflict (but it doesn't matter, we just ignore the error since the conflict doesn't do any damage). I can make this more clear however.
| } | ||
|
|
||
| /// Deletes the series of `cluster_id`. | ||
| pub(crate) fn remove_cluster(&self, cluster_id: ClusterId) { |
There was a problem hiding this comment.
What is the contract here with the registration function. How do we ensure that they stay in sync? Should they stay in sync? Would it be better to have a clear marker on registration that indicates the behaviour and then we don't need the special code here per label type?
E.g., I am imagining it is possible to have the set of metric types (HistogramVec, IntCounterVec, etc), and then register with a name "time_to_first_row_seconds" and the behaviour (drop on cluster or not). And then we don't need to have the special handling here.
There was a problem hiding this comment.
Ah that's a good point. We can either do it automatically on registration (i.e. implicitly do this when they register metrics into register_into) or just have a compile time error. Will iterate on this.
Before we'd have to also specify the cluster on removal, which could risk unsynchronization from registration. Now we override the `register` method to ensure every metric getting registered also gets deleted on cluster remove.
3 participants
For metrics like mz_time_to_first_row, we were tracking histograms keyed by clusters. However these would accumulate even when the cluster is gone. Thus we reclaim/cleanup the series on cluster drop, similar to other mz_compute_peek_duration_seconds.
The way we do this is when we drop the cluster, using the catalog implications framework we clean up the series keyed by that cluster.
Motivation
Helps reduce
/metricpayload to solve Incident 1252Verification
I've created unit tests on the metrics functions and a server integration test. Also manually tested via starting 10 clusters, issuing peeks, observing the metrics, then dropping the clusters and observing the samples are gone.
For the customer who this is an issue for, ran the following query to figure out cluster churn:
and got 1500 clusters created/dropped for a week. Their steady state is about 300. Given
mz_time_to_first_rowwas our most expensive metric, and we have about 17 buckets for each label combination, we'd be substantially decreasing how much each scrape pulls.