-
Notifications
You must be signed in to change notification settings - Fork 591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: sink metrics #18730
refactor: sink metrics #18730
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM!
@@ -218,12 +219,33 @@ impl<F: LogStoreFactory> SinkExecutor<F> { | |||
if self.sink.is_sink_into_table() { | |||
processed_input.boxed() | |||
} else { | |||
let labels = [ | |||
&actor_id.to_string(), | |||
"NA", // TODO: remove the connector label for log writer metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to erase the connector label?
We can get the connector label easily from self.sink
, which can be supported by implementing the following method for Sinkimpl
impl SinkImpl {
pub fn sink_name(&self) -> &'static str {
fn get_sink_name<S: Sink>(_sink: &S) -> &'static str {
S::SINK_NAME
}
dispatch_sink!(self, sink, get_sink_name(&**sink))
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do. I was thinking that, for LogWriterMetrics, it's supposed to be unaware of the log reader i.e. the actual Sink implementation.
Also, the sink_name
is here, which I think is enough for users.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Background
In other places (such as source), the usage of metrics is generally as follows:
SourceMetrics
, there is a pair ofMetricsVec
instead of actual Metrics.Metrics
fromMetricsVec
with particular labels.While, on the sink side,
SinkMetrics
directly consist of a collection of Metrics.What's in this PR
SinkMetrics
from a set of Metrics to a set of MetricVecGLOBAL_SINK_METRICS
SinkWriterMetrics
for general sink metricsLogWriterMetrics
LogReaderMetrics
iceberg/mod.rs
for detailsThis will also resolve the issue discussed at #17812 (comment), because it follows the standard pattern of #14838.
Follow-ups
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.