Add dmcache input plugin #1667

vlasad · 2016-08-24T19:49:55Z

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)

sebito91 · 2017-01-20T16:34:07Z

Checking in on this PR, the CircleCI issues weren't fails from the dmcache code but from other components.

sparrc · 2017-01-21T00:28:45Z

yes, it does appear so, I'll slate this for 1.3 and try to get it reviewed soon

sparrc · 2017-01-24T19:52:56Z

plugins/inputs/dmcache/README.md

+
+```
+[[inputs.dmcache]]
+## Whether to report per-device stats or not


these lines should both be indented by two spaces

sparrc · 2017-01-24T19:53:09Z

plugins/inputs/dmcache/dmcache.go

+}
+
+var sampleConfig = `
+	## Whether to report per-device stats or not


indent by 2 spaces instead of tabs

sparrc · 2017-01-24T19:56:02Z

plugins/inputs/dmcache/dmcache_linux.go

+}
+
+func parseDMSetupStatus(line string) (status map[string]interface{}, err error) {
+	defer func() {


why would this function panic? you should write your code to avoid panics rather than write a recover()

sparrc · 2017-01-24T19:57:17Z

plugins/inputs/dmcache/dmcache_linux.go

+		}
+	}()
+
+	values := strings.Split(line, " ")


you might want to use strings.Fields here in case of multiple spaces or tabs

sparrc · 2017-01-24T19:57:44Z

plugins/inputs/dmcache/dmcache_linux.go

+	values := strings.Split(line, " ")
+	status = make(map[string]interface{})
+
+	status["device"] = values[0][:len(values[0])-1]


all of these should check length of the array to avoid panicking.

or you can check that the array is of a minimum length, and just ignore all metrics if it isn't.

sparrc · 2017-01-24T19:58:32Z

plugins/inputs/dmcache/dmcache_linux.go

