You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Zheaoli (via GitHub)" <gi...@apache.org> on 2023/10/18 06:20:33 UTC
[PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Zheaoli opened a new pull request, #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335
For now, we already have basic metric output in the Prometheus layer.
1. requests_total
2. requests_duration_seconds
3. bytes_total
All the metrics are grouped by scheme and operation for now.
For now, in some circumstances, we need to group by the metric by an extra path. Here's an example: if we choose to write to different devices in Linux using OpenDAL, and one of the devices has some problem, the write and read action becomes slow and slow. We can't monitor and alert for the specific device if we don't have a path label.
But on the other hand, if we output all the paths as a label in metric, the final metric we get will be extremely massive. So I chose to introduce a new config option for the people to choose how many path levels they want to export
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1363316860
##########
core/src/layers/prometheus.rs:
##########
@@ -134,6 +136,15 @@ impl PrometheusLayer {
}
self
}
+
+ /// set path label level
+ /// 0: no path label, the path label will be the ""
+ /// >0: the path label will be the path split by "/" and get the last n level, like "/abc/def/ghi", if n=1, the path label will be "/abc"
+ /// -1: the path label will be full path
+ pub fn path_label_level(mut self, level: i32) -> Self {
+ self.path_label_level = level;
Review Comment:
How about adding API like `enable_path_label(mut self, level: usize)` while:
- `0` means we don't record `path`, which is the default value.
- `level` means the level we will record.
We should not provide full path support (users can pass `usize::MAX` if they want).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1367233422
##########
core/src/layers/prometheus.rs:
##########
@@ -604,26 +663,39 @@ pub struct PrometheusMetricWrapper<R> {
op: Operation,
stats: Arc<PrometheusMetrics>,
scheme: String,
+ path: String,
}
impl<R> PrometheusMetricWrapper<R> {
- fn new(inner: R, op: Operation, stats: Arc<PrometheusMetrics>, scheme: &String) -> Self {
+ fn new(
+ inner: R,
+ op: Operation,
+ stats: Arc<PrometheusMetrics>,
+ scheme: &String,
Review Comment:
Scheme is an enum that cheap to clone, no extra allocation needed. So it's better to store Scheme everywhere snd convert to `&'static str` while needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1367208629
##########
core/src/layers/prometheus.rs:
##########
@@ -604,26 +663,39 @@ pub struct PrometheusMetricWrapper<R> {
op: Operation,
stats: Arc<PrometheusMetrics>,
scheme: String,
+ path: String,
}
impl<R> PrometheusMetricWrapper<R> {
- fn new(inner: R, op: Operation, stats: Arc<PrometheusMetrics>, scheme: &String) -> Self {
+ fn new(
+ inner: R,
+ op: Operation,
+ stats: Arc<PrometheusMetrics>,
+ scheme: &String,
Review Comment:
I think scheme: &String here would be a better choice
1. keep same style with PrometheusAccessor
2. we only need scheme to be a string in this layer, this means we always need a value to store scheme as string to avoid `scheme.to_string()` every time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1367177904
##########
core/src/layers/prometheus.rs:
##########
@@ -645,11 +717,16 @@ impl<R: oio::Read> oio::Read for PrometheusMetricWrapper<R> {
}
fn poll_next(&mut self, cx: &mut Context<'_>) -> Poll<Option<Result<Bytes>>> {
+ let labels = self.stats.generate_metric_label(
Review Comment:
This is not a blocker. We can improve this part later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1366647894
##########
core/src/layers/prometheus.rs:
##########
@@ -211,6 +230,27 @@ impl PrometheusMetrics {
kind.into_static()
);
}
+
+ /// generate metric label
+ pub fn generate_metric_label<'a>(
+ &self,
+ scheme: &'a str,
+ operation: &'a str,
+ path_label: &'a str,
+ ) -> Vec<&'a str> {
+ match self.path_label_level {
Review Comment:
Maybe use `if self.path_label_level == 0 { xx } else { yy }` is more clear?
##########
core/src/layers/prometheus.rs:
##########
@@ -211,6 +230,27 @@ impl PrometheusMetrics {
kind.into_static()
);
}
+
+ /// generate metric label
+ pub fn generate_metric_label<'a>(
Review Comment:
Seems we don't need to make this function `pub`?
##########
core/src/layers/prometheus.rs:
##########
@@ -760,3 +857,32 @@ impl<R: oio::BlockingWrite> oio::BlockingWrite for PrometheusMetricWrapper<R> {
})
}
}
+
+fn get_path_label(path: &str, path_level: usize) -> &str {
+ match path_level {
Review Comment:
How about using `if path_level == 0 { xx } else { yy }`?
##########
core/src/layers/prometheus.rs:
##########
@@ -645,11 +717,16 @@ impl<R: oio::Read> oio::Read for PrometheusMetricWrapper<R> {
}
fn poll_next(&mut self, cx: &mut Context<'_>) -> Poll<Option<Result<Bytes>>> {
+ let labels = self.stats.generate_metric_label(
Review Comment:
Maybe we can store labels in `PrometheusMetricWrapper` so that we don't need to build them every time calling `poll_read`.
##########
core/src/layers/prometheus.rs:
##########
@@ -604,26 +663,39 @@ pub struct PrometheusMetricWrapper<R> {
op: Operation,
stats: Arc<PrometheusMetrics>,
scheme: String,
+ path: String,
}
impl<R> PrometheusMetricWrapper<R> {
- fn new(inner: R, op: Operation, stats: Arc<PrometheusMetrics>, scheme: &String) -> Self {
+ fn new(
+ inner: R,
+ op: Operation,
+ stats: Arc<PrometheusMetrics>,
+ scheme: &String,
Review Comment:
Use `Scheme` whenever possible.
##########
core/src/layers/prometheus.rs:
##########
@@ -604,26 +663,39 @@ pub struct PrometheusMetricWrapper<R> {
op: Operation,
stats: Arc<PrometheusMetrics>,
scheme: String,
+ path: String,
}
impl<R> PrometheusMetricWrapper<R> {
- fn new(inner: R, op: Operation, stats: Arc<PrometheusMetrics>, scheme: &String) -> Self {
+ fn new(
+ inner: R,
+ op: Operation,
+ stats: Arc<PrometheusMetrics>,
+ scheme: &String,
+ path: &String,
+ ) -> Self {
Self {
inner,
op,
stats,
scheme: scheme.to_string(),
+ path: path.to_string(),
}
}
}
impl<R: oio::Read> oio::Read for PrometheusMetricWrapper<R> {
fn poll_read(&mut self, cx: &mut Context<'_>, buf: &mut [u8]) -> Poll<Result<usize>> {
+ let labels = self.stats.generate_metric_label(
+ &self.scheme,
+ Operation::Read.into_static(),
+ self.path.as_str(),
Review Comment:
Use `&self.path` instead of `self.path.as_str()` whenever possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1363320386
##########
core/src/layers/prometheus.rs:
##########
@@ -760,3 +874,51 @@ impl<R: oio::BlockingWrite> oio::BlockingWrite for PrometheusMetricWrapper<R> {
})
}
}
+
+fn get_path_label(path: &str, path_level: i32) -> String {
+ match path_level {
+ 0 => "".to_string(),
+ -1 => path.to_string(),
+ n if n > 0 => {
Review Comment:
We can implement those logic like:
```rust
let idx: Option<usize> = s.char_indices()
.filter(|&(_, c)| c == '/')
.nth(n)
.map(|(i, _)| i);
```
And can we return `&str` here to avoid an additional allocation?
##########
core/src/layers/prometheus.rs:
##########
@@ -134,6 +136,15 @@ impl PrometheusLayer {
}
self
}
+
+ /// set path label level
+ /// 0: no path label, the path label will be the ""
+ /// >0: the path label will be the path split by "/" and get the last n level, like "/abc/def/ghi", if n=1, the path label will be "/abc"
+ /// -1: the path label will be full path
+ pub fn path_label_level(mut self, level: i32) -> Self {
+ self.path_label_level = level;
Review Comment:
How about adding API like `enable_path_label(mut self, level: usize)` while:
- `0` means we don't record `path`, which is the default value.
- `lvl` means the level we will record.
We should not provide full path support (users can pass `usize::MAX` if they want).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1363321416
##########
core/src/layers/prometheus.rs:
##########
@@ -171,11 +185,12 @@ impl PrometheusMetrics {
registry: Registry,
requests_duration_seconds_buckets: Vec<f64>,
bytes_total_buckets: Vec<f64>,
+ path_label_level: i32,
) -> Self {
let requests_total = register_int_counter_vec_with_registry!(
"requests_total",
"Total times of create be called",
- &["scheme", "operation"],
+ &["scheme", "operation", "path"],
Review Comment:
We don't not include `path` if users not enable it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1367180710
##########
core/src/layers/prometheus.rs:
##########
@@ -645,11 +717,16 @@ impl<R: oio::Read> oio::Read for PrometheusMetricWrapper<R> {
}
fn poll_next(&mut self, cx: &mut Context<'_>) -> Poll<Option<Result<Bytes>>> {
+ let labels = self.stats.generate_metric_label(
Review Comment:
Nice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335#discussion_r1366870732
##########
core/src/layers/prometheus.rs:
##########
@@ -645,11 +717,16 @@ impl<R: oio::Read> oio::Read for PrometheusMetricWrapper<R> {
}
fn poll_next(&mut self, cx: &mut Context<'_>) -> Poll<Option<Result<Bytes>>> {
+ let labels = self.stats.generate_metric_label(
Review Comment:
I'm not sure if there is some best practice in Rust to store the `Vec<&str>` in struct
If we store the Vec<String> instead, there will be two conversion when use it(convert vec<&str> to Vec<String> when input, and convert back when output), I not sure it would be faster than build them every time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(layer/prometheus): Support output path as a metric label [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3335:
URL: https://github.com/apache/incubator-opendal/pull/3335
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org