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

Draft: Add CPU in MHz to stats and emit nomad.client.allocs.cpu.total_mhz metric #11794

Conversation

conorevans
Copy link
Contributor

Signed-off-by: Conor Evans coevans@tcd.ie

Resolves #11142

This is a draft PR as I would like to get feedback if I've gone awry. I wasn't sure how the driver.pb.go file gets updated (I see a reference in the makefile but wasn't sure if that gets run as part of PR). I couldn't see any instructions as to regenerating it locally.

@DerekStrickland
Copy link
Contributor

Hi @conorevans

Thanks for this and the other contributions! We'll get them in the pipeline for review!

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @conorevans! The overall approach here looks ok.

If you have buf installed (as per make depsor justgo install github.com/bufbuild/buf/cmd/buf@v0.36.0), you can run make proto` and that should regenerate the protobuf file. Once that's done we can review the implementation in detail.

I'm a little concerned about how showing MHz works in practice given CPU frequency scaling... are we going to be giving users completely misleading metrics? But maybe we can assume that if you're using MHz for your metrics that you also know what's going on with CPU frequency scaling? Not sure.

@tgross tgross self-assigned this Mar 8, 2022
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@TrueBrain
Copy link
Contributor

TrueBrain commented Jun 20, 2023

While I was fiddling a bit how to get the MHz as a counter, I noticed this PR. Maybe a silly question, and I might misunderstand the code: but isn't total_ticks already in MHz? TotalTicksAvailable returns cpuTotalTicks which is calculated as cpuTotalTicks = math.Floor(float64(cpuNumCores) * cpuMhzPerCore), which is the same CPUMHzTotal returns (minus a Floor)

Mainly I think ticks is a bit ambiguous, as one could think they are jiffies; but they are not.

Again, not that into the Nomad code, so might misunderstand things, but just wanted to leave this here :)

(I mainly ask, as the test-case shows something different, and I couldn't figure out why :P So that is why I am guessing I might be misunderstanding something here)

@tgross
Copy link
Member

tgross commented Jun 20, 2023

Yeah I think you're right there @TrueBrain. Let's get your PR #17579 in to get this done.

@tgross tgross closed this Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Provide CPU metrics in MHz
5 participants