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

Export thread count from process_collector #401

Merged
merged 1 commit into from Aug 9, 2021

Conversation

free
Copy link
Contributor

@free free commented May 17, 2021

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 free force-pushed the process_collector-export-process_threads branch 2 times, most recently from b2d6349 to ce11f25 Compare May 17, 2021 09:57
@free
Copy link
Contributor Author

free commented May 31, 2021

Gentle nudge.

@lucab
Copy link
Member

lucab commented Jun 1, 2021

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
Copy link
Contributor Author

free commented Jun 2, 2021

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
Copy link
Member

lucab commented Jun 2, 2021

I'm not aware of any such discussion, so I started it at prometheus/docs#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.

@@ -100,6 +101,12 @@ impl ProcessCollector {
.unwrap();
descs.extend(start_time.desc().into_iter().cloned());

let threads = Gauge::with_opts(
Opts::new("process_threads", "Number of OS threads.").namespace(namespace.clone()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number of OS threads in the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done,

@lucab
Copy link
Member

lucab commented Aug 6, 2021

@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 free force-pushed the process_collector-export-process_threads branch 2 times, most recently from 268c6f7 to 3f21f15 Compare August 6, 2021 21:15
This simply retrieves and exports the value of the  field from the
already populated  that CPU, memory and start time are
sourced.

Signed-off-by: Alin Sinpalean <alin.sinpalean@gmail.com>
@free free force-pushed the process_collector-export-process_threads branch from 3f21f15 to e1905a6 Compare August 6, 2021 21:16
@free
Copy link
Contributor Author

free commented Aug 6, 2021

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.

@lucab lucab merged commit fab7e76 into tikv:master Aug 9, 2021
@lucab lucab mentioned this pull request Sep 27, 2021
JanBerktold pushed a commit to JanBerktold/rust-prometheus that referenced this pull request Nov 12, 2022
This simply retrieves and exports the value of the  field from the
already populated  that CPU, memory and start time are
sourced.

Signed-off-by: Alin Sinpalean <alin.sinpalean@gmail.com>
Signed-off-by: Jan Berktold <jberktold@roblox.com>
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

Successfully merging this pull request may close these issues.

None yet

2 participants