Skip to content
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

Custom Collector with sub_registry_with_prefix outputs name multiple times #155

Open
allada opened this issue Jul 17, 2023 · 3 comments
Open

Comments

@allada
Copy link

allada commented Jul 17, 2023

If you create a custom Collector and attach it to a sub registry it appends the name improperly.

#[derive(Debug)]
struct MyCollector {}

impl prometheus_client::collector::Collector for MyCollector {
  fn collect<'a>(&'a self) -> Box<dyn Iterator<Item = (std::borrow::Cow<'a, prometheus_client::registry::Descriptor>, prometheus_client::MaybeOwned<'a, Box<dyn prometheus_client::registry::LocalMetric>>)> + 'a> {
    let c: Box<dyn prometheus_client::registry::LocalMetric> = Box::new(prometheus_client::metrics::counter::ConstCounter::new(42));
    let descriptor = prometheus_client::registry::Descriptor::new(
      "my_counter",
      "This is my counter",
      None,
      None,
      vec![],
    );
    Box::new(std::iter::once((std::borrow::Cow::Owned(descriptor), prometheus_client::MaybeOwned::Owned(c))))
  }
}

let my_collector = Box::new(MyCollector{});    
{
    let mut root_registry = <Registry>::with_prefix("root");
    let r = root_registry.sub_registry_with_prefix("000");
    let r = r.sub_registry_with_prefix("111");
    let r = r.sub_registry_with_prefix("222");
    r.register_collector(my_collector);
}

Results:

# HELP root_root_000_root_000_111_root_000_111_222_my_counter This is my counter.....
# TYPE root_root_000_root_000_111_root_000_111_222_my_counter counter
root_root_000_root_000_111_root_000_111_222_my_counter_total 42
# EOF

Expected:

# HELP root_000_111_222_my_counter This is my counter.....
# TYPE root_000_111_222_my_counter counter
root_000_111_222_my_counter_total 42
# EOF
@mxinden
Copy link
Member

mxinden commented Jul 20, 2023

You are right. Thank you for the report. I have discovered the same with https://github.com/libp2p/rust-libp2p/tree/master/misc/metrics.

I have fixed this issue in #149 (not yet released). A nice side-effect is that it significantly simplifies the Collector trait.

@allada would you mind testing with latest master, i.e. with #149 included?

I am sorry for not having patched this earlier. Thanks for debugging.

(Good to see the Collector trait, while faulty, needed and used.)

@allada
Copy link
Author

allada commented Jul 20, 2023

It looks like the entire API has changed a lot and without having non-code-based documentation that I can use it's going to be difficult to test this in our system without refactoring everything.

However, you might be interested in seeing the wrappers we are using, as it might give you some ideas on API designs:

Internal metrics api:
https://github.com/allada/turbo-cache/blob/fd1de941b796287abe24dad852ed50a27d5d86af/util/prometheus_utils.rs

Example usage:
https://github.com/allada/turbo-cache/blob/fd1de941b796287abe24dad852ed50a27d5d86af/cas/worker/running_actions_manager.rs#L1138
https://github.com/allada/turbo-cache/blob/fd1de941b796287abe24dad852ed50a27d5d86af/cas/worker/local_worker.rs#L484
https://github.com/allada/turbo-cache/blob/fd1de941b796287abe24dad852ed50a27d5d86af/util/evicting_map.rs#L375

There was also a short discussion that we had here:
TraceMachina/nativelink#209

You might be interested in the idea of a procedural macro.

@mxinden
Copy link
Member

mxinden commented Jul 31, 2023

It looks like the entire API has changed a lot and without having non-code-based documentation that I can use it's going to be difficult to test this in our system without refactoring everything.

I am sorry for the trouble this causes on your end. I am not sure I am fully following. What is non-code-based documentation? How can I make your transition to the new Collector trait easier @allada?

You might be interested in the idea of a procedural macro.

I am not sure how a procedural macro could simplify the Collector trait. Do you have concrete suggestions in mind? In general, I try to use macros as a last resort only, especially when exposed on the public API surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants