From 52b2ecdd366f2e6e5eeaef4dc28f6667d220ce8f Mon Sep 17 00:00:00 2001 From: Ben Keith Date: Thu, 13 Feb 2020 08:32:05 -0500 Subject: [PATCH] filesystems monitor: Strip mountpoint dim properly (#1190) 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. --- docs/monitors/filesystems.md | 23 +++++++ pkg/monitors/filesystems/filesystems.go | 3 + pkg/monitors/filesystems/filesystems_test.go | 66 ++++++++++++++++++++ pkg/monitors/filesystems/metadata.yaml | 23 +++++++ selfdescribe.json | 2 +- 5 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 pkg/monitors/filesystems/filesystems_test.go diff --git a/docs/monitors/filesystems.md b/docs/monitors/filesystems.md index 0049b90006..35177f7384 100644 --- 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 index 3e1151cef8..1d3c0a2572 100644 --- 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 new file mode 100644 index 0000000000..b2b8538fac --- /dev/null +++ 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 index 9528b41499..499f4f1717 100644 --- 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 index 79cada12f8..94aed818f2 100644 --- 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": "",