Skip to content

Commit

Permalink
Fix duplicate metric registration for filters (#397)
Browse files Browse the repository at this point in the history
Some filter currently call `register` (rather than
`register_if_not_exists`) which throws an error if the metric
has already been registered - this meant that we couldn't use
multiple instances of those filter in the filter chain
  • Loading branch information
iffyio authored Sep 21, 2021
1 parent ae0c424 commit d4ced10
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 20 deletions.
2 changes: 1 addition & 1 deletion src/filters/capture_bytes/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl Metrics {
"CaptureBytes",
"Total number of packets dropped due capture size being larger than the received packet",
))?
.register(registry)?,
.register_if_not_exists(registry)?,
})
}
}
6 changes: 3 additions & 3 deletions src/filters/compress/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,21 @@ impl Metrics {
),
&operation_labels,
)?
.register(registry)?;
.register_if_not_exists(registry)?;

let decompressed_bytes_total = IntCounter::with_opts(filter_opts(
"decompressed_bytes_total",
"Compress",
"Total number of decompressed bytes either received or sent.",
))?
.register(registry)?;
.register_if_not_exists(registry)?;

let compressed_bytes_total = IntCounter::with_opts(filter_opts(
"compressed_bytes_total",
"Compress",
"Total number of compressed bytes either received or sent.",
))?
.register(registry)?;
.register_if_not_exists(registry)?;

Ok(Metrics {
packets_dropped_compress: dropped_metric
Expand Down
2 changes: 1 addition & 1 deletion src/filters/local_rate_limit/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl Metrics {
"LocalRateLimit",
"Total number of packets dropped due to rate limiting",
))?
.register(registry)?,
.register_if_not_exists(registry)?,
})
}
}
2 changes: 1 addition & 1 deletion src/filters/token_router/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl Metrics {
),
&label_names,
)?
.register(registry)?;
.register_if_not_exists(registry)?;

Ok(Metrics {
packets_dropped_no_token_found: metric
Expand Down
25 changes: 11 additions & 14 deletions src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,25 +43,22 @@ pub fn filter_opts(name: &str, filter_name: &str, description: &str) -> Opts {
opts(name, &format!("filter_{}", filter_name), description)
}

pub trait CollectorExt: Collector + Clone + Sized + 'static {
/// Registers the current metric collector with the provided registry.
/// Returns an error if a collector with the same name has already
/// been registered.
fn register(self, registry: &Registry) -> Result<Self> {
registry.register(Box::new(self.clone()))?;
Ok(self)
}
/// Registers the current metric collector with the provided registry.
/// Returns an error if a collector with the same name has already
/// been registered.
fn register_metric<T: Collector + Sized + 'static>(
registry: &Registry,
collector: T,
) -> Result<()> {
registry.register(Box::new(collector))
}

pub trait CollectorExt: Collector + Clone + Sized + 'static {
/// Registers the current metric collector with the provided registry
/// if not already registered.
fn register_if_not_exists(self, registry: &Registry) -> Result<Self> {
match self.clone().register(registry) {
match register_metric(registry, self.clone()) {
Ok(_) | Err(prometheus::Error::AlreadyReg) => Ok(self),
Err(prometheus::Error::Msg(msg)) if msg.contains("already exists") => {
// FIXME: We should be able to remove this branch entirely if `AlreadyReg` gets fixed.
// https://github.com/tikv/rust-prometheus/issues/247
Ok(self)
}
Err(err) => Err(err),
}
}
Expand Down

0 comments on commit d4ced10

Please sign in to comment.