Added support for apcupsd #2552

jonaz · 2017-03-20T19:04:01Z

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)
README.md updated (if adding a new plugin)

jonaz · 2017-03-21T08:59:51Z

Does circle-ci not allow tcp server/client? The tests worked on my machine...

--- FAIL: TestClientNoKnownKeyValuePairs (0.00s)
	client_test.go:141: failed to dial Client: dial tcp [::]:44573: connect: network is unreachable
--- FAIL: TestClientAllTypesKeyValuePairs (0.00s)
	client_test.go:141: failed to dial Client: dial tcp [::]:37356: connect: network is unreachable

I found a bug report here: golang/go#18806
So circle-ci seems to be missing ipv6 loopback address.

I'll try to fix the tests by binding to ipv4

Edit: ive now fixed the tcp issue using suggested workarround in the golang issue.

jonaz · 2017-06-09T09:19:23Z

@danielnelson have i missed anything or why does approval take such a long time?

danielnelson · 2017-06-09T18:27:27Z

I'm spending most of my time on bugs, but I'm planning to review this soon.

danielnelson · 2017-06-16T21:15:25Z

Normally we don't bundle libraries, where is the apcupsd_client library from?

kylejvrsa · 2017-08-28T11:15:56Z

Wouldn't it be better to support http://networkupstools.org/ rather than just apcupsd? That way it will cover more UPS manufacturers or am I missing something?

jonaz · 2017-08-28T11:53:45Z

@danielnelson
its based on https://github.com/mdlayher/apcupsd with some enhancements (added some more fields etc)

Just did it the same way as in https://github.com/influxdata/telegraf/tree/master/plugins/inputs/hddtemp/go-hddtemp
with the client code in separate directory.

The alternative would have been to fork it on my github user and then use it as a dep. But since its so small i didnt see the point. Or just put those files directly in the main apcupsd folder.

@kylejvrsa I dont know. I've only ever used apcupsd in the last 15 years.

danielnelson · 2017-08-29T00:41:09Z

From what I can tell, according to Debian popcon apcupsd is about twice as popular as nut, though we could definitely have plugins for both of them. Unless there is a common API I think this would be required.

@jonaz Have you opened a pull request to mdlayher/apcupsd? Ideally the changes could be incorporated upstream so we don't need to have a fork.

jonaz · 2017-10-02T06:32:11Z

@danielnelson no i have not opened a PR there.

danielnelson · 2017-10-02T23:08:21Z

Can you open the PR and set this up to use the library?

jonaz · 2018-01-21T09:49:46Z

Sorry for the delay. Beeing a parent is a new thing for me :)

A PR is open upstream now: mdlayher/apcupsd#4

Once thats merged i will update this PR and add it with gdm.

jonaz · 2018-01-25T19:35:08Z

@danielnelson now using upstream library!

danielnelson · 2018-01-29T19:59:31Z

plugins/inputs/apcupsd/apcupsd.go

+			"input_voltage":          status.LineVoltage,
+			"load_percent":           status.LoadPercent,
+			"battery_charge_percent": status.BatteryChargePercent,
+			"time_left":              status.TimeLeft.Minutes(),


Why did you select minutes as the unit for this field, is this the max resolution?

We should add the unit to the field name too, time_left_minutes.

What we get from apcupsd is in minutes: TIMELEFT : 103.0 Minutes
I will suffix unit.

danielnelson · 2018-01-29T20:02:15Z

plugins/inputs/apcupsd/apcupsd.go

+		}
+
+		fields := map[string]interface{}{
+			"online":                 status.Status == "ONLINE",


Maybe we should have a tag for status? We can still have this information encoded as a field but we should consider encoding it as an int instead of a bool, which while harder to understand does work better with many visualization programs.

danielnelson · 2018-01-29T20:06:48Z

plugins/inputs/apcupsd/apcupsd.go

+}
+
+func fetchStatus(addr string) (*apcupsd.Status, error) {
+	client, err := apcupsd.Dial("tcp", addr)


Would it be possible to add timeouts on all the network operations, perhaps using a Context? This would require a modification in the upstream library but would prevent network issues from making Telegraf unresponsive.

Should that timeout be configurable in the telegraf config or should i just set it to like 5 sec?

PR is open upstream to support DialContext: mdlayher/apcupsd#5

EDIT: i will make it configurable

jonaz · 2018-02-10T21:26:41Z

@danielnelson addressed comments

bgulla · 2018-03-08T23:27:06Z

can this be merged? I would really like to start using this.

danielnelson · 2018-03-08T23:27:01Z

plugins/inputs/apcupsd/apcupsd.go

+	}
+	defer client.Close()
+
+	return client.Status()


