refactor(metrics): use LabelGuarded for vec metrics in StreamingMetrics #16737
Conversation
…hao/guarded-metrics-stream
|
Should I merge this first and let @stdrc do the rest? |
Can you wait for me? Will do it on the next work day. |
sure! Thanks |
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
|
I've refactored the Agg and GroupTopN part, also resolved some comment related to Source. PTAL, thanks! |
Signed-off-by: Richard Chien <stdrc@outlook.com>
| @@ -180,18 +186,14 @@ impl<S: StateStore> ColumnDeduplicater<S> { | |||
| } | |||
|
|
|||
| /// Flush the deduplication table. | |||
| fn flush(&mut self, dedup_table: &StateTable<S>, ctx: ActorContextRef) { | |||
| fn flush(&mut self, _dedup_table: &StateTable<S>) { | |||
There was a problem hiding this comment.
It seems that we won't flush the dedup_table anymore here. May remove the parameter and also update the method comment and the method name to avoid confusion.
| ) -> StreamResult<Self> { | ||
| let metrics_info = MetricsInfo::new( | ||
| ctx.streaming_metrics.clone(), | ||
| state_table.table_id(), | ||
| ctx.id, | ||
| "GroupTopN", | ||
| "AppendOnlyGroupTopN", |
There was a problem hiding this comment.
Previously it was GroupTopN. Was it a bug?
There was a problem hiding this comment.
Perhaps not, here is inside InnerAppendOnlyGroupTopNExecutor
| .map(AsRef::as_ref) | ||
| .collect::<Vec<&str>>(), | ||
| ) | ||
| .with_guarded_label_values(&self.get_metric_labels().each_ref().map(AsRef::as_ref)) |
There was a problem hiding this comment.
A remaining temporary with_guarded_label_values. Is this expected?
| ) -> StreamResult<Self> { | ||
| let metrics_info = MetricsInfo::new( | ||
| ctx.streaming_metrics.clone(), | ||
| state_table.table_id(), | ||
| ctx.id, | ||
| "GroupTopN", | ||
| "AppendOnlyGroupTopN", |
There was a problem hiding this comment.
Perhaps not, here is inside InnerAppendOnlyGroupTopNExecutor
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
close #16728
related #12805
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.