filesystems monitor: Strip mountpoint dim properly (#1190)

benkeith-splunk · keitwb · commit 52b2ecdd366f · 2020-02-28T11:08:47.000-05:00
If there is a hostFSPath configured and the mountpoint is the root of that path,
insert a / after the stripping out of the hostFSPath.

Also add doc about migrating from collectd/df.
diff --git a/docs/monitors/filesystems.md b/docs/monitors/filesystems.md
@@ -21,6 +21,29 @@ procPath: /hostfs/proc
 monitors:
  - type: filesystems
    hostFSPath: /hostfs
+
+## Migrating from collectd/df
+The `collectd/df` monitor is being deprecated in favor of the `filesystems`
+monitor.  While the `collectd/df` monitor will still be available in
+5.0, it is recommended that you switch to the `filesystems` monitor soon
+after upgrading.  There are a few incompatibilities to be aware of between
+the two monitors:
+
+ - `collectd/df` used a dimension called `plugin_instance` to identify the
+   mount point or device of the filesystem.  This dimension is completely
+   removed in the `filesystems` monitor and replaced by the `mountpoint`
+   and `device` dimensions.  You no longer have to select between the two
+   (the `reportByDevice` option on `collectd/df`) as both are always
+   reported.
+
+ - The mountpoints in the `plugin_instance` dimension of `collectd/df`
+   were reported with `-` instead of the more conventional `/` separated
+   path segments.  The `filesystems` monitor always reports mountpoints in
+   the `mountpoint` dimension and uses the conventional `/` separator.
+
+ - The `collectd/df` plugin set a dimension `plugin: df` on all datapoints,
+   but `filesystems` has no such comparable dimension.
+
 ```
 
 
diff --git a/pkg/monitors/filesystems/filesystems.go b/pkg/monitors/filesystems/filesystems.go
@@ -69,6 +69,9 @@ func (m *Monitor) getCommonDimensions(partition *gopsutil.PartitionStat) map[str
 	// sanitize hostfs path in mountpoint
 	if m.hostFSPath != "" {
 		dims["mountpoint"] = strings.Replace(dims["mountpoint"], m.hostFSPath, "", 1)
+		if dims["mountpoint"] == "" {
+			dims["mountpoint"] = "/"
+		}
 	}
 
 	return dims
diff --git a/pkg/monitors/filesystems/filesystems_test.go b/pkg/monitors/filesystems/filesystems_test.go
@@ -0,0 +1,66 @@
+package filesystems
+
+import (
+	"testing"
+
+	gopsutil "github.com/shirou/gopsutil/disk"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCommonDimensions(t *testing.T) {
+	cases := []struct {
+		hostFSPath   string
+		ps           *gopsutil.PartitionStat
+		expectedDims map[string]string
+	}{
+		{
+			hostFSPath: "/hostfs",
+			ps: &gopsutil.PartitionStat{
+				Device:     "/dev/sdb1",
+				Mountpoint: "/hostfs/var/lib",
+				Fstype:     "ext4",
+			},
+			expectedDims: map[string]string{
+				"mountpoint": "/var/lib",
+				"device":     "/dev/sdb1",
+				"fs_type":    "ext4",
+			},
+		},
+		{
+			hostFSPath: "/hostfs",
+			ps: &gopsutil.PartitionStat{
+				Device:     "/dev/sdb1",
+				Mountpoint: "/hostfs",
+				Fstype:     "ext4",
+			},
+			expectedDims: map[string]string{
+				"mountpoint": "/",
+				"device":     "/dev/sdb1",
+				"fs_type":    "ext4",
+			},
+		},
+		{
+			hostFSPath: "",
+			ps: &gopsutil.PartitionStat{
+				Device:     "/dev/sdb1",
+				Mountpoint: "/",
+				Fstype:     "ext4",
+			},
+			expectedDims: map[string]string{
+				"mountpoint": "/",
+				"device":     "/dev/sdb1",
+				"fs_type":    "ext4",
+			},
+		},
+	}
+
+	for _, tt := range cases {
+		m := Monitor{
+			hostFSPath: tt.hostFSPath,
+		}
+
+		dims := m.getCommonDimensions(tt.ps)
+
+		assert.Equal(t, tt.expectedDims, dims)
+	}
+}
diff --git a/pkg/monitors/filesystems/metadata.yaml b/pkg/monitors/filesystems/metadata.yaml
@@ -12,6 +12,29 @@ monitors:
     monitors:
      - type: filesystems
        hostFSPath: /hostfs
+
+    ## Migrating from collectd/df
+    The `collectd/df` monitor is being deprecated in favor of the `filesystems`
+    monitor.  While the `collectd/df` monitor will still be available in
+    5.0, it is recommended that you switch to the `filesystems` monitor soon
+    after upgrading.  There are a few incompatibilities to be aware of between
+    the two monitors:
+
+     - `collectd/df` used a dimension called `plugin_instance` to identify the
+       mount point or device of the filesystem.  This dimension is completely
+       removed in the `filesystems` monitor and replaced by the `mountpoint`
+       and `device` dimensions.  You no longer have to select between the two
+       (the `reportByDevice` option on `collectd/df`) as both are always
+       reported.
+
+     - The mountpoints in the `plugin_instance` dimension of `collectd/df`
+       were reported with `-` instead of the more conventional `/` separated
+       path segments.  The `filesystems` monitor always reports mountpoints in
+       the `mountpoint` dimension and uses the conventional `/` separator.
+
+     - The `collectd/df` plugin set a dimension `plugin: df` on all datapoints,
+       but `filesystems` has no such comparable dimension.
+
     ```
   metrics:
     df_complex.free:
diff --git a/selfdescribe.json b/selfdescribe.json
@@ -25712,7 +25712,7 @@
       "sendAll": false,
       "sendUnknown": false,
       "dimensions": null,
-      "doc": "This monitor reports metrics about free disk space on mounted devices.\n\nOn Linux hosts, this monitor relies on the `/proc` filesystem.\nIf the underlying host's `/proc` file system is mounted somewhere other than\n/proc please specify the path using the top level configuration `procPath`.\n\n```yaml\nprocPath: /hostfs/proc\nmonitors:\n - type: filesystems\n   hostFSPath: /hostfs\n```\n",
+      "doc": "This monitor reports metrics about free disk space on mounted devices.\n\nOn Linux hosts, this monitor relies on the `/proc` filesystem.\nIf the underlying host's `/proc` file system is mounted somewhere other than\n/proc please specify the path using the top level configuration `procPath`.\n\n```yaml\nprocPath: /hostfs/proc\nmonitors:\n - type: filesystems\n   hostFSPath: /hostfs\n\n## Migrating from collectd/df\nThe `collectd/df` monitor is being deprecated in favor of the `filesystems`\nmonitor.  While the `collectd/df` monitor will still be available in\n5.0, it is recommended that you switch to the `filesystems` monitor soon\nafter upgrading.  There are a few incompatibilities to be aware of between\nthe two monitors:\n\n - `collectd/df` used a dimension called `plugin_instance` to identify the\n   mount point or device of the filesystem.  This dimension is completely\n   removed in the `filesystems` monitor and replaced by the `mountpoint`\n   and `device` dimensions.  You no longer have to select between the two\n   (the `reportByDevice` option on `collectd/df`) as both are always\n   reported.\n\n - The mountpoints in the `plugin_instance` dimension of `collectd/df`\n   were reported with `-` instead of the more conventional `/` separated\n   path segments.  The `filesystems` monitor always reports mountpoints in\n   the `mountpoint` dimension and uses the conventional `/` separator.\n\n - The `collectd/df` plugin set a dimension `plugin: df` on all datapoints,\n   but `filesystems` has no such comparable dimension.\n\n```\n",
       "groups": {
         "": {
           "description": "",

Original file line number	Diff line number	Diff line change
`@@ -69,6 +69,9 @@ func (m Monitor) getCommonDimensions(partition gopsutil.PartitionStat) map[str`
`69`	`69`	`// sanitize hostfs path in mountpoint`
`70`	`70`	`if m.hostFSPath != "" {`
`71`	`71`	`dims["mountpoint"] = strings.Replace(dims["mountpoint"], m.hostFSPath, "", 1)`
	`72`	`+ if dims["mountpoint"] == "" {`
	`73`	`+ dims["mountpoint"] = "/"`
	`74`	`+ }`
`72`	`75`	`}`
`73`	`76`
`74`	`77`	`return dims`