Implementing a process collector

[gagbo]

Hello,

I recently stumbled upon tikv/rust-prometheus#392 and now I'm preparing a migration from rust-prometheus. I'm wondering how it would be possible to add a collector for process metrics just like what rust-prometheus does currently. I'm really inexperienced in those packages, and I don't really see how the ProcessCollector thing would translate here, to add some metrics related to the running process.

Can you tell me what would be necessary to add support for this ? I was thinking probably as another crate that exposes a single global function like crate::export_process_metrics(); so that crate would have to add timers to run the collection and hope that the timer runs often enough to give a precise measurement when prometheus scrapes the endpoint ?

Regards,
Gerry

[mxinden]

Hi Gerry,

As you noted above open-metrics-client does not itself support exposition of process metrics today.

I was thinking probably as another crate

That sounds good. I am open to whether that crate would live in this repository or not. The former would likely make development easier as one can make atomic changes across both crates at once (with one pull request).

Off the top of my head the interface of such crate could look like:

fn register_process_metrics(registry: &mut Registry)

The crate would register multiple custom metrics, e.g. a gauge metric exposing the number of threads. Those custom metrics would each implement EncodeMetric and collect the concrete metric values from the system on EncodeMetric::encode.

@gagbo does the above make sense? Would you be interested in contributing such a crate?

[gagbo]

If I can find some time it’d be nice yeah, hopefully I’ll be able to find time to dig into this.

I think I’d like a way to specify running the encode function in a separate thread (which would require the registry to be Sync ?), so that all the metrics related calls can be handled by a different core if need be ? (My point is that you probably don’t need to be running in the same thread to collect process info, so offloading those calls to another core and another cache might be nice to have)

[mxinden]

If I can find some time it’d be nice yeah, hopefully I’ll be able to find time to dig into this.

Great. Let me know in case you need any help!

I think I’d like a way to specify running the encode function in a separate thread

Can you expand on what you want to optimize? All metric implementations are synchronized (e.g. Counter is just an atomic integer), thus allowing metric recording and metric collection to happen in different threads.

(which would require the registry to be Sync ?)

I am guessing that you are referring to interior mutability and not the Sync marker trait? I don't see why one would need interior mutability for the Registry itself, unless one wants to register metrics after startup, which I would argue is an anti-pattern.

[gagbo]

Hello,

Sorry for the late answer, I've been struggling to find time these days :(

For the time being I don't see myself working too much on this as our monitoring coverage is decent now and we have other priorities but I'll keep this in mind when it becomes an important subject again!

Can you expand on what you want to optimize?

I thought that all encode calls had to be executed in the same thread as the one recording the events, but as you clarified it shouldn't be an issue after all.

I am guessing that you are referring to interior mutability and not the Sync marker trait?

I was really thinking about the marker trait here (so that registries can be borrowed by an encode call in another thread), but it doesn't matter as it's irrelevant now, my understanding of the Registry structure seems wrong.

[dovreshef]

Hi, all

I've tried to take a stab at implementing this, since we need this as well. I looked at the current implementation for the other prometheus crate.

As part of the implementation it reads data from procfs to figure out stats on the process. In the current implementation, it is implemented as a custom collector, so the data is read once, and then used for all metrics. But with the design of the current crate, I found it hard to emulate this, since each metrics is its own separate thing, and the logic is spread out across each encode metric impl (If I understood it correctly).

I think it would help if it was also possible to have something analogous to the Collect trait for a group of metrics that share a source, so to speak.

I also found issue #49 which I think is showing other usecases where it would be helpful.

Just my 2c.

[mxinden]

@dovreshef would you be retrieving the information from the system in time or on an interval. I think the former is the Prometheus way.

Would the custom metric example not be the interface you need? I.e. be called on scrape to retrieve and generate the metrics?

https://github.com/prometheus/client_rust/blob/master/examples/custom-metric.rs

[dovreshef]

@dovreshef would you be retrieving the information from the system in time or on an interval. I think the former is the Prometheus way.

Sorry, I'm not sure I understand the question. I'll be retrieving the info in the EncodeMetric trait encode function, as demoed in the example.

There are multiple metrics that the process collector gathers, and the process to gather each one is similar. We read the /proc file system for the calling process, and extract the data from several files there.
The issue is that they all share a few (relatively) expensive initial steps, and if I'll gather the data for each metric separately I'll be repeating the steps for each metric, which is a bit of a waste.

In the existing Prometheus client implementation all the metrics are gathered in a single collect call, and so they share those initial steps.

So I think it would help if we would have a way to collect/encode multiple metrics in a single call.

[mxinden]

The issue is that they all share a few (relatively) expensive initial steps, and if I'll gather the data for each metric separately I'll be repeating the steps for each metric, which is a bit of a waste.

Ah, sorry, I forgot having this discussion in the past.

As suggested on #49 (comment), what do you think of the option to be able to register a Collector on a Registry. A Collector would be able to return a set of metrics where each metric can have a different metric type.

For the process collector, you would implement the Collector trait and register an instance with a Registry. On encode we would iterate the Collectors registered with the Registry, call Collector::collect and encode each returned metric.

Does that make sense @dovreshef? If so, would you like to prototype this?

As an aside, we would likely want to introduce StaticCounter, StaticGauge, ... so that you don't have to pay the cost of an AtomicU64 on each Collector::collect call.

[dovreshef]

Does that make sense @dovreshef? If so, would you like to prototype this?
Sure.

So the design is:

• Have a new Collector trait that looks something like:

trait Collector<'a, M>
where
    M: EncodeMetric + 'a,
    Self::List: Iterator<Item = &'a (Descriptor, M)>
{
    type List;

    fn collect(&self) -> Self::List;
}

• Registry implements Collector, which returns RegistryIterator.
• text::encode calls the collect method on the registry to retrieve the iterator.

Now I can see two ways to continue from here:

Either:

• Registry no longer holds sub_registries: Vec<Registry<M>> but instead holds Vec<Box<dyn Collector>> .
• Registry will have a new function to add a subregistry as a Box<dyn Collector>.
• No need to add new fields.

Or:

• Have a new field on the registry that holds the Box<dyn Collector>.
• RegistryIterator will also iterate over all the collectors.

WDYT? Any other design?

[mxinden]

• Have a new Collector trait that looks something like:

trait Collector<'a, M>
where
    M: EncodeMetric + 'a,
    Self::List: Iterator<Item = &'a (Descriptor, M)>
{
    type List;

    fn collect(&self) -> Self::List;
}

👍

Small nit, maybe type Collection; would be more intuitive.

• Registry implements Collector, which returns RegistryIterator.

👍

• text::encode calls the collect method on the registry to retrieve the iterator.

Instead of taking a Registry, text::encode could now even take some C: Collector.

Either:

* Registry no longer holds `sub_registries: Vec<Registry<M>>` but instead holds `Vec<Box<dyn Collector>>` .

* Registry will have a new function to add a subregistry as a `Box<dyn Collector>`.

* No need to add new fields.

That would be very clean in my opinion. My gut feeling tells me we will be running in some trait object issues. That said, I think we should give it a try.

Thanks @dovreshef for looking into this!

[mxinden]

@dovreshef are you still interested in contributing the Collector pattern? :)