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.

Sign up for GitHub

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

Jump to bottom

system: fix uptime calculation #41

Merged
merged 1 commit into from
Nov 16, 2016
Merged

system: fix uptime calculation #41

merged 1 commit into from
Nov 16, 2016

Conversation

martinlindhe
Copy link
Collaborator

…raw data doesnt change. fixes #40

@martinlindhe martinlindhe mentioned this pull request Nov 14, 2016
Closed
@martinlindhe
Copy link
Collaborator Author

Would like input from @carlpett or @brian-brazil before merging this.

Also, this change makes existing exporters confused I guess since the value is radically changed, what would be best way to handle this. I would prefer semver (eg release 0.2.0 with this change?)

@martinlindhe
Copy link
Collaborator Author

Second, would it make more sense to convert this to a unix timestamp? [1]

1: https://prometheus.io/docs/practices/instrumentation/#timestamps-not-time-since

@carlpett
Copy link
Collaborator

Had a look at this - the way they calculate it in the formatted data is by using TimestampObject: (TimestampObject-SystemUpTime)/TicksToSeconds comes out correct. So I'd suggest we do that instead, so we don't have to query a new object.

Regarding the continuity of the metric, I'm not sure. Since the previous value was broken, I'd consider it a bugfix rather than a "feature", so I don't think it needs a point release from a semver point of view.

@brian-brazil
Copy link

Generally we should always export raw data. For example there's probably a race in here in somewhere that can cause the value to fluctuate slightly.

@martinlindhe
Copy link
Collaborator Author

Reworked the patch:

  • dropped usage of preformatted data
  • calc uptime as carl suggested: (Timestamp_Object-SystemUpTime)/Frequency_Object

carlpett
carlpett reviewed Nov 15, 2016
View reviewed changes
collector/system.go
var dst []Win32_PerfRawData_PerfOS_System
if err := wmi.Query(wmi.CreateQuery(&dst, ""), &dst); err != nil {
var raw []Win32_PerfRawData_PerfOS_System
if err := wmi.Query(wmi.CreateQuery(&raw, ""), &raw); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nitpick: Now that you aren't pulling in multiple counters, might as well revert this name to dst, since that is what we use everywhere else (in truth, I'm not sure why we use that name, though? 😄 )

@martinlindhe martinlindhe force-pushed the fix_system_uptime branch 2 times, most recently from a5ddd59 to 59c760e Compare November 15, 2016 08:05
@martinlindhe
Copy link
Collaborator Author

martinlindhe commented Nov 15, 2016 •
edited
Loading

Reworked again. Now it converts the Windows timestamp into a unix timestamp.

@martinlindhe martinlindhe changed the title system: read uptime from Win32_PerfFormattedData_PerfOS_System since … system: fix uptime calculation Nov 15, 2016
@brian-brazil
Copy link

👍

@martinlindhe martinlindhe merged commit 2808da6 into master Nov 16, 2016
@carlpett carlpett deleted the fix_system_uptime branch May 12, 2018 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlpett carlpett carlpett left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make wmi_system_system_up_time format more Prometheus friendly
3 participants
@martinlindhe @carlpett @brian-brazil