search

mdlayher / apcupsd

Package apcupsd provides a client for the apcupsd Network Information Server (NIS). MIT Licensed.
MIT License
45 stars 22 forks source link

XOFFBATT may have value "N/A", causing parsing to fail #2

Closed rschaeuble closed 7 years ago

rschaeuble commented 7 years ago

I have just realized (unfortunately before submitting merge request #1) that the fields XONBATT and XOFFBAT are not always set to a date value. If no transfers to or from battery have happened since apcupsd started, the fields have the value "N/A". In this case, parsing the value as date fails, causing the whole call to client.Status to fail.

I see two possible ways to fix this:

I think I'd prefer the second variant. Currently one single problematic value leads to no status being returned at all. I think it might be better to just have the one unparseable field set to nil but still get all the other values that could be parsed.

@mdlayher I'll gladly implement the fix and submit a new merge request, but I'd like to hear your preference first.

mdlayher commented 7 years ago

I like option 1 actually. Logging in libraries is a nuisance for users of the library, and we can just return a zero value compatible with https://golang.org/pkg/time/#Time.IsZero on "N/A".

Do you agree? Seems most reasonable to me, and there would be an easy method to check for that zero value if the caller needed to.

rschaeuble commented 7 years ago

The main reason why I suggested option 2 is robustness. In case one field can't be parsed (for whatever reason), I'd still like the other values to show up in Prometheus (I'm using this library indirectly through your apcupsd_exporter).

But it's just a preference I'm having for option 2. I still see option 1 as a perfectly valid one. Also, it's your library, so if you'd prefer that option, I'll gladly implement it that way. On Wed, 12 Apr 2017 at 00:28 Matt Layher notifications@github.com wrote:

I like option 1 actually. Logging in libraries is a nuisance for users of the library, and we can just return a zero value compatible with https://golang.org/pkg/time/#Time.IsZero on "N/A".

Do you agree? Seems most reasonable to me, and there would be an easy method to check for that zero value if the caller needed to.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mdlayher/apcupsd/issues/2#issuecomment-293418868, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVjUE8bSRgXmWbu5qgC-phUtJYoZkGkks5ru_6GgaJpZM4M6t8b .

mdlayher commented 7 years ago

Yep, if we return an empty time.Time{} and nil error when apcupsd reports "N/A" then it'll be fine. I think this makes sense, and like I said, the caller has a way of determining if time is zero too.

rschaeuble commented 7 years ago

Correction to my previous observation: only XOFFBATT seems to report as NA sometimes. XONBATT just isn't reported when it's not available. So much for consistent/logical behavior of the apcupsd NIS.

mdlayher commented 7 years ago

I'm not at all surprised. :) I get the feeling the NIS was designed for humans and not machine parsing.

The good news is that the zero value in Go will be the same as parsing "N/A" into a zero time.Time.

rschaeuble commented 7 years ago

Yeah, I like that the status struct has the time fields as values, not as pointers. Safes the user from having to distinguish between nil and empty time.