Added local_time metric with time_zone label to OS collector #67
Conversation
collector/os.go
Outdated
| ch <- prometheus.MustNewConstMetric( | ||
| c.LocalDateTime, | ||
| prometheus.GaugeValue, | ||
| float64(time.Unix()), |
There was a problem hiding this comment.
Can you clarify, is this local time or is it unix time? If local time, what does that mean?
There was a problem hiding this comment.
This is Unix time (as described in https://golang.org/pkg/time/#Time.Unix).
There was a problem hiding this comment.
Local time implies that timezones are involved, which is not the case for unixtime. This should be just called "time".
The timezone should go on a separate 1-valued metric.
There was a problem hiding this comment.
You are right, see the latest updated code below. Can you do a review for that as well?
collector/os.go
Outdated
| nil, | ||
| ), | ||
| TimeZone: prometheus.NewDesc( | ||
| prometheus.BuildFQName(Namespace, subsystem, "time_zone"), |
|
This seems fine from a metrics standpoint, but I'll leave on of the WMI people to review and merge. |
collector/os.go
Outdated
| @@ -26,6 +27,8 @@ type OSCollector struct { | |||
| PagingLimitBytes *prometheus.Desc | |||
| VirtualMemoryBytes *prometheus.Desc | |||
| VisibleMemoryBytes *prometheus.Desc | |||
| Time *prometheus.Desc | |||
| TimeZone *prometheus.Desc | |||
collector/os.go
Outdated
| nil, | ||
| ), | ||
| TimeZone: prometheus.NewDesc( | ||
| prometheus.BuildFQName(Namespace, subsystem, "time_zone"), |
collector/os.go
Outdated
| TimeZone: prometheus.NewDesc( | ||
| prometheus.BuildFQName(Namespace, subsystem, "time_zone"), | ||
| "OperatingSystem.LocalDateTime", | ||
| []string{"time_zone"}, |
|
Implementation-wise, this looks fine to me. Thanks @atezs82! |
Seconds is the convention in Prometheus.
There are multiple timezones that can result in the same offsets at various times of the year. For example, I'm in UTC for half the year. The full timezone name is what you need. |
Right, sloppy phrasing on my part. What I meant was that what WMI exposes is the offset in minutes. It should be exposed as seconds, as you say.
After re-reading the code and trying a few zone/dst changes locally, I can agree to this. I had two concerns, which both seem to have been wrong:
So, in short, I'm fine with this. |
"time" for the Unix time "timezone" for the name of the timezone
|
Thanks for all the comments! I did my best to incorporate them. |
|
@atezs82 Thank you very much for this contribution! |
Added a new metric called wmi_os_local_time to the exporter, with which OS time on Windows can be also scraped from Prometheus (and, for instance alerts can be generated when it does not match the time of the server). The metric also contains a label called time_zone in which the time zone on Windows is stored (and can be compared accordingly with any value in Prometheus).