Can we get read and write timeouts in apcupsd (StatusContext(ctx) or such)? We can use the same timeout for the entire operation: dial + status.

danielnelson · 2018-03-08T23:29:53Z

plugins/inputs/apcupsd/apcupsd.go

+var sampleConfig = `
+  # a list of running apcupsd server to connect to. 
+  # If not provided will default to 127.0.0.1:3551
+  servers = ["127.0.0.1:3551"]


Sorry I didn't mention this sooner, but let's make this a URL: "tcp://127.0.0.1:3551" and use url.Parse to split it up. This will allow ipv6 support and we won't have to adjust it later (something I have been doing a lot of recently).

MorphBonehunter · 2018-07-19T07:57:55Z

@jonaz have you the time to look at this again?

jonaz · 2018-07-19T09:41:02Z

Yes when I'm back from parental leave in October

…

On Thu, Jul 19, 2018, 09:58 Daniel Wendler ***@***.***> wrote: @jonaz <https://github.com/jonaz> have you the time to look at this again? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2552 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABF-FRhSWee4jPc0BiBg_KhJx5AUSspQks5uIDwNgaJpZM4Mi4HW> .

glinton · 2018-07-25T23:34:36Z

@jonaz, if you'd like to cherry-pick these two commits: ac2b3ca 14bfe14 from this branch, we can move forward merging.
alternatively, i could push them to the branch on your repo

glinton · 2018-07-26T17:50:54Z

I suppose 029017e wouldn't hurt either, then merge master in

jonaz · 2018-07-26T18:06:15Z

@glinton rebased on latest master and merged your 3 commits.

danielnelson · 2018-07-26T22:01:08Z

Gopkg.lock

@@ -3,14 +3,14 @@

 [[projects]]
  branch = "master"
-  digest = "1:d7582b4af1b0b953ff2bb9573a50f787c7e1669cb148fb086a3d1c670a1ac955"
+  digest = "1:fc0802104acded1f48e4860a9f2db85b82b4a754fca9eae750ff4e8b8cdf2116"


Need to regenerate with dep v0.4.1 to avoid lockfile churn. Should just be installing the right version and running dep ensure

danielnelson · 2018-07-26T22:08:53Z

plugins/inputs/apcupsd/apcupsd_test.go

+	"github.com/influxdata/telegraf/testutil"
+)
+
+func TestApcupsdGather(t *testing.T) {


We need to split the tests out into individual test cases. Each test should try to check only one thing, and similar tests can use a table driven test if possible.

danielnelson · 2018-07-26T22:10:13Z

plugins/inputs/apcupsd/apcupsd_test.go

+	}
+
+	if testing.Short() {
+		t.Skip("Skipping network dependent tests in short mode")


It is okay to not skip the network tests so long as they don't require any outside services to be running, and they complete immediately.

danielnelson · 2018-07-26T22:33:24Z

plugins/inputs/apcupsd/apcupsd_test.go

+		in := make([]byte, 128)
+		n, err := conn.Read(in)
+		if err != nil {
+			t.Fatal(fmt.Sprintf("failed to read from connection - %s", err.Error()))


Use testify for these messages:

require.NoError(t, err)

If you want a custom error message:

require.NoError(t, err, "failed to read from connection")

Make sure you shutdown correctly...

danielnelson · 2018-07-26T22:36:02Z

plugins/inputs/apcupsd/apcupsd.go

+			"internal_temp":          status.InternalTemp,
+			"battery_voltage":        status.BatteryVoltage,
+			"input_frequency":        status.LineFrequency,
+			"time_on_battery":        status.TimeOnBattery.Minutes(),


Add units: time_on_battery_minutes or minutes_on_battery

I now believe here it would be best to convert to nanoseconds and report the field as time_on_battery_ns. We are moving to this encoding for time durations and it can be shown easily in Grafana using the time -> nanoseconds unit.

danielnelson · 2018-07-26T22:37:15Z

plugins/inputs/apcupsd/apcupsd.go

+		tags := map[string]string{
+			"serial":   status.SerialNumber,
+			"ups_name": status.UPSName,
+			"online":   status.Status,


I think this should be status=ONLINE because it would be confusing if we have online=OFFLINE.

danielnelson · 2018-07-26T22:37:56Z

plugins/inputs/apcupsd/apcupsd.go

+		}
+
+		fields := map[string]interface{}{
+			"online":                 boolToInt(status.Status == "ONLINE"),


We should have this be a key that is different from the tag name to simplify querying the data, if we change the tag to status then this should be status_code.

status can be all those things. so the most correct think here would be to put status_online as a 1/0 and keep the whole text in tag status

// Now output status in human readable form statstr = ""; if (status & UPS_calibration) statstr += "CAL "; if (status & UPS_trim) statstr += "TRIM "; if (status & UPS_boost) statstr += "BOOST "; if (status & UPS_online) statstr += "ONLINE "; if (status & UPS_onbatt) statstr += "ONBATT "; if (status & UPS_overload) statstr += "OVERLOAD "; if (status & UPS_battlow) statstr += "LOWBATT "; if (status & UPS_replacebatt) statstr += "REPLACEBATT "; if (!(status & UPS_battpresent)) statstr += "NOBATT ";

I see, sets are a little tricky for us to encode nicely.

I think this combined status string might be hard to use in queries, because you may need to look for many string combinations to select what the series you are interested in. Looking at that code snippet and also these status-bits, and doing more interpretation (which could get us into trouble), it seems like the UPS status can be consider one of: online, on_battery, or unknown. The battery can be: okay, low, no_battery, or replace. Finally there are warning that output is overloaded, and informational messages about cal, trim, and boost.

What if we expose the ups status values and battery status as tags + enum field, and the other status flags as boolean fields. I'm on the fence if overload should be a tag.

apcupsd,status=online,battery=ok status_code=0i,battery_code=0i,overload=false,smarttrim=false,smartboost=false,calibration=false

MorphBonehunter · 2019-01-17T07:59:54Z

@jonaz did you see any chance to get this one ready for 1.10. ?

jonaz · 2019-01-17T09:44:22Z

@MorphBonehunter I dont think so. My spare time is limited at the moment. But if someone else would like this in and can address the comments in this PR im as happy as the other people waiting to get this in :)

danielnelson · 2019-09-10T18:23:42Z

This PR was merged in #6226, thanks @jonaz

danielnelson added the plugin request label Mar 21, 2017

jonaz force-pushed the feature/apcupsd branch 2 times, most recently from c7615a7 to d69c277 Compare March 22, 2017 11:46

danielnelson mentioned this pull request Jun 9, 2017

Create an APCUPCd Plug-in #2877

Closed

danielnelson added this to the 1.4.0 milestone Jun 9, 2017

danielnelson added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin request labels Aug 12, 2017

danielnelson modified the milestones: 1.4.0, 1.5.0 Aug 14, 2017

danielnelson added the new plugin label Sep 18, 2017

danielnelson removed this from the 1.5.0 milestone Nov 29, 2017

jonaz force-pushed the feature/apcupsd branch from d69c277 to 55ca027 Compare January 25, 2018 14:41

danielnelson reviewed Jan 29, 2018

View reviewed changes

jonaz force-pushed the feature/apcupsd branch 2 times, most recently from b68e0ad to 31c6279 Compare February 10, 2018 21:23

danielnelson reviewed Mar 8, 2018

View reviewed changes

jonaz and others added 4 commits July 26, 2018 20:01

Added support for apcupsd

cccb56d

Use scheme in address when fetching status

fbbb468

Add tests for apcupsd input

a12a520

Fix minor formatting issues

57befbf

jonaz force-pushed the feature/apcupsd branch from 2be175f to 57befbf Compare July 26, 2018 18:05

Reorder all.go

84197b4

glinton approved these changes Jul 26, 2018

View reviewed changes

danielnelson suggested changes Jul 26, 2018

View reviewed changes

glinton mentioned this pull request Aug 7, 2019

Add apcupsd input plugin #6226

Merged

3 tasks

danielnelson closed this Sep 10, 2019

Added support for apcupsd #2552

Added support for apcupsd #2552

Uh oh!

Conversation

jonaz commented Mar 20, 2017

Required for all PRs:

Uh oh!

jonaz commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonaz commented Jun 9, 2017

Uh oh!

danielnelson commented Jun 9, 2017

Uh oh!

danielnelson commented Jun 16, 2017

Uh oh!

kylejvrsa commented Aug 28, 2017

Uh oh!

jonaz commented Aug 28, 2017

Uh oh!

danielnelson commented Aug 29, 2017

Uh oh!

jonaz commented Oct 2, 2017

Uh oh!

danielnelson commented Oct 2, 2017

Uh oh!

jonaz commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonaz commented Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonaz Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonaz commented Feb 10, 2018

Uh oh!

bgulla commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorphBonehunter commented Jul 19, 2018

Uh oh!

jonaz commented Jul 19, 2018 via email

Uh oh!

glinton commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glinton commented Jul 26, 2018

Uh oh!

jonaz commented Jul 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonaz commented Mar 21, 2017 •

edited

Loading

jonaz commented Jan 21, 2018 •

edited

Loading

jonaz Feb 10, 2018 •

edited

Loading

glinton commented Jul 25, 2018 •

edited

Loading