Use perflib for cpu collector #335
Conversation
collector/perflib.go
Outdated
| } | ||
|
|
||
| const ( | ||
| perfCounterRawcountHex uint32 = 0 |
There was a problem hiding this comment.
I would write these numbers in hex as there's less symmetry in their decimal representation
|
Looks good, anything left to work out for the cpu collection over perflib? Also looks like CI chokes on perflib not vendored |
|
Yeah, haven't vendored yet because I'm waiting for a PR to be merged, figured I could get some feedback first :) |
|
... Except docs, I just realized as I was about to merge. This adds a bunch new metrics. |
collector/perflib.go
Outdated
| return indexed, nil | ||
| } | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Most of these constants are exported by perflib_exporter: https://github.com/leoluk/perflib_exporter/blob/master/collector/mapper.go
(as of leoluk/perflib_exporter#14)
There was a problem hiding this comment.
Ah, I was only looking at the perflib package.
|
Hi @carlpett. Will these perflib metrics be completely new or will they replace the old WMI metrics? I ask this question because I think there will be some work to transition from the old wmi metrics to these new ones, dashboards, alerts and so on. This is a transition we must do, because of loosing a lot of metrics during peek loads. |
|
@theimdal They will be identical to the current metrics, so you should not need to make any changes at all! |
|
This looks great. We've got a fairly large windows install base and have been hitting a lot of WMI issues lately, so for us this is definitely a good direction to take this project. |
|
I'm excited about this concept! |
|
@nbellowe Yes, I would expect the majority of collectors to be converted once we feel confident in the results. There are a few collectors which don't have any direct raw counter, eg For now, we'd really appreciate it if you have time to test out the prerelease (here) on a few systems and see if it makes the situation better for you! Especially the new |
|
@carlpett I'm running both instances with these arguments: I wondered if the process filtering could be an issue, but didn't really see any difference after removing that (and the process collector) Is there any extra information I can provide? |
|
@petedishman Thanks for trying it out! I've opened a new issue (#345) where we can investigate further. |
|
How is this looking overall? Is this a path wmi_exporter will want to continue down? |
|
@poblahblahblah Yes, I believe so. It fixes a lot of issues we've been seeing over the years, so I definitely want to move ahead with it. I've been somewhat swamped the last few weeks/months though, and haven't had time to put the finishing touches in on this. Hope to have a free weekend soon. |
Abort scrapes after configurable timeout
carlpett
changed the title
|
Sigh, thought I was ready to merge, but managed to find a data race. |
|
This now passes quite a long period of the Go race detector at least, and is probably easier to reason about than the previous nest of goroutines and channels. |
|
@martinlindhe Please have a look when you have time |
|
🎉 Glad to see perflib being used! |
|
🎉 |
…-cpu Use perflib for cpu collector
This is a draft implementation of how we could start switching collectors over to perflib. Main structural change is introducing a "scrape context" which for now just carries the perflib data, but could later be used for cancellations etc.
This scrape context is populated with a "Global" perflib reading at the start of the scrape, and collectors can then pull from this reading.
I have implemented a simple tag-based unmarshaller of perflib data, so the collector merely builds a struct with tags matching the counters, calls unmarshal and can then get down to pushing metrics back the channel.