multiplatform support for cpuinfo

[pgier]

This adds basic handling of reading /proc/cpuinfo for ARM, PPC, and
s390x.
Fixes #256

[Prashanth684]

don't we want to parse the "clock" field in the cpu info for cpu MHz like for x86 ?

[pgier]

@Prashanth684 thanks for the feedback! I've added support for CPUMHz on both PPC and s390x.

[Prashanth684]

@Prashanth684 thanks for the feedback! I've added support for CPUMHz on both PPC and s390x.

no problem. thanks for this!

[Prashanth684]

lgtm

[discordianfish]

Looks good to me in general but I'm wondering if we should rather use build flags to determine the expected format. Or is there a use case where a x64 system would read a arm cpuinfo?

[pgier]

@discordianfish I did it this way originally because I was thinking that might be able to handle platforms that weren't accounted for, and it seemed easier to test this way. But I think you are right that build flags would be better.

[discordianfish]

@pgier kk, awaiting your changes :)

[pgier]

@discordianfish I experimented a bit with separate files and build flags for the different arches, but I don't have an easy way to test the various platforms. I'm wondering if we should just leave this as is since none of the code is really platform dependent, it only depends on the different text file formats. WDYT?

[discordianfish]

@pgier Hrm I don't like this implicit runtime detection. I'd say we should either use build flags or add functions for each platform and leave it to the caller to invoke the right one for the platform.

But what was the problem with separate files and build flags? Just the tests? Maybe we could keep the functions but use platform specific files with build flags that implement the correct function for the platform, e.g:

cpuinfo_amd64.go:

var parseCPUInfo = parseCPUInfoX86

...and then you can still test all functions on x86.

[pgier]

@discordianfish Thanks for the suggestions, sorry it took so long to get this updated! Please take another look.

[discordianfish]

Not sure we need and want to maintain the detection code. All this would work without it, right? We just need to run parseCPUInfo which is set in the platform specific files.
I'm not strictly against it, but I don't really see where this would be useful. I can't think of a case where I would like to parse a foreign cpuinfo.

[pgier]

@discordianfish ok, I have removed the format detection code.

[SuperQ]

LGTM

[mfraioli]

Hey guys, I'm trying to build some code which apparently pulls in this procfs project as a dependency, and it started failing yesterday on ppc and s390 with this error:

+ go get github.com/uber/jaeger-lib/metrics/prometheus
# github.com/prometheus/procfs
golib/src/github.com/prometheus/procfs/cpuinfo.go:71:9: undefined: parseCPUInfo

The actual go build command fails in the same way.

I assume it's related to this change. I see that parseCPUInfo is a function variable that is set in a platform-specific file to point to the platform-specific function, but I"m not clear on how the appropriate file is supposed to be included. Somehow it needs to know to use cpuinfo_ppc64.go on ppc, etc. How is this meant to be specified? Thanks!

[mfraioli]

On 390 there is a slightly different error:

+ go get github.com/prometheus/client_golang/prometheus/promhttp
# github.com/prometheus/procfs
golib/src/github.com/prometheus/procfs/cpuinfo.go:71:2: not enough arguments to return
golib/src/github.com/prometheus/procfs/cpuinfo_s390x.go:19:20: undefined: parseCPUInfoS390x

This seems to be a simple typo in cpuinfo_s390x.go. It specifies the function name as parseCPUInfoS390x but it should be parseCPUInfoS390X (upper case X at the end).

For the ppc problem, the issue may be with the build constraints in cpuinfo_ppc64.go:

// +build linux
// +build ppc64,ppc64le

The Go doc on build constraints says:

A build constraint is evaluated as the OR of space-separated options. Each option evaluates as the AND of its comma-separated terms.

So that means this will only be included if both ppc64 AND ppc64le are set, but I believe only one or the other will ever be true.

[mfraioli]

With

// +build ppc64,ppc64le

corrected to:

// +build ppc64 ppc64le

it still doesn't work, because go build filters by the filename as well. Since I am using ppc64le, the file cpuinfo_ppc64.go gets left out. If I rename it to cpuinfo_ppc64le.go then it works. So probably it needs to be two separate files, one for each platform.

[pgier]

@mfraioli thanks so much for reporting this! I created #288 which hopefully fixes the issues. Can you give that one a quick look?