Make vmem monitor default on Linux (#1143)

- Remove beta warning for Linux - Remove plugin dimension - Clean up memory monitor - Also making retry on EBADF more robust against wrapped exceptions
signalfx · Feb 28, 2020 · 7b49a07 · 7b49a07
1 parent 6aafbe1
commit 7b49a07
Show file tree

Hide file tree

Showing 23 changed files with 91 additions and 88 deletions.
diff --git a/deployments/ansible/README.md b/deployments/ansible/README.md
@@ -40,7 +40,7 @@ This role sources the following variables:
         - type: net-io
         - type: load
         - type: memory
-        - type: collectd/vmem
+        - type: vmem
         - type: collectd/signalfx-metadata
           omitProcessInfo: true
         - type: host-metadata

diff --git a/deployments/ansible/example-config.yml b/deployments/ansible/example-config.yml
@@ -13,4 +13,4 @@ sfx_agent_config:
       omitProcessInfo: true
     - type: host-metadata
     - type: processlist
-    - type: collectd/vmem
+    - type: vmem
diff --git a/deployments/chef/README.md b/deployments/chef/README.md
@@ -56,7 +56,7 @@ node['signalfx_agent']['conf'] = {
     {type: "load"},
     {type: "memory"},
     {"type": "collectd/signalfx-metadata", "omitProcessInfo": true},
-    {type: "collectd/vmem"}
+    {type: "vmem"}
     {type: "host-metadata"},
     {type: "processlist"},
   ],

diff --git a/deployments/chef/example_attrs.json b/deployments/chef/example_attrs.json
@@ -15,7 +15,7 @@
       {"type": "load"},
       {"type": "memory"},
       {"type": "collectd/signalfx-metadata", "omitProcessInfo": true},
-      {"type": "collectd/vmem"},
+      {"type": "vmem"},
       {"type": "host-metadata"}
       {"type": "processlist"}
     ],

diff --git a/deployments/docker/agent.yaml b/deployments/docker/agent.yaml
@@ -29,7 +29,7 @@ monitors:
   - type: net-io
   - type: load
   - type: memory
-  - type: collectd/vmem
+  - type: vmem
   - type: collectd/signalfx-metadata
     omitProcessInfo: true
   - type: host-metadata

diff --git a/deployments/ecs/agent.yaml b/deployments/ecs/agent.yaml
@@ -47,7 +47,7 @@ monitors:
     omitProcessInfo: true
   - type: host-metadata
   - type: processlist
-  - type: collectd/vmem
+  - type: vmem
 
   - type: docker-container-stats
     labelsToDimensions:

diff --git a/deployments/fargate/agent.yaml b/deployments/fargate/agent.yaml
@@ -26,7 +26,7 @@ monitors:
   - type: collectd/protocols
   - type: collectd/signalfx-metadata
     omitProcessInfo: true
-  - type: collectd/vmem
+  - type: vmem
 
   - type: ecs-metadata
     excludedImages:

diff --git a/deployments/k8s/configmap.yaml b/deployments/k8s/configmap.yaml
@@ -44,7 +44,7 @@ data:
     - type: host-metadata
     - type: processlist
     - type: collectd/uptime
-    - type: collectd/vmem
+    - type: vmem
 
     - type: kubelet-stats
       kubeletAPI:

diff --git a/deployments/k8s/helm/signalfx-agent/templates/configmap.yaml b/deployments/k8s/helm/signalfx-agent/templates/configmap.yaml
@@ -68,7 +68,7 @@ data:
     - type: host-metadata
     - type: processlist
     - type: collectd/uptime
-    - type: collectd/vmem
+    - type: vmem
 
     - type: kubelet-stats
       {{- if .Values.containerStatsIntervalSeconds }}

diff --git a/deployments/puppet/README.md b/deployments/puppet/README.md
@@ -29,7 +29,7 @@ class accepts the following parameters:
         {type: "host-metadata"},
         {type: "processlist"},
         {type: "collectd/uptime"},
