search

tikv / rust-prometheus

Prometheus instrumentation library for Rust applications
Apache License 2.0
1.07k stars 182 forks source link

Export thread count from `process_collector` #401

Closed free closed 3 years ago

free commented 3 years ago

This simply retrieves and exports the value of the num_threads field from the already populated procfs::process::Stat that CPU, memory and start time are sourced.

free commented 3 years ago

Gentle nudge.

lucab commented 3 years ago

Thanks for the PR. Please note however that the ProcessCollector implements https://prometheus.io/docs/instrumenting/writing_clientlibs/#process-metrics. I'm thus wary of adding more arbitrary stuff directly here.

I'd be happy to see the "Number of OS threads" metrics standardized upstream, and once that is published we can promptly merge this.

free commented 3 years ago

I'd be happy to see the "Number of OS threads" metrics standardized upstream, and once that is published we can promptly merge this.

Is anything like that being worked on (upstream, I mean)? On the one hand the Golang library has go_threads and Java has jvm_threads_current (and I'm sure other language libraries have their own).

On the other hand, the only other way of getting this metric (other than everyone rolling their own implementation, which is what we ended up doing) is to enable the (by default disabled) processes collector of node-exporter (above which there's a warning that the collector may be disabled due to "prolonged runtime" or "significant resource usage"). While adding the metric here boils down to simply exporting a value that has already been retrieved from the OS.

I'm not going to insist that this be merged, but the fact that process_threads is not a standard process metric is not a good reason against exporting it regardless.

lucab commented 3 years ago

I'm not aware of any such discussion, so I started it at https://github.com/prometheus/docs/pull/1964. As you mentioned, it is a common metrics across libraries so it makes sense to have it uniformed. Plus, the process_ prefix is reserved and I think that having to name this rust_process_threads does not make much sense here.

lucab commented 3 years ago

@free sorry for putting this through the long path. The upstream doc change has been merged, so this can land too. It only needs a rebase and a minor rewording.

free commented 3 years ago

Phew, took a bit to get the DCO right.

sorry for putting this through the long path.

No worries, thanks for following through. I put together a polyfill locally in the meantime, so no problem.