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

instrumenting/clientlibs: add process_threads #1964

Merged
merged 1 commit into from Aug 6, 2021

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Jun 2, 2021

This reserves a new well-known process_threads gauge for client
libraries, in order to expose OS threads count on all instrumented
processes.
As this metrics is language/runtime independent, it makes sense to
be under the process_ namespace so that libraries can align on
the name (if/when they start exposing this).

For reference, this is coming from an enhancement request on the Rust library: tikv/rust-prometheus#401.

@lucab lucab force-pushed the ups/clientlibs-process-threads branch 2 times, most recently from 9921974 to 6f073af Compare June 2, 2021 19:34
@roidelapluie
Copy link
Member

Thanks for the sugestion.

I do not think that go or java exposes that metric, can you be more specific?

Also, is the threading model the same accross all OS? Would that metric mean the same for different OS?

@lucab
Copy link
Contributor Author

lucab commented Jun 3, 2021

I do not think that go or java exposes that metric, can you be more specific?

@roidelapluie that's documented in the linked "For more context and existing languages/libraries references", but let me duplicate that here explicitly:

"Number of OS threads" is not really runtime/language specific, but individual libraries do not freely own the process_ namespace, so the same metrics may end up named in different ways and with a $language_ prefix.
(Although runtimes can keep more specific metrics if they want, in order to specifically track only OS threads created by the runtime but not by other things in the same process; this may apply to Go/JVM, but is not relevant for Rust).

Also, is the threading model the same across all OS? Would that metric mean the same for different OS?

The threading model could be different, specifically on what is the exact definition of an "OS thread" (if that primitive exists in the OS).
This metrics does not require a uniform definition and only relies on the facts that 1) there exist OS threads, 2) they can be counted.
This is no different than other process_ metrics; where it cannot be applied, the docs are already suggesting that libraries "SHOULD prefer leaving out the corresponding metric".

@roidelapluie
Copy link
Member

roidelapluie commented Jun 3, 2021

jvm_threads_current is threads, it is not OS threads, so the meaning is different. On certain OS, this can map to OS threads but AFAIK this is not guaranteed.

@lucab
Copy link
Contributor Author

lucab commented Jun 3, 2021

@roidelapluie fair point, thanks for the feedback. I wasn't sure about each runtime details (Go one either), but I did mention above that keeping other runtime-specific metrics is fine and orthogonal. Specifically, green-threads or only-threads-owned-by-the-runtime.

This still leaves us with:

  1. processes can have multiple OS threads (on the vast majority of currently used OSes)
  2. client libraries would benefit from a uniform way of gauging this

Other than removing the "other libraries" mention in my commit, in which direction should I push this PR to cover the usecase above?

@free
Copy link
Contributor

free commented Jun 3, 2021

One argument in favor of adding process_threads to the list of standard process metrics, is that on Linux it is one of the fields provided by /proc/<pid>/stat (search for num_threads), which is the most likely source for all other process metrics.

Also, the document already says "client libraries SHOULD prefer leaving out the corresponding metric over exporting bogus, inaccurate, or special values", so it's not like this addition will break any client library's compatibility or anything. E.g. if Java uses a different mechanism to retrieve thread count (and Java threads are not necessarily the same as OS threads), then it can simply leave this out (which I suspect it already does with all process metrics).

@free
Copy link
Contributor

free commented Jun 3, 2021

Also, is the threading model the same accross all OS? Would that metric mean the same for different OS?

I believe a lot of things are not standard across OSs. E.g. I believe Fuchsia does not have file descriptors. And for all I know there may be OSs without a concept or resident vs. virtual memory.

But overall, I believe all widely used OSs have the concept of threads. And again, the documentation says "leave it out if it doesn't make sense", so there doesn't seem to be a requirement that all process metrics make sense on every single OS. I mean some of the things Prometheus monitors aren't even processes.

@lucab lucab force-pushed the ups/clientlibs-process-threads branch from 6f073af to edc75c5 Compare August 5, 2021 13:28
@lucab
Copy link
Contributor Author

lucab commented Aug 5, 2021

Gentle ping. In the meanwhile I've rebased this and updated the commit message to avoid mislead references to other libraries/runtimes, PTAL.

This reserves a new well-known `process_threads` gauge for client
libraries, in order to expose OS threads count on all instrumented
processes.
As this metrics is language/runtime independent, it makes sense to
be under the `process_` namespace so that libraries can align on
the name (if/when they start exposing this).

Signed-off-by: Luca BRUNO <luca.bruno@coreos.com>
@lucab lucab force-pushed the ups/clientlibs-process-threads branch from e7d2e79 to 72b9ad3 Compare August 5, 2021 15:10
@RichiH
Copy link
Member

RichiH commented Aug 5, 2021

I am basically fine adding it, though I wonder if process_os_threads wouldn't make more sense. This also opens up e.g. process_vm_threads for JVM.

I might also be overthinking this.

@roidelapluie
Copy link
Member

Those would be jvm_metrics, not process_ metrics. I think process_ makes it more or less clear that it comes from /proc and not language dependant.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@SuperQ SuperQ merged commit 4bf6de4 into prometheus:master Aug 6, 2021
@lucab
Copy link
Contributor Author

lucab commented Aug 6, 2021

Thanks all!

@lucab lucab deleted the ups/clientlibs-process-threads branch August 6, 2021 09:12
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

5 participants