-        {type: "collectd/vmem"}
+        {type: "vmem"}
       ]
     }
     ```

diff --git a/deployments/salt/README.md b/deployments/salt/README.md
@@ -51,7 +51,7 @@ signalfx-agent:
       - type: net-io
       - type: load
       - type: memory
-      - type: collectd/vmem
+      - type: vmem
       - type: collectd/signalfx-metadata
         omitProcessInfo: true
       - type: host-metadata

diff --git a/deployments/salt/pillar.example b/deployments/salt/pillar.example
@@ -15,5 +15,5 @@ signalfx-agent:
       - type: memory
       - type: collectd/signalfx-metadata
         omitProcessInfo: true
-      - type: collectd/vmem
+      - type: vmem
       - type: processlist
diff --git a/docs/monitors/collectd-vmem.md b/docs/monitors/collectd-vmem.md
@@ -15,6 +15,10 @@ subsystem of the kernel using the [collectd vmem
 plugin](https://collectd.org/wiki/index.php/Plugin:vmem).  There is no
 configuration available for this plugin.
 
+**This monitor is deprecated in favor of the `vmem` monitor.  The metrics
+should be fully compatible with this monitor.** This monitor will be
+removed in a future agent release.
+
 
 ## Configuration
 

diff --git a/docs/monitors/vmem.md b/docs/monitors/vmem.md
@@ -10,8 +10,8 @@ Monitor Type: `vmem` ([Source](https://github.com/signalfx/signalfx-agent/tree/m
 
 ## Overview
 
-Collects information about the virtual memory
-subsystem of the kernel.
+Collects information specific to the virtual memory subsystem of the
+kernel.  For general memory statistics, see the [memory monitor](./memory.md).
 
 On Linux hosts, this monitor relies on the `/proc` filesystem.
 If the underlying host's `/proc` file system is mounted somewhere other than

diff --git a/packaging/etc/agent.yaml b/packaging/etc/agent.yaml
@@ -28,6 +28,6 @@ monitors:
   - type: memory
   - type: collectd/signalfx-metadata
     omitProcessInfo: true
-  - type: collectd/vmem
+  - type: vmem
 
 enableBuiltInFiltering: true
diff --git a/pkg/monitors/collectd/vmem/metadata.yaml b/pkg/monitors/collectd/vmem/metadata.yaml
@@ -5,6 +5,10 @@ monitors:
     subsystem of the kernel using the [collectd vmem
     plugin](https://collectd.org/wiki/index.php/Plugin:vmem).  There is no
     configuration available for this plugin.
+
+    **This monitor is deprecated in favor of the `vmem` monitor.  The metrics
+    should be fully compatible with this monitor.** This monitor will be
+    removed in a future agent release.
   metrics:
     vmpage_faults.majflt:
       description: Number of major page faults on the system

diff --git a/pkg/monitors/memory/memory.go b/pkg/monitors/memory/memory.go
@@ -2,7 +2,6 @@ package memory
 
 import (
 	"context"
-	"runtime"
 	"time"
 
 	"github.com/shirou/gopsutil/mem"
@@ -15,8 +14,6 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-const windowsOS = "windows"
-
 func init() {
 	monitors.Register(&monitorMetadata, func() interface{} { return &Monitor{} }, &Config{})
 }
@@ -33,73 +30,22 @@ type Monitor struct {
 	logger logrus.FieldLogger
 }
 
-func (m *Monitor) makeDatapointsWindows(memInfo *mem.VirtualMemoryStat, dimensions map[string]string) []*datapoint.Datapoint {
-	return []*datapoint.Datapoint{
-		datapoint.New("memory.available", dimensions, datapoint.NewIntValue(int64(memInfo.Available)), datapoint.Gauge, time.Time{}),
-	}
-}
-
-func (m *Monitor) makeDatapointsNotWindows(memInfo *mem.VirtualMemoryStat, dimensions map[string]string) []*datapoint.Datapoint {
-	return []*datapoint.Datapoint{
-		datapoint.New("memory.free", dimensions, datapoint.NewIntValue(int64(memInfo.Free)), datapoint.Gauge, time.Time{}),
-	}
-}
-
-func (m *Monitor) makeDatapointsDarwin(memInfo *mem.VirtualMemoryStat, dimensions map[string]string) []*datapoint.Datapoint {
-	return []*datapoint.Datapoint{
-		datapoint.New("memory.active", dimensions, datapoint.NewIntValue(int64(memInfo.Active)), datapoint.Gauge, time.Time{}),
-		datapoint.New("memory.inactive", dimensions, datapoint.NewIntValue(int64(memInfo.Inactive)), datapoint.Gauge, time.Time{}),
-		datapoint.New("memory.wired", dimensions, datapoint.NewIntValue(int64(memInfo.Wired)), datapoint.Gauge, time.Time{}),
-	}
-}
-
-func (m *Monitor) makeDatapointsLinux(memInfo *mem.VirtualMemoryStat, dimensions map[string]string) []*datapoint.Datapoint {
-	return []*datapoint.Datapoint{
-		datapoint.New("memory.buffered", dimensions, datapoint.NewIntValue(int64(memInfo.Buffers)), datapoint.Gauge, time.Time{}),
-		// for some reason gopsutil decided to add slab_reclaimable to cached which collectd does not
-		datapoint.New("memory.cached", dimensions, datapoint.NewIntValue(int64(memInfo.Cached-memInfo.SReclaimable)), datapoint.Gauge, time.Time{}),
-		datapoint.New("memory.slab_recl", dimensions, datapoint.NewIntValue(int64(memInfo.SReclaimable)), datapoint.Gauge, time.Time{}),
-		datapoint.New("memory.slab_unrecl", dimensions, datapoint.NewIntValue(int64(memInfo.Slab-memInfo.SReclaimable)), datapoint.Gauge, time.Time{}),
-	}
-}
-
 // EmitDatapoints emits a set of memory datapoints
 func (m *Monitor) emitDatapoints() {
 	// mem.VirtualMemory is a gopsutil function
 	memInfo, err := mem.VirtualMemory()
 	if err != nil {
-		if err == context.DeadlineExceeded {
-			m.logger.WithField("debug", err).Debugf("unable to collect memory time info")
-		} else {
-			m.logger.WithError(err).Errorf("unable to collect memory time info")
-		}
+		m.logger.WithError(err).Errorf("Unable to collect memory stats")
 		return
 	}
 
 	// all platforms
-	dps := []*datapoint.Datapoint{datapoint.New("memory.utilization", nil, datapoint.NewFloatValue(memInfo.UsedPercent), datapoint.Gauge, time.Time{}),
+	dps := []*datapoint.Datapoint{
+		datapoint.New("memory.utilization", nil, datapoint.NewFloatValue(memInfo.UsedPercent), datapoint.Gauge, time.Time{}),
 		datapoint.New("memory.used", nil, datapoint.NewIntValue(int64(memInfo.Used)), datapoint.Gauge, time.Time{}),
 	}
 
-	// windows only
-	if runtime.GOOS == windowsOS {
-		dps = append(dps, m.makeDatapointsWindows(memInfo, nil)...)
-	}
-
-	// linux + darwin only
-	if runtime.GOOS != windowsOS {
-		dps = append(dps, m.makeDatapointsNotWindows(memInfo, nil)...)
-	}
-
-	// darwin only
-	if runtime.GOOS == "darwin" {
-		dps = append(dps, m.makeDatapointsDarwin(memInfo, nil)...)
-	}
-
-	// linux only
-	if runtime.GOOS == "linux" {
-		dps = append(dps, m.makeDatapointsLinux(memInfo, nil)...)
-	}
+	dps = append(dps, m.makeMemoryDatapoints(memInfo, nil)...)
 
 	m.Output.SendDatapoints(dps...)
 }

diff --git a/pkg/monitors/memory/memory_darwin.go b/pkg/monitors/memory/memory_darwin.go
@@ -0,0 +1,17 @@
+package memory
+
+import (
+	"time"
+
+	"github.com/shirou/gopsutil/mem"
+	"github.com/signalfx/golib/v3/datapoint"
+)
+
+func (m *Monitor) makeMemoryDatapoints(memInfo *mem.VirtualMemoryStat, dimensions map[string]string) []*datapoint.Datapoint {
+	return []*datapoint.Datapoint{
+		datapoint.New("memory.active", dimensions, datapoint.NewIntValue(int64(memInfo.Active)), datapoint.Gauge, time.Time{}),
+		datapoint.New("memory.inactive", dimensions, datapoint.NewIntValue(int64(memInfo.Inactive)), datapoint.Gauge, time.Time{}),
+		datapoint.New("memory.wired", dimensions, datapoint.NewIntValue(int64(memInfo.Wired)), datapoint.Gauge, time.Time{}),
+		datapoint.New("memory.free", dimensions, datapoint.NewIntValue(int64(memInfo.Free)), datapoint.Gauge, time.Time{}),
+	}
+}
diff --git a/pkg/monitors/memory/memory_linux.go b/pkg/monitors/memory/memory_linux.go
@@ -0,0 +1,19 @@
+package memory
+
+import (
+	"time"
+
+	"github.com/shirou/gopsutil/mem"
+	"github.com/signalfx/golib/v3/datapoint"
+)
+
+func (m *Monitor) makeMemoryDatapoints(memInfo *mem.VirtualMemoryStat, dimensions map[string]string) []*datapoint.Datapoint {
+	return []*datapoint.Datapoint{
+		datapoint.New("memory.buffered", dimensions, datapoint.NewIntValue(int64(memInfo.Buffers)), datapoint.Gauge, time.Time{}),
+		// for some reason gopsutil decided to add slab_reclaimable to cached which collectd does not
+		datapoint.New("memory.cached", dimensions, datapoint.NewIntValue(int64(memInfo.Cached-memInfo.SReclaimable)), datapoint.Gauge, time.Time{}),
+		datapoint.New("memory.slab_recl", dimensions, datapoint.NewIntValue(int64(memInfo.SReclaimable)), datapoint.Gauge, time.Time{}),
+		datapoint.New("memory.slab_unrecl", dimensions, datapoint.NewIntValue(int64(memInfo.Slab-memInfo.SReclaimable)), datapoint.Gauge, time.Time{}),
+		datapoint.New("memory.free", dimensions, datapoint.NewIntValue(int64(memInfo.Free)), datapoint.Gauge, time.Time{}),
+	}
+}
diff --git a/pkg/monitors/memory/memory_windows.go b/pkg/monitors/memory/memory_windows.go
@@ -0,0 +1,14 @@
+package memory
+
+import (
+	"time"
+
+	"github.com/shirou/gopsutil/mem"
+	"github.com/signalfx/golib/v3/datapoint"
+)
+
+func (m *Monitor) makeMemoryDatapoints(memInfo *mem.VirtualMemoryStat, dimensions map[string]string) []*datapoint.Datapoint {
+	return []*datapoint.Datapoint{
+		datapoint.New("memory.available", dimensions, datapoint.NewIntValue(int64(memInfo.Available)), datapoint.Gauge, time.Time{}),
+	}
+}
diff --git a/pkg/monitors/vmem/metadata.yaml b/pkg/monitors/vmem/metadata.yaml
@@ -1,8 +1,8 @@
 monitors:
 - dimensions:
   doc: |
-    Collects information about the virtual memory
-    subsystem of the kernel.
+    Collects information specific to the virtual memory subsystem of the
+    kernel.  For general memory statistics, see the [memory monitor](./memory.md).
 
     On Linux hosts, this monitor relies on the `/proc` filesystem.
     If the underlying host's `/proc` file system is mounted somewhere other than

diff --git a/pkg/monitors/vmem/vmem_linux.go b/pkg/monitors/vmem/vmem_linux.go
@@ -17,18 +17,18 @@ import (
 )
 
 var cumulativeCounters = map[string]string{
-	"pgpgin":     "vmpage_io.memory.in",
-	"pgpgout":    "vmpage_io.memory.out",
-	"pswpin":     "vmpage_io.swap.in",
-	"pswpout":    "vmpage_io.swap.out",
-	"pgmajfault": "vmpage_faults.majflt",
-	"pgfault":    "vmpage_faults.minflt",
+	"pgpgin":     vmpageIoMemoryIn,
+	"pgpgout":    vmpageIoMemoryOut,
+	"pswpin":     vmpageIoSwapIn,
+	"pswpout":    vmpageIoSwapOut,
+	"pgmajfault": vmpageFaultsMajflt,
+	"pgfault":    vmpageFaultsMinflt,
 }
 
 var gauges = map[string]string{
-	"nr_free_pages":      "vmpage_number.free_pages",
-	"nr_mapped":          "vmpage_number.mapped",
-	"nr_shmem_pmdmapped": "vmpage_number.shmem_pmdmapped",
+	"nr_free_pages":      vmpageNumberFreePages,
+	"nr_mapped":          vmpageNumberMapped,
+	"nr_shmem_pmdmapped": vmpageNumberShmemPmdmapped,
 }
 
 func (m *Monitor) parseFileForDatapoints(contents []byte) []*datapoint.Datapoint {
@@ -54,7 +54,7 @@ func (m *Monitor) parseFileForDatapoints(contents []byte) []*datapoint.Datapoint
 					m.logger.Errorf("failed to parse value for metric %s", metricName)
 					continue
 				}
-				dps = append(dps, datapoint.New(metricName, map[string]string{"plugin": monitorType}, datapoint.NewIntValue(val), metricType, time.Time{}))
+				dps = append(dps, datapoint.New(metricName, nil, datapoint.NewIntValue(val), metricType, time.Time{}))
 			}
 		}
 	}
@@ -65,7 +65,6 @@ func (m *Monitor) parseFileForDatapoints(contents []byte) []*datapoint.Datapoint
 // Configure and run the monitor on linux
 func (m *Monitor) Configure(conf *Config) (err error) {
 	m.logger = logrus.WithField("monitorType", monitorType)
-	m.logger.Warningf("'%s' monitor is in beta on this platform.  For production environments please use 'collectd/%s'.", monitorType, monitorType)
 
 	// create contexts for managing the the plugin loop
 	var ctx context.Context

diff --git a/selfdescribe.json b/selfdescribe.json
@@ -19353,7 +19353,7 @@
       "monitorType": "collectd/vmem",
       "sendAll": false,
       "dimensions": null,
-      "doc": "Collects information about the virtual memory\nsubsystem of the kernel using the [collectd vmem\nplugin](https://collectd.org/wiki/index.php/Plugin:vmem).  There is no\nconfiguration available for this plugin.\n",
+      "doc": "Collects information about the virtual memory\nsubsystem of the kernel using the [collectd vmem\nplugin](https://collectd.org/wiki/index.php/Plugin:vmem).  There is no\nconfiguration available for this plugin.\n\n**This monitor is deprecated in favor of the `vmem` monitor.  The metrics\nshould be fully compatible with this monitor.** This monitor will be\nremoved in a future agent release.\n",
       "groups": {
         "": {
           "description": "",
@@ -48105,7 +48105,7 @@
       "monitorType": "vmem",
       "sendAll": false,
       "dimensions": null,
-      "doc": "Collects information about the virtual memory\nsubsystem of the kernel.\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: /proc\nmonitors:\n - type: vmem\n```\n",
+      "doc": "Collects information specific to the virtual memory subsystem of the\nkernel.  For general memory statistics, see the [memory monitor](./memory.md).\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: /proc\nmonitors:\n - type: vmem\n```\n",
       "groups": {
         "": {
           "description": "",