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

Add system.cpu.count metric #2384

Closed
wants to merge 2 commits into from

Conversation

trask
Copy link
Member

@trask trask commented Feb 25, 2022

I wasn't sure if this should be a resource attribute or metric, but I tested that cpu count can change at runtime, e.g. docker update --cpuset-cpus, and nproc and Java Runtime.availableProcessors() pick up the change at runtime, so seems like it should be a metric.

Changes

Adds a new metric system.cpu.count to capture the number of CPUs available.

@trask trask marked this pull request as ready for review February 25, 2022 23:07
@trask trask requested review from a team as code owners February 25, 2022 23:07
@tigrannajaryan
Copy link
Member

I wasn't sure if this should be a resource attribute or metric, but I tested that cpu count can change at runtime, e.g. docker update --cpuset-cpus, and nproc and Java Runtime.availableProcessors() pick up the change at runtime, so seems like it should be a metric.

  • I think there was recent discussion that said the set of attributes can't change at the runtime (I can't find the link).
  • CPU number is a dimension of system.cpu.* metrics.

Does this mean system.cpu.* metrics are broken since one of the attributes has a varying number of possible values over time?

@bogdandrutu
Copy link
Member

Per the definition of the javadoc, https://www.tutorialspoint.com/java/lang/runtime_availableprocessors.htm the availableProcessors does not represent the number of CPU on the host/container.

@bogdandrutu
Copy link
Member

I believe this should be a "process.cpu.available" or something like that metric. Since this is just the number of cores available to that specific instance of the JVM. May even be a jvm specific metric.

@@ -35,6 +35,7 @@ instruments not explicitly defined in the specification.
| | | | | | cpu | CPU number [0..n-1] |
| system.cpu.utilization | | 1 | Asynchronous Gauge | Double | state | idle, user, system, interrupt, etc. |
| | | | | | cpu | CPU number (0..n) |
| system.cpu.count | | 1 | Asynchronous UpDownCounter | Int64 | | |
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a Gauge, since it does not sum "anything".

Copy link
Member Author

@trask trask Mar 1, 2022

Choose a reason for hiding this comment

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

it counts sums cores(?)

@bogdandrutu
Copy link
Member

@trask here is what I found https://www.oracle.com/java/technologies/javase/8u191-relnotes.html#JDK-8146115 which means that the value is ... definitely not the number of total CPUs in the environment. Or it can or cannot be to be more exact :)

@trask
Copy link
Member Author

trask commented Mar 1, 2022

I believe this should be a "process.cpu.available" or something like that metric. Since this is just the number of cores available to that specific instance of the JVM.

this is a great point

I did a test:

taskset --cpu 0-1 java ...

and Runtime.availableProcessors() reported just the two cpus, not the full system cpu count.

I'll open a new PR for process.cpu.available.

I don't have any specific need for this PR anymore, but happy to move it forward if there's interest.

@tigrannajaryan
Copy link
Member

I don't have any specific need for this PR anymore, but happy to move it forward if there's interest.

I am in favour of closing unless there is good evidence that the CPU count at the system level can actually change at runtime and there is a use case that needs this as a metric.

@trask
Copy link
Member Author

trask commented Mar 2, 2022

No objection to closing

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

4 participants