+func toInt(s string) int {
+	i, err := strconv.Atoi(s)
+	if err != nil {
+		panic(err)


don't panic, return error

sparrc · 2017-01-24T19:59:58Z

plugins/inputs/dmcache/dmcache_test.go

@@ -0,0 +1,94 @@
+// +build linux


I believe you can run these tests on non-linux system as well

sparrc · 2017-01-24T20:02:36Z

plugins/inputs/dmcache/dmcache.go

+		return &DMCache{
+			PerDevice: true,
+			rawStatus: func() ([]string, error) {
+				out, err := exec.Command("/bin/sh", "-c", "sudo /sbin/dmsetup status --target cache").Output()


you need to document that this plugin requires sudo, and that the telegraf user needs to be setup to be able to execute sudo without a password.

You should also document that sudo /sbin/dmsetup status --target cache is the full command that telegraf will run for debugging purposes.

sparrc · 2017-01-24T20:04:26Z

plugins/inputs/dmcache/dmcache_linux.go

+			fields[f] = calculateSize(data, f)
+		}
+
+		if c.PerDevice {


this if-statement doesn't seem correct to me.

If the user sets per_device = true, then it should report the per-device stats AND total stats.

Is there a reason users would not want total stats? If so it should be a separate config option.

vlasad · 2017-03-29T09:29:59Z

All requested changes were done.

danielnelson · 2017-03-31T22:56:41Z

plugins/inputs/dmcache/dmcache.go

+		return &DMCache{
+			PerDevice: true,
+			rawStatus: func() ([]string, error) {
+				out, err := exec.Command("/bin/sh", "-c", "sudo /sbin/dmsetup status --target cache").Output()


Why is this being ran in a shell instead of just being directly executed?

Maybe it is required for sudo?

Yes. It's required for sudo.

danielnelson · 2017-03-31T22:59:02Z

plugins/inputs/dmcache/dmcache.go

+	inputs.Add("dmcache", func() telegraf.Input {
+		return &DMCache{
+			PerDevice: true,
+			rawStatus: func() ([]string, error) {


I would rather have this be a named function that you pass in to the struct.

danielnelson · 2017-03-31T23:00:41Z

plugins/inputs/dmcache/dmcache.go

+					return nil, err
+				}
+				if string(out) == "No devices found\n" {
+					return nil, nil


Return an empty slice instead of nil

danielnelson · 2017-03-31T23:15:33Z

plugins/inputs/dmcache/dmcache.go

+				}
+
+				status := strings.Split(string(out), "\n")
+				status = status[:len(status)-1] // removing last empty line


This will panic if there is status is not at least length 1, I suggest you strings.TrimRight(out, "\n") before splitting lines.

danielnelson · 2017-03-31T23:28:51Z

plugins/inputs/dmcache/dmcache_linux.go

+	status := make(map[string]interface{})
+	values := strings.Fields(line)
+	if len(values) < 15 {
+		return nil, errors.New("dmsetup status data have invalid format")


Grammar, I suggest "Output from dmsetup could not be parsed"

danielnelson · 2017-03-31T23:42:03Z

plugins/inputs/dmcache/dmcache_linux.go

+	if err != nil {
+		return nil, err
+	}
+	status["cache_used"], err = strconv.Atoi(strings.Split(values[7], "/")[0])


Split once, make sure it return len of 2

danielnelson · 2017-03-31T23:46:04Z

plugins/inputs/dmcache/dmcache_linux.go

+
+func parseDMSetupStatus(line string) (map[string]interface{}, error) {
+	var err error
+	status := make(map[string]interface{})


You should define a struct for these values, that way less casting is needed later

But, in this case, function "calculateSize" will require to use reflection to get value by key.

danielnelson · 2017-03-31T23:47:33Z

plugins/inputs/dmcache/dmcache_linux.go

+	return status, nil
+}
+
+func calculateSize(data map[string]interface{}, key string) (value int) {


If I understand this correctly, we are synthesizing a few aggregate values in this function? Why not just provide the values dmsetup returns, since the user can always calculate these at query time.

#1581 - this is the source of this solution.
@daviesalex could you comment?

Thats exactly what is going on. I had a look at it and I agree, these should be do-able at query time without drama (in very old versions of InfluxDB it was a challenge to do everything at query time if you wanted to select if I recall, which is why I think this started off this way).

@vlasad I suggest we remove this. If we did want to do this, the neat way to do it is probably a output plugin in telegraf.

@sebito91 FYI

danielnelson · 2017-03-31T23:52:19Z

plugins/inputs/dmcache/dmcache_test.go

+		"dirty":         0,
+	}
+	acc.AssertContainsTaggedFields(t, "dmcache", fields, tags)
+}


The tests need to be made more comprehensive. Please try to think of some more corner cases and error conditions to test for. Some things that come to mind is the "No devices found" error and errors from rawOutput(). Try checking the coverage for more ideas.

danielnelson · 2017-03-31T23:54:44Z

plugins/inputs/dmcache/dmcache.go

+
+type DMCache struct {
+	PerDevice bool `toml:"per_device"`
+	rawStatus func() ([]string, error)


I feel like the name rawStatus is not fitting, since this functions both executes the command and does some processing on the output.

danielnelson

I noticed while reading through the manpages for dmsetup that there is a stats subcommand. From what I gathered it requires the user to define a "region" before it can be used. Have you used this before? How do the stats it provides compare to the ones from the status command?

danielnelson · 2017-04-05T00:49:18Z

plugins/inputs/dmcache/README.md

+[[inputs.dmcache]]
+  ## Whether to report per-device stats or not
+  per_device = true
+```


Make sure you document the tags/fields and stick to the format of the example as closely as possible (for instance little things such as three #'s for Configuration.)

https://raw.githubusercontent.com/influxdata/telegraf/master/plugins/inputs/EXAMPLE_README.md

How did you go about naming the fields? Can you link to any documentation you may have used?

All of the fields are basically taken from the output of the dmsetup status --target=cache output for the given disks. In this case, we want to split all the data into multiple lines per disk we set up.

So the output of that command, based on the test cases, is more or less a table of values. Where did you find out what each column represents?

danielnelson · 2017-04-05T00:58:37Z

plugins/inputs/dmcache/dmcache.go

+	return "Provide a native collection for dmsetup based statistics for dm-cache"
+}
+
+func dmSetupStatus() ([]string, error) {


I think this should go into dmcache_linux since it is linux specific.

This is fine, how does one accomplish this on Windows (not a Windows user/fan).

danielnelson · 2017-04-05T01:12:18Z

plugins/inputs/dmcache/dmcache_linux.go

+		return nil, parseError
+	}
+
+	status["device"] = strings.TrimRight(values[0], ":")


Still want these in a struct, this separates the responsibilities. Call it something appropriate that represents the data, maybe CacheStatus. I get why you did this but I think it will clear up some weirdness in the code, such as taking device from fields to add to tags. You won't need to do type conversions and summing will be more explicit. This open PR does a good job. #2031

danielnelson · 2017-04-05T01:14:05Z

plugins/inputs/dmcache/dmcache_test.go

+		"device": "cs-1",
+	}
+	fields1 := map[string]interface{}{
+		"device":             "cs-1",


Device shouldn't be both a tag and a field.

that is valid, in the actual output it's not a field but just a tag.

danielnelson · 2017-04-05T01:30:23Z

plugins/inputs/dmcache/dmcache_linux.go

+	if err != nil {
+		return nil, err
+	}
+


What about the values after dirty, why do we not collect them?

danielnelson · 2017-04-05T01:31:36Z

plugins/inputs/dmcache/dmcache_linux.go

+	if err != nil {
+		return nil, err
+	}
+


What about the values after dirty, do we need to collect them?

Not sure what you're asking here, are you questioning why we're collecting the "dirty" blocks?

I noticed in the sample output included in the tests that there are more values after the column containing "dirty". Are these metrics important, should we be collecting them, and why?

sebito91

Appreciate the feedback on this, it's been a long while getting this collector on-boarded.

Some of the changes you mention are fair, but some of the others I'd have to disagree. In particular, using the dmsetup stats is separate altogether and requires its own configuration outside of a standard lvmcache entry. The data that comes back from the status command we've listed shows just the necessary data for the particular cache-disk.

https://www.kernel.org/doc/Documentation/device-mapper/cache.txt

sebito91 · 2017-04-05T03:15:01Z

plugins/inputs/dmcache/README.md

+[[inputs.dmcache]]
+  ## Whether to report per-device stats or not
+  per_device = true
+```


All of the fields are basically taken from the output of the dmsetup status --target=cache output for the given disks. In this case, we want to split all the data into multiple lines per disk we set up.

sebito91 · 2017-04-05T03:15:51Z

plugins/inputs/dmcache/dmcache.go

+	return "Provide a native collection for dmsetup based statistics for dm-cache"
+}
+
+func dmSetupStatus() ([]string, error) {


This is fine, how does one accomplish this on Windows (not a Windows user/fan).

sebito91 · 2017-04-05T03:17:59Z

plugins/inputs/dmcache/dmcache_test.go

+		"device": "cs-1",
+	}
+	fields1 := map[string]interface{}{
+		"device":             "cs-1",


that is valid, in the actual output it's not a field but just a tag.

sebito91 · 2017-04-05T03:22:45Z

plugins/inputs/dmcache/dmcache_linux.go

+	if err != nil {
+		return nil, err
+	}
+


Not sure what you're asking here, are you questioning why we're collecting the "dirty" blocks?

sebito91 · 2017-04-05T03:26:49Z

Here is some output to (hopefully) provide some clarity...

[root@hostname ~]# telegraf --test --config /etc/telegraf/telegraf.conf --input-filter dmcache
* Plugin: inputs.dmcache, Collection 1
> dmcache,bu=linux,cls=server,dc=colo,device=vg02-splunk_data_lv,env=production,host=hostname,sr=splunk_indexer,trd=false cache_free=0i,cache_used=1099511627776i,demotions=3265223720960i,dirty=0i,
metadata_free=1060880384i,metadata_used=12861440i,promotions=3265223720960i,read_hits=995134034411520i,read_misses=916807089127424i,write_hits=195107267543040i,write_misses=563725346013184i 1491362011000000000
[root@hostname ~]# dmsetup status --target=cache
vg02-splunk_data_lv: 0 70301769728 cache 8 3140/262144 2048 1048576/1048576 949033802 874335375 186072328 537610402 3113960 3113960 0 1 writethrough 2 migration_threshold 2048000 smq 0 rw -

sebito91 · 2017-04-05T11:50:04Z

@danielnelson, @sparrc we've made the proposed changes if you can please give it another look?

danielnelson · 2017-04-05T19:06:15Z

plugins/inputs/dmcache/README.md

+```
+$ ./telegraf --test --config /etc/telegraf/telegraf.conf --input-filter dmcache
+* Plugin: inputs.dmcache, Collection 1
+> dmcache,bu=linux,cls=server,dc=colo,device=vg02-splunk_data_lv,env=production,host=hostname,sr=splunk_indexer,trd=false cache_free=0i,cache_used=1099511627776i,demotions=3265223720960i,dirty=0i,metadata_free=1060880384i,metadata_used=12861440i,promotions=3265223720960i,read_hits=995134034411520i,read_misses=916807089127424i,write_hits=195107267543040i,write_misses=563725346013184i 1491362011000000000


Can you clean out the custom tags you have?

danielnelson · 2017-04-05T19:10:38Z

plugins/inputs/dmcache/dmcache_linux.go

+			tags := map[string]string{"device": status.device}
+			acc.AddFields(metricName, fields, tags)
+		}
+		aggregateStats(total, fields)


Switch to fields at the last moment. Overall flow should be get the cacheStatus, aggregate, then convert to fields and emit as needed.

gg

danielnelson

Just a minor style thing in the tests and then add this to the CHANGELOG.md.

danielnelson · 2017-04-06T20:23:02Z

plugins/inputs/dmcache/dmcache_test.go

+	getCurrentStatus: output2Devices,
+}
+
+func TestDMCacheStats_1(t *testing.T) {


Can you move the DMCache initialization into the function and rename the tests like TestWhateverThisTests.

danielnelson · 2017-04-07T18:33:03Z

I just noticed this, can you get it?
plugins/inputs/dmcache/dmcache.go:30: undefined: dmSetupStatus

vlasad · 2017-04-07T19:58:22Z

I cannot get it.
dmSetupStatus is defined in plugins/inputs/dmcache/dmcache_linux.go:177

danielnelson · 2017-04-07T20:51:08Z

On the non linux builds.

dswarbrick · 2017-04-20T14:22:32Z

I was also working on a dmcache plugin for Telegraf, but put it aside whilst waiting for another plugin I wrote to be accepted, during what seems to have been quite a long moratorium on accepting new plugins.

I took a slightly different approach, and implemented the ioctls so that it does not require os.Exec(). However, it does mean that the telegraf process must be able to open /dev/mapper/control with O_RDWR. If people are interested, I could merge my approach into this plugin. See my unanswered comment last November in #1581.

danielnelson · 2017-04-20T18:07:04Z

@dswarbrick I'll reply over on the issue.

Add dmcache input plugin

67952f1

jwilder added the feature request Requests for new plugin and for new features to existing plugins label Sep 1, 2016

sparrc added this to the 1.3.0 milestone Jan 21, 2017

sparrc previously requested changes Jan 24, 2017

View reviewed changes

sparrc added the plugin request label Jan 25, 2017

dmcache refactoring

7143b60

danielnelson suggested changes Mar 31, 2017

View reviewed changes

vlasad added 2 commits April 3, 2017 16:02

refactoring and new tests

f5e762f

removed synthesized values

e5943bb

danielnelson suggested changes Apr 5, 2017

View reviewed changes

sebito91 reviewed Apr 5, 2017

View reviewed changes

vlasad added 2 commits April 5, 2017 11:25

updated readme

5eadca5

refactored map to struct

dc4c419

danielnelson reviewed Apr 5, 2017

View reviewed changes

refactoring: switching to fields at the last moment

b8d6d96

danielnelson reviewed Apr 6, 2017

View reviewed changes

vlasad added 7 commits April 7, 2017 11:49

tests refactoring

039d558

Update CHANGELOG.md

2091671

Add dmcache input plugin

edd631a

dmcache refactoring

525fc05

refactoring and new tests

c7a7ab6

removed synthesized values

b696320

updated readme

9044638

vlasad added 4 commits April 7, 2017 13:20

refactored map to struct

6ab3f04

refactoring: switching to fields at the last moment

95e01e0

tests refactoring

7981757

Merge remote-tracking branch 'origin/master'

0fc7149

added stub dmSetupStatus func for not linux build

bc220fb

danielnelson merged commit aa722fa into influxdata:master Apr 7, 2017

sebito91 mentioned this pull request Apr 22, 2017

dm-cache plugin #1581

Closed

vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017

Add dmcache input plugin (influxdata#1667)

a1819ac

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add dmcache input plugin (#1667)

a92ec65

Add dmcache input plugin #1667

Add dmcache input plugin #1667

Conversation

vlasad commented Aug 24, 2016 • edited Loading

Required for all PRs:

sebito91 commented Jan 20, 2017

sparrc commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlasad commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlasad Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebito91 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebito91 commented Apr 5, 2017

sebito91 commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson commented Apr 7, 2017

vlasad commented Apr 7, 2017

danielnelson commented Apr 7, 2017

dswarbrick commented Apr 20, 2017 • edited Loading

danielnelson commented Apr 20, 2017

vlasad commented Aug 24, 2016 •

edited

Loading

vlasad Apr 3, 2017 •

edited

Loading

sebito91 left a comment •

edited

Loading

dswarbrick commented Apr 20, 2017 •

edited

Loading