Closed eosti closed 1 year ago
Thanks for your work on this! Just a smaller change is required.
A small change: now the getPowerStatus
command will log an error explicitly instead of what it was doing previously with the implicit logging via the output of grep
. There is no change in how the script behaves: this is just to make it more obvious where the error might be coming from and to match style with my modified getTemp
command.
Very cool @eosti !
An odd issue I noticed on my EdgeRouter X (EdgeOSv2.0.8-hotfix.1): it doesn't support temperature checking, so when I enabled the default config, I assumed that the script would automatically detect that and give a nice error message, like the power monitoring does. Instead, it threw some weird error about a metric parse error. I found two issues:
The error message for the temperature is
Temperature not supported on this platform
. Compare that to the one for power:Power status is not supported on this platform
. Note the lack of "is", which would seem to cause thegrep
call to not function properly for the temperature check. This would be an easy and straightforward fix, except...(this took a while to find) Unlike
getPowerStatus
,getTemp
will print the error message itself, and then return null instead of returning the error message. So checking for a substring never would have worked, becausetemperature_info
is empty when there is an error! Gotta love inconsistent code practices.Anyway, this PR changes the check condition to check for null instead of checking for a substring. It also adds the error message that gets lost due to the
getTemp
shenanigans, so that the error will show up in Telegraf logs.Bonus fix: in the default config, the firmware check uses the interval
1d
, which isn't valid. I changed it to24h
which is valid and equivalent.