Per actor metrics: should be cleaned when the actor is dropped or moved.

[fuyufjh]

Porblem

The leaked actor memory not only consumes extra memory but also affect the metrics. As the screenshot shows, the Actor 24 has already been dropped, but the metrics still exist.

Cause

The is caused by the design of MetricVec (actually a hashmap of labels -> single metrics) in the Prometheus client library.

For example,

pub agg_cached_keys: GenericGaugeVec<AtomicI64>,

which is actually backed by MetricVec

/// The underlying implementation for [`GaugeVec`] and [`IntGaugeVec`].
pub type GenericGaugeVec<P> = MetricVec<GaugeVecBuilder<P>>;

When call it with with_label_values, a new key (label) will be created in that hashmap e.g.

this.metrics
    .agg_cached_keys
    .with_label_values(&[&table_id_str, &actor_id_str])
    .set(vars.agg_group_cache.len() as i64);

But it's never been removed.

Solution

Similar to LockGuard, one solution I can tell is to wrap the MetricVec varaibles e.g. agg_cached_keys within a handler object, and remove itselves' key (label) from MetricVec's hashmap when being dropped.

[fuyufjh]

Quite annoying when investigating problems... 🥲 Hope to be fixed

[MrCroxx]

IMO, the streaming actor metrics imitate how the batch task metrics are cleaned (with customized Collector). Related issues: #3832 #4844 #5742. What are your opinions? @fuyufjh @ZENOTME

[MrCroxx]

And currently StreamingMetrics contains metrics of task level, actor level, executor level, etc, which can be split into multiple metrics for better lifetime management.

[fuyufjh]

IMO, the streaming actor metrics imitate how the batch task metrics are cleaned (with customized Collector). Related issues: #3832 #4844 #5742. What are your opinions? @fuyufjh @ZENOTME

Sounds good to me. Similar to the solution that I imagined before (described in the PR's desciption) i.e. using something to hold the lifetime of these actor-level metrics

[ZENOTME]

IMO, the streaming actor metrics imitate how the batch task metrics are cleaned (with customized Collector). Related issues: #3832 #4844 #5742. What are your opinions? @fuyufjh @ZENOTME

+1. I think it's the same problem in batch and it solved in #9378.

[MrCroxx]

One thing that makes streaming metrics harder to clean is that the labels of the streaming metrics are not the same. 🤣 Let me try to register/collect them with as less modifications as possible.

[fuyufjh]

@MrCroxx Any further updates?

[MrCroxx]

Worked on the new file cache engine before. Let me get back to this PR these days.

[lmatz]

The example above is an aggregation.
Als have a case for join.

[fuyufjh]

@MrCroxx any updates? 👀