-
Notifications
You must be signed in to change notification settings - Fork 344
Add parser for sysfs devices/system/cpu/... #94
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
Conversation
a685b80
to
dac3ead
Compare
52671f0
to
bebb299
Compare
Ping @mdlayher :-) |
Ack, sorry, out of town. Will try to get to this on Monday. |
More poke @mdlayher 🍏 |
sysfs/system_cpu.go
Outdated
|
||
systemCpufreq := SystemCpufreq{} | ||
for _, cpu := range cpus { | ||
_, cpuName := filepath.Split(cpu) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not use filepath.Base() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no reason, just overlooked.
50617e9
to
7c1ae43
Compare
Hrm, tests are failing due to something else:
|
@grobie Poke? Please? 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golint output (not all relevant to this PR):
✔ ~/src/github.com/prometheus/procfs [superq/cpu|✔]
12:12 $ golint ./...
bcache/bcache.go:32:6: type name will be used as bcache.BcacheStats by other packages, and that stutters; consider calling this Stats
internal/util/parse.go:54:1: exported function ReadUintFromFile should have comment or be unexported
internal/util/parse.go:66:1: comment on exported function SysReadFile should be of the form "SysReadFile ..."
sysfs/net_class_test.go:47:28: should drop = 0 from declaration of var netDevGroup; it is the zero value
sysfs/system_cpu.go:26:6: type SystemCpuCpufreq should be SystemCPUCpufreq
sysfs/system_cpu.go
Outdated
} | ||
|
||
// SystemCpufreq is a collection of SystemCpuCpufreq for every CPU. | ||
type SystemCpufreq map[string]SystemCpuCpufreq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are no methods on this type, I'd remove the type.
sysfs/system_cpu.go
Outdated
) | ||
|
||
// SystemCpuCpufreq contains stats from devices/system/cpu/cpu[0-9]*/cpufreq/... | ||
type SystemCpuCpufreq struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*CPU throughout, as golint will mention too.
sysfs/system_cpu.go
Outdated
} | ||
|
||
// NewSystemCpufreq returns CPU frequency metrics for all CPUs. | ||
func (fs FS) NewSystemCpufreq() (SystemCpufreq, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I think my preference would be to return a slice with the CPU's name as a member of the structure. Maps are usually a little gross in APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the pattern in net_class.go, which returns a type NetClass map[string]NetClassIface
.
But, it also includes a Name
in the NetClassIface
, so that it can be identified stand-alone. I can do the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal preference would be a slice with field Name in both cases. Maps can be clunky to use and the undefined map iteration order behavior bites a lot of people on a regular basis.
sysfs/system_cpu.go
Outdated
if _, err := os.Stat(cpuCpufreqPath); os.IsNotExist(err) { | ||
continue | ||
} else { | ||
if _, err = os.Stat(filepath.Join(cpuCpufreqPath, "scaling_cur_freq")); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd personally go with a loop with the file names as the slice elements over if/else if.
sysfs/system_cpu.go
Outdated
} | ||
|
||
func parseCpufreqCpuinfo(prefix string, cpuPath string) (SystemCpuCpufreq, error) { | ||
current, err := util.ReadUintFromFile(filepath.Join(cpuPath, prefix+"_cur_freq")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you can probably shorten this up some by using loops for all of the types that need "ReadUintFromFile" and then "SysReadFile".
d5d1caa
to
633577a
Compare
internal/util/parse.go
Outdated
if err != nil { | ||
return 0, err | ||
} | ||
value, err := strconv.ParseUint(strings.TrimSpace(string(data)), 10, 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you could just return strconv.ParseUint(/* ... */)
at this point.
sysfs/system_cpu.go
Outdated
} | ||
|
||
// SystemCpufreq is a collection of SystemCPUCpufreq for every CPU. | ||
type SystemCpufreq map[string]SystemCPUCpufreq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you see my previous thought?
Since there are no methods on this type, I'd remove the type.
internal/util/parse.go
Outdated
return nil, err | ||
} | ||
|
||
return b[:n], nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it seems like a common need, we could bytes.TrimSpace
on the return value here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also returning it as a string
makes sense.
sysfs/system_cpu.go
Outdated
} | ||
|
||
// SystemCpufreq is a collection of SystemCPUCpufreq for every CPU. | ||
type SystemCpufreq []SystemCPUCpufreq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this type has no methods, I would probably remove it and just return the slice directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does now. func (s SystemCpufreq) parseCpufreqCpuinfo(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @mdlayher means getting rid of SystemCpufreq
and returning []SystemCPUCpufreq
instead and I agree on that.
sysfs/system_cpu.go
Outdated
return systemCpufreq, nil | ||
} | ||
|
||
func (s SystemCpufreq) parseCpufreqCpuinfo(prefix string, cpuPath string) (*SystemCPUCpufreq, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an idea to tighten up this function: https://play.golang.org/p/T35yJhQlsmZ
Same logic could apply to the SysReadFile parts with a second loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, good idea.
sysfs/system_cpu.go
Outdated
) | ||
|
||
// SystemCPUCpufreq contains stats from devices/system/cpu/cpu[0-9]*/cpufreq/... | ||
type SystemCPUCpufreq struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would go with something like SystemCPUFrequency
as that's more human friendly than the Linux directory name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the literal Linux names most of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, I don't like either of these names. The name should convey what this is about. So maybe (System)CPUStats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly prefer to use the names we get from Linux, this is the least surprising mapping between the library and the kernel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get where you're coming from but if you just look at the code without knowing the linux names it's just hard to read. Let's append at least Stats
or something because this struct isn't a "frequency" then?
@mdlayher Any other issues to fix up? |
Sorry, not much spare time this week. @grobie and @discordianfish ? |
sysfs/system_cpu.go
Outdated
systemCPUCpufreqClass.MinimumFrequency = uintOut[2] | ||
systemCPUCpufreqClass.TransitionLatency = uintOut[3] | ||
|
||
stringFiles := [5]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use []string instead. No need to pre-allocate this. Pretty sure the compiler does the right thing anyway.
sysfs/system_cpu.go
Outdated
"related_cpus", | ||
"scaling_setspeed", | ||
} | ||
var stringOut [5]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd perfer stringOut := make([]string, len(stringFiles))
. No need to optimize on cost of readability.
* Add parser for `devices/system/cpu/cpu*/cpufreq/...`. * Add helper `util.ReadUintFromFile()`. Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
* Add `Name` to information struct. * Use parser as method. Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Ok, I've cleaned up a bunch of things. I think I've resolved all the comments, PTAL. |
/cc @metalmatze |
Looking at this I can't find any issues regarding the Go code itself (except some nits that don't change anything, probably personal preference). As I said earlier today, I'm not an expert when it comes to these internal APIs. |
@discordianfish Any last issues? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add parser for sysfs devices/system/cpu/... Signed-off-by: Ben Kochie <superq@gmail.com>
devices/system/cpu/cpu*/cpufreq/...
.util.ReadUintFromFile()
.sysReadFile()
helper fromsysfs
tointernal/util
.Signed-off-by: Ben Kochie superq@gmail.com