Add vmstat interface #59
Conversation
|
Okay, down to the actual code issues now. Fixing. |
|
Okay, lets try that. |
types/host.go
Outdated
| ThpSwpout uint64 `vmstat:"thp_swpout"` | ||
| ThpSwpoutFallback uint64 `vmstat:"thp_swpout_fallback"` | ||
| SwapRa uint64 `vmstat:"swap_ra"` | ||
| SwapRaHit uint64 `vmstat:"swap_ra_hit"` |
There was a problem hiding this comment.
Maybe this is too much info? 😬 Should we have an struct that can be more easily implemented for other OSs?
Even if other OSs don't have a /proc/vmstat, I guess that they will have some similar virtual memory stats.
There was a problem hiding this comment.
Yah, I was thinking of this. The thing is, this library makes an effort to be highly cross-platform, and we already have a lot of platform-independent higher-level memory APIs, I figured it couldn't hurt to have some more bare-metal stats. Also, I didn't want to add overhead to the main Memory() Call by adding extra sources to it that most users won't need. I'm not sold on it though, since it might be a pain for clients to handle this for multiple OSes if we come up with similar low-level APIs for other OSes.
There was a problem hiding this comment.
If we did it some other way, how would the user get platform-dependent stats?
providers/linux/vmstat.go
Outdated
| return true | ||
| } | ||
| return false | ||
| }) |
providers/linux/vmstat.go
Outdated
| // Search The struct object to see if we have a field with a tag that matches the raw key coming off the file input | ||
| // This is the best way I've found to "search" for a a struct field based on a struct tag value. | ||
| // In this case, the /proc/vmstat keys are struct tags. | ||
| fieldToSet := refVal.FieldByNameFunc(func(name string) bool { |
There was a problem hiding this comment.
Perhaps it would make sense to iterate over the fields and read their tags just once to build up a map of tag to Value. This looks likes it's O(n^2) now.
There was a problem hiding this comment.
Ugh, yah, I think you're right. This is gonna end up getting called by metricbeat on every Fetch, so I'll see what I can do.
fearful-symmetry
force-pushed
the
add-vmstat
branch
from
06f24cb
to
a48bf8f
Compare
providers/linux/vmstat.go
Outdated
|
|
||
| // parseVMStat parses the contents of /proc/vmstat | ||
| func parseVMStat(content []byte) (*types.VMStatInfo, error) { | ||
| vmStat := &types.VMStatInfo{} |
There was a problem hiding this comment.
How about something like this where you bootstrap a mapping just once?
// vmstatTagToFieldIndex contains a mapping of json struct tags to struct field indices.
var vmstatTagToFieldIndex map[string]int
func init(){
var vmstat VMStatInfo
val := reflect.ValueOf(vmstat)
typ := reflect.TypeOf(vmstat)
vmstatTagToFieldIndex = map[string]int{}
for i := 0; i < val.NumField(); i++ {
field := typ.Field(i)
if tag := field.Tag.Get("json"); tag != "" {
vmstatTagToFieldIndex[tag] = i
}
}
}
var vmStat VMStatInfo
val := reflect.ValueOf(&vmStat).Elem()
if idx, found := vmstatTagToFieldIndex["my_key"]; found {
f := val.Field(idx)
if f.CanSet() {
f.SetUint(42)
}
}
There was a problem hiding this comment.
I briefly thought of that but decided against it, but it didn't occur to me to use the struct index instead of the raw Value. Let's try that.
|
Alright, this moves it to an |
providers/linux/vmstat.go
Outdated
|
|
||
| // parseVMStat parses the contents of /proc/vmstat | ||
| func parseVMStat(content []byte) (*types.VMStatInfo, error) { | ||
|
|
There was a problem hiding this comment.
Can you drop this newline please.
See elastic/beats#13175 This took me longer than expected, as I needed to figure out how this library actually works, and it's a tad different from how gosigar works. This should hopefully be fine; I've attempted to run it against the test suite and the unit tests works, but I still need to put together some "IRL" tests on a normal linux host. Also not entirely sure the test suite is working.
We also need godocs for a bunch of the types, but that's a separate PR.