Merge pull request #123 from stefanotorresi/refactor-metrics

Refactor metrics

Author: Stefano Torresi

README.md

@@ -48,7 +48,7 @@ go get github.com/ClusterLabs/ha_cluster_exporter
 ```
 
 ### RPM
-You can find the repositories for RPM based distributions in [SUSE's Open Build Service](https://build.opensuse.org/repositories/server:monitoring/prometheus-ha_cluster_exporter).  
+You can find the repositories for RPM based distributions in [SUSE's Open Build Service](https://build.opensuse.org/package/show/server:monitoring/prometheus-ha_cluster_exporter).  
 On openSUSE or SUSE Linux Enterprise you can just use the `zypper` system package manager:
 ```shell
 export DISTRO=SLE_15_SP1 # change as desired

corosync_metrics.go

@@ -12,29 +12,24 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-var (
-	corosyncMetrics = metricDescriptors{
-		// the map key will function as an identifier of the metric throughout the rest of the code;
-		// it is arbitrary, but by convention we use the actual metric name
-		"quorate":           NewMetricDesc("corosync", "quorate", "Whether or not the cluster is quorate", nil),
-		"ring_errors_total": NewMetricDesc("corosync", "ring_errors_total", "Total number of corosync ring errors", nil),
-		"quorum_votes":      NewMetricDesc("corosync", "quorum_votes", "Cluster quorum votes; one line per type", []string{"type"}),
-	}
-)
-
 func NewCorosyncCollector(cfgToolPath string, quorumToolPath string) (*corosyncCollector, error) {
 	err := CheckExecutables(cfgToolPath, quorumToolPath)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not initialize Corosync collector")
 	}
 
-	return &corosyncCollector{
+	collector := &corosyncCollector{
 		DefaultCollector{
-			metrics: corosyncMetrics,
+			subsystem: "corosync",
 		},
 		cfgToolPath,
 		quorumToolPath,
-	}, nil
+	}
+	collector.setDescriptor("quorate", "Whether or not the cluster is quorate", nil)
+	collector.setDescriptor("ring_errors", "The number of corosync ring errors", nil)
+	collector.setDescriptor("quorum_votes", "Cluster quorum votes; one line per type", []string{"type"})
+
+	return collector, nil
 }
 
 type corosyncCollector struct {
@@ -72,7 +67,7 @@ func (c *corosyncCollector) collectRingErrorsTotal(ch chan<- prometheus.Metric)
 		return errors.Wrap(err, "cannot parse ring status")
 	}
 
-	ch <- c.makeGaugeMetric("ring_errors_total", float64(ringErrorsTotal))
+	ch <- c.makeGaugeMetric("ring_errors", float64(ringErrorsTotal))
 
 	return nil
 }

doc/metrics.md

@@ -25,14 +25,12 @@ The Pacemaker subsystem collects an atomic snapshot of the HA cluster directly f
 
 0. [Sample](../test/pacemaker.metrics)
 1. [`ha_cluster_pacemaker_config_last_change`](#ha_cluster_pacemaker_config_last_change)
-3. [`ha_cluster_pacemaker_fail_count`](#ha_cluster_pacemaker_fail_count)
-2. [`ha_cluster_pacemaker_location_constraints`](#ha_cluster_pacemaker_location_constraints)
+2. [`ha_cluster_pacemaker_fail_count`](#ha_cluster_pacemaker_fail_count)
+3. [`ha_cluster_pacemaker_location_constraints`](#ha_cluster_pacemaker_location_constraints)
 4. [`ha_cluster_pacemaker_migration_threshold`](#ha_cluster_pacemaker_migration_threshold)
-5. [`ha_cluster_pacemaker_nodes_total`](#ha_cluster_pacemaker_nodes_total)
-6. [`ha_cluster_pacemaker_nodes`](#ha_cluster_pacemaker_nodes)
-7. [`ha_cluster_pacemaker_resources_total`](#ha_cluster_pacemaker_resources_total)
-8. [`ha_cluster_pacemaker_resources`](#ha_cluster_pacemaker_resources)
-9. [`ha_cluster_pacemaker_stonith_enabled`](#ha_cluster_pacemaker_stonith_enabled)
+5. [`ha_cluster_pacemaker_nodes`](#ha_cluster_pacemaker_nodes)
+6. [`ha_cluster_pacemaker_resources`](#ha_cluster_pacemaker_resources)
+7. [`ha_cluster_pacemaker_stonith_enabled`](#ha_cluster_pacemaker_stonith_enabled)
 
 
 ### `ha_cluster_pacemaker_config_last_change`
@@ -92,13 +90,6 @@ Either the value is `1`, or the line is absent altogether.
 The total number of lines for this metric will be the cardinality of `name` times the cardinality of `status`.
 
 
-### `ha_cluster_pacemaker_nodes_total` 
-
-#### Description
-
-The total number of *configured* nodes in the cluster. This value is mostly static and *does not* take into account the status of the nodes. It only changes when the Pacemaker configuration changes.
-
-
 ### `ha_cluster_pacemaker_resources` 
 
 #### Description
@@ -117,13 +108,6 @@ Either the value is `1`, or the line is absent altogether.
 The total number of lines for this metric will be the cardinality of `id` times the cardinality of `status`.
 
 
-### `ha_cluster_pacemaker_resources_total` 
-
-#### Description
-
-The total number of *configured* resources in the cluster. This value is mostly static and *does not* take into account the status of the resources. It only changes when the Pacemaker configuration changes.
-
-
 ### `ha_cluster_pacemaker_stonith_enabled`
 
 #### Description
@@ -139,7 +123,7 @@ The Corosync subsystem collects cluster quorum votes and ring status by parsing
 0. [Sample](../test/corosync.metrics)
 1. [`ha_cluster_corosync_quorate`](#ha_cluster_corosync_quorate)
 2. [`ha_cluster_corosync_quorum_votes`](#ha_cluster_corosync_quorum_votes)
-3. [`ha_cluster_corosync_ring_errors_total`](#ha_cluster_corosync_ring_errors_total)
+3. [`ha_cluster_corosync_ring_errors`](#ha_cluster_corosync_ring_errors)
 
 
 ### `ha_cluster_corosync_quorate`
@@ -161,44 +145,35 @@ Cluster quorum votes; one line per type.
 - `type`: one of `expected_votes|highest_expected|total_votes|quorum`
 
 
-### `ha_cluster_corosync_ring_errors_total`
+### `ha_cluster_corosync_ring_errors`
 
 #### Description
 
-Total number of corosync ring errors.
+The number of corosync ring errors.
 
 
 ## SBD
 
-The SBD subsystems collect devices stats by parsing its configuration the output of `sbd --dump`.
+The SBD subsystems collect devices stats by parsing its configuration and the output of `sbd --dump`.
 
 0. [Sample](../test/sbd.metrics)
-1. [`ha_cluster_sbd_device_status`](#ha_cluster_sbd_device_status)
-2. [`ha_cluster_sbd_devices_total`](#ha_cluster_sbd_devices_total)
+2. [`ha_cluster_sbd_devices`](#ha_cluster_sbd_devices)
 
-
-### `ha_cluster_sbd_device_status`
+### `ha_cluster_sbd_devices`
 
 #### Description
 
-Whether or not an SBD device is healthy. One line per `device`.  
-Value is either `1` or `0`.
+The SBD devices in the cluster; one line per device.  
+Either the value is `1`, or the line is absent altogether.
 
 #### Labels
 
-- `device`: the path of the device.
+- `device`: the path of the SBD device
+- `status`: one of `healthy|unhealthy`
 
 The total number of lines for this metric will be the cardinality of `device`.
 
 
-### `ha_cluster_sbd_devices_total`
-
-#### Description
-
-Total count of configured SBD devices.  
-Value is an integer greater than or equal to `0`.
-
-
 ## DRBD
 
 The DRBD subsystems collect devices stats by parsing its configuration the JSON output of `drbdsetup`.

drbd_metrics.go

@@ -43,41 +43,37 @@ type drbdStatus struct {
 	} `json:"connections"`
 }
 
-var (
-	drbdMetrics = metricDescriptors{
-		// the map key will function as an identifier of the metric throughout the rest of the code;
-		// it is arbitrary, but by convention we use the actual metric name
-		"resources":            NewMetricDesc("drbd", "resources", "The DRBD resources; 1 line per name, per volume", []string{"resource", "role", "volume", "disk_state"}),
-		"written":              NewMetricDesc("drbd", "written", "KiB written to DRBD; 1 line per res, per volume", []string{"resource", "volume"}),
-		"read":                 NewMetricDesc("drbd", "read", "KiB read from DRBD; 1 line per res, per volume", []string{"resource", "volume"}),
-		"al_writes":            NewMetricDesc("drbd", "al_writes", "Writes to activity log; 1 line per res, per volume", []string{"resource", "volume"}),
-		"bm_writes":            NewMetricDesc("drbd", "bm_writes", "Writes to bitmap; 1 line per res, per volume", []string{"resource", "volume"}),
-		"upper_pending":        NewMetricDesc("drbd", "upper_pending", "Upper pending; 1 line per res, per volume", []string{"resource", "volume"}),
-		"lower_pending":        NewMetricDesc("drbd", "lower_pending", "Lower pending; 1 line per res, per volume", []string{"resource", "volume"}),
-		"quorum":               NewMetricDesc("drbd", "quorum", "Quorum status per resource and per volume", []string{"resource", "volume"}),
-		"connections":          NewMetricDesc("drbd", "connections", "The DRBD resource connections; 1 line per per resource, per peer_node_id", []string{"resource", "peer_node_id", "peer_role", "volume", "peer_disk_state"}),
-		"connections_sync":     NewMetricDesc("drbd", "connections_sync", "The in sync percentage value for DRBD resource connections", []string{"resource", "peer_node_id", "volume"}),
-		"connections_received": NewMetricDesc("drbd", "connections_received", "KiB received per connection", []string{"resource", "peer_node_id", "volume"}),
-		"connections_sent":     NewMetricDesc("drbd", "connections_sent", "KiB sent per connection", []string{"resource", "peer_node_id", "volume"}),
-		"connections_pending":  NewMetricDesc("drbd", "connections_pending", "Pending value per connection", []string{"resource", "peer_node_id", "volume"}),
-		"connections_unacked":  NewMetricDesc("drbd", "connections_unacked", "Unacked value per connection", []string{"resource", "peer_node_id", "volume"}),
-		"split_brain":          NewMetricDesc("drbd", "split_brain", "Whether a split brain has been detected; 1 line per resource, per volume.", []string{"resource", "volume"}),
-	}
-)
-
 func NewDrbdCollector(drbdSetupPath string, drbdSplitBrainPath string) (*drbdCollector, error) {
 	err := CheckExecutables(drbdSetupPath)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not initialize DRBD collector")
 	}
 
-	return &drbdCollector{
+	collector := &drbdCollector{
 		DefaultCollector{
-			metrics: drbdMetrics,
+			subsystem: "drbd",
 		},
 		drbdSetupPath,
 		drbdSplitBrainPath,
-	}, nil
+	}
+
+	collector.setDescriptor("resources", "The DRBD resources; 1 line per name, per volume", []string{"resource", "role", "volume", "disk_state"})
+	collector.setDescriptor("written", "KiB written to DRBD; 1 line per res, per volume", []string{"resource", "volume"})
+	collector.setDescriptor("read", "KiB read from DRBD; 1 line per res, per volume", []string{"resource", "volume"})
+	collector.setDescriptor("al_writes", "Writes to activity log; 1 line per res, per volume", []string{"resource", "volume"})
+	collector.setDescriptor("bm_writes", "Writes to bitmap; 1 line per res, per volume", []string{"resource", "volume"})
+	collector.setDescriptor("upper_pending", "Upper pending; 1 line per res, per volume", []string{"resource", "volume"})
+	collector.setDescriptor("lower_pending", "Lower pending; 1 line per res, per volume", []string{"resource", "volume"})
+	collector.setDescriptor("quorum", "Quorum status per resource and per volume", []string{"resource", "volume"})
+	collector.setDescriptor("connections", "The DRBD resource connections; 1 line per per resource, per peer_node_id", []string{"resource", "peer_node_id", "peer_role", "volume", "peer_disk_state"})
+	collector.setDescriptor("connections_sync", "The in sync percentage value for DRBD resource connections", []string{"resource", "peer_node_id", "volume"})
+	collector.setDescriptor("connections_received", "KiB received per connection", []string{"resource", "peer_node_id", "volume"})
+	collector.setDescriptor("connections_sent", "KiB sent per connection", []string{"resource", "peer_node_id", "volume"})
+	collector.setDescriptor("connections_pending", "Pending value per connection", []string{"resource", "peer_node_id", "volume"})
+	collector.setDescriptor("connections_unacked", "Unacked value per connection", []string{"resource", "peer_node_id", "volume"})
+	collector.setDescriptor("split_brain", "Whether a split brain has been detected; 1 line per resource, per volume.", []string{"resource", "volume"})
+
+	return collector, nil
 }
 
 type drbdCollector struct {

ha_cluster_exporter.go

@@ -26,49 +26,55 @@ func (SystemClock) Now() time.Time {
 	return time.Now()
 }
 
-type metricDescriptors map[string]*prometheus.Desc
-
 type DefaultCollector struct {
-	metrics metricDescriptors
+	subsystem   string
+	descriptors map[string]*prometheus.Desc
+}
+
+func (c *DefaultCollector) getDescriptor(name string) *prometheus.Desc {
+	desc, ok := c.descriptors[name]
+	if !ok {
+		// we hard panic on this because it's most certainly a coding error
+		panic(errors.Errorf("undeclared metric '%s'", name))
+	}
+	return desc
+}
+
+// Convenience wrapper around prometheus.NewDesc constructor.
+// Stores a metric descriptor with a fully qualified name like `NAMESPACE_subsystem_name`.
+// `name` is the last and most relevant part of the metrics Full Qualified Name;
+// `help` is the message displayed in the HELP line
+// `variableLabels` is a list of labels to declare. Use `nil` to declare no labels.
+func (c *DefaultCollector) setDescriptor(name, help string, variableLabels []string) {
+	if c.descriptors == nil {
+		c.descriptors = make(map[string]*prometheus.Desc)
+	}
+	c.descriptors[name] = prometheus.NewDesc(prometheus.BuildFQName(NAMESPACE, c.subsystem, name), help, variableLabels, nil)
 }
 
 func (c *DefaultCollector) Describe(ch chan<- *prometheus.Desc) {
-	for _, metric := range c.metrics {
-		ch <- metric
+	for _, descriptor := range c.descriptors {
+		ch <- descriptor
 	}
 }
 
-func (c *DefaultCollector) makeGaugeMetric(metricKey string, value float64, labelValues ...string) prometheus.Metric {
-	return c.makeMetric(metricKey, value, prometheus.GaugeValue, labelValues...)
+func (c *DefaultCollector) makeGaugeMetric(name string, value float64, labelValues ...string) prometheus.Metric {
+	return c.makeMetric(name, value, prometheus.GaugeValue, labelValues...)
 }
 
-func (c *DefaultCollector) makeCounterMetric(metricKey string, value float64, labelValues ...string) prometheus.Metric {
-	return c.makeMetric(metricKey, value, prometheus.CounterValue, labelValues...)
+func (c *DefaultCollector) makeCounterMetric(name string, value float64, labelValues ...string) prometheus.Metric {
+	return c.makeMetric(name, value, prometheus.CounterValue, labelValues...)
 }
 
-func (c *DefaultCollector) makeMetric(metricKey string, value float64, valueType prometheus.ValueType, labelValues ...string) prometheus.Metric {
-	desc, ok := c.metrics[metricKey]
-	if !ok {
-		// we hard panic on this because it's most certainly a coding error
-		panic(errors.Errorf("undeclared metric '%s'", metricKey))
-	}
+func (c *DefaultCollector) makeMetric(name string, value float64, valueType prometheus.ValueType, labelValues ...string) prometheus.Metric {
+	desc := c.getDescriptor(name)
 	metric := prometheus.MustNewConstMetric(desc, valueType, value, labelValues...)
 	if config.GetBool("enable-timestamps") {
 		metric = prometheus.NewMetricWithTimestamp(clock.Now(), metric)
 	}
 	return metric
 }
 
-// Convenience wrapper around Prometheus constructors.
-// Produces a metric with name `NAMESPACE_subsystem_name`.
-// `NAMESPACE` is a global project constant;
-// `subsystem` is an arbitrary name used to group related metrics under the same name prefix;
-// `name` is the last and most relevant part of the metrics Full Qualified Name;
-// `variableLabels` is a list of labels to declare. Use `nil` to declare no labels.
-func NewMetricDesc(subsystem, name, help string, variableLabels []string) *prometheus.Desc {
-	return prometheus.NewDesc(prometheus.BuildFQName(NAMESPACE, subsystem, name), help, variableLabels, nil)
-}
-
 // check that all the given paths exist and are executable files
 func CheckExecutables(paths ...string) error {
 	for _, path := range paths {

ha_cluster_exporter_test.go

@@ -37,15 +37,12 @@ func expectMetrics(t *testing.T, c prometheus.Collector, fixture string) {
 }
 
 func TestMetricFactory(t *testing.T) {
-	SUT := &DefaultCollector{
-		metrics: metricDescriptors{
-			"test_metric": NewMetricDesc("test", "metric", "", nil),
-		},
-	}
+	SUT := &DefaultCollector{}
+	SUT.setDescriptor("test_metric", "", nil)
 
 	metric := SUT.makeGaugeMetric("test_metric", 1)
 
-	assert.Equal(t, SUT.metrics["test_metric"], metric.Desc())
+	assert.Equal(t, SUT.getDescriptor("test_metric"), metric.Desc())
 }
 
 func TestMetricFactoryWithTimestamp(t *testing.T) {
@@ -56,11 +53,8 @@ func TestMetricFactoryWithTimestamp(t *testing.T) {
 	}()
 
 	clock = StoppedClock{}
-	SUT := &DefaultCollector{
-		metrics: metricDescriptors{
-			"test_metric": NewMetricDesc("test", "metric", "", nil),
-		},
-	}
+	SUT := &DefaultCollector{}
+	SUT.setDescriptor("test_metric", "", nil)
 
 	metric := SUT.makeGaugeMetric("test_metric", 1)
 	metricDto := &dto.Metric{}