switch logging and flag parsing to prometheus best practice packages

(switch logrus to promlog, switch pflag to kingpin)
ClusterLabs · May 23, 2022 · a38c72a · a38c72a
1 parent 6dff7ff
commit a38c72a
Show file tree

Hide file tree

Showing 20 changed files with 433 additions and 289 deletions.
diff --git a/README.md b/README.md
@@ -100,6 +100,37 @@ Please refer to the example [YAML configuration](ha_cluster_exporter.yaml) for m
 
 Additional CLI flags can also be passed via `/etc/sysconfig/prometheus-ha_cluster_exporter`.
 
+#### General Flags
+
+Name                                       | Description
+----                                       | -----------
+web.listen-address                         | Address to listen on for web interface and telemetry.
+web.telemetry-path                         | Path under which to expose metrics.
+web.config.file                            | Path to a [web configuration file](#tls-and-basic-authentication)
+log.level                                  | Logging verbosity (default: info)
+version                                    | Print the version information.
+
+##### Deprecated Flags
+Name                                       | Description
+----                                       | -----------
+address                                    | deprecated: please use --web.listen-address or --web.config.file to use Prometheus Exporter Toolkit
+port                                       | deprecated: please use --web.listen-address or --web.config.file to use Prometheus Exporter Toolkit
+log-level                                  | deprecated: please use log.level
+enable-timestamps                          | deprecated: server-side metric timestamping is discouraged by Prometheus best-practices and should be avoided
+
+#### Collector Flags
+
+Name                                       | Description
+----                                       | -----------
+crm-mon-path                               | path to crm_mon executable (default `/usr/sbin/crm_mon`)
+cibadmin-path                              | path to cibadmin executable (default `/usr/sbin/cibadmin`)
+corosync-cfgtoolpath-path                  | path to corosync-cfgtool executable (default `/usr/sbin/corosync-cfgtool`)
+corosync-quorumtool-path                   | path to corosync-quorumtool executable (default `/usr/sbin/corosync-quorumtool`)
+sbd-path                                   | path to sbd executable (default `/usr/sbin/sbd`)
+sbd-config-path                            | path to sbd configuration (default `/etc/sysconfig/sbd`)
+drbdsetup-path                             | path to drbdsetup executable (default `/sbin/drbdsetup`)
+drbdsplitbrain-path                        | path to drbd splitbrain hooks temporary files (default `/var/run/drbd/splitbrain`)
+
 ### systemd integration
 
 A [systemd unit file](ha_cluster_exporter.service) is provided with the RPM packages. You can enable and start it as usual:  
@@ -116,7 +147,7 @@ We recommend having a look at the [design document](doc/design.md) and the [deve
 
 ## License
 
-Copyright 2019-2020 SUSE LLC
+Copyright 2019-2022 SUSE LLC
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

diff --git a/collector/corosync/corosync.go b/collector/corosync/corosync.go
@@ -3,23 +3,24 @@ package corosync
 import (
 	"os/exec"
 
+	"github.com/go-kit/log"
+	"github.com/go-kit/log/level"
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
-	log "github.com/sirupsen/logrus"
 
 	"github.com/ClusterLabs/ha_cluster_exporter/collector"
 )
 
 const subsystem = "corosync"
 
-func NewCollector(cfgToolPath string, quorumToolPath string) (*corosyncCollector, error) {
+func NewCollector(cfgToolPath string, quorumToolPath string, timestamps bool, logger log.Logger) (*corosyncCollector, error) {
 	err := collector.CheckExecutables(cfgToolPath, quorumToolPath)
 	if err != nil {
 		return nil, errors.Wrapf(err, "could not initialize '%s' collector", subsystem)
 	}
 
 	c := &corosyncCollector{
-		collector.NewDefaultCollector(subsystem),
+		collector.NewDefaultCollector(subsystem, timestamps, logger),
 		cfgToolPath,
 		quorumToolPath,
 		NewParser(),
@@ -41,7 +42,7 @@ type corosyncCollector struct {
 }
 
 func (c *corosyncCollector) CollectWithError(ch chan<- prometheus.Metric) error {
-	log.Debugln("Collecting corosync metrics...")
+	level.Debug(c.Logger).Log("msg", "Collecting corosync metrics...")
 
 	// We suppress the exec errors because if any interface is faulty the tools will exit with code 1, but we still want to parse the output.
 	cfgToolOutput, _ := exec.Command(c.cfgToolPath, "-s").Output()
@@ -62,9 +63,11 @@ func (c *corosyncCollector) CollectWithError(ch chan<- prometheus.Metric) error
 }
 
 func (c *corosyncCollector) Collect(ch chan<- prometheus.Metric) {
+	level.Debug(c.Logger).Log("msg", "Collecting corosync metrics...")
+
 	err := c.CollectWithError(ch)
 	if err != nil {
-		log.Warnf("'%s' collector scrape failed: %s", c.GetSubsystem(), err)
+		level.Warn(c.Logger).Log("msg", c.GetSubsystem()+" collector scrape failed", "err", err)
 	}
 }
 

diff --git a/collector/corosync/corosync_test.go b/collector/corosync/corosync_test.go
@@ -3,45 +3,46 @@ package corosync
 import (
 	"testing"
 
+	"github.com/go-kit/log"
 	"github.com/stretchr/testify/assert"
 
 	assertcustom "github.com/ClusterLabs/ha_cluster_exporter/internal/assert"
 )
 
 func TestNewCorosyncCollector(t *testing.T) {
-	_, err := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/fake_corosync-quorumtool.sh")
+	_, err := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/fake_corosync-quorumtool.sh", false, log.NewNopLogger())
 	assert.Nil(t, err)
 }
 
 func TestNewCorosyncCollectorChecksCfgtoolExistence(t *testing.T) {
-	_, err := NewCollector("../../test/nonexistent", "../../test/fake_corosync-quorumtool.sh")
+	_, err := NewCollector("../../test/nonexistent", "../../test/fake_corosync-quorumtool.sh", false, log.NewNopLogger())
 
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "'../../test/nonexistent' does not exist")
 }
 
 func TestNewCorosyncCollectorChecksQuorumtoolExistence(t *testing.T) {
-	_, err := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/nonexistent")
+	_, err := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/nonexistent", false, log.NewNopLogger())
 
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "'../../test/nonexistent' does not exist")
 }
 
 func TestNewCorosyncCollectorChecksCfgtoolExecutableBits(t *testing.T) {
-	_, err := NewCollector("../../test/dummy", "../../test/fake_corosync-quorumtool.sh")
+	_, err := NewCollector("../../test/dummy", "../../test/fake_corosync-quorumtool.sh", false, log.NewNopLogger())
 
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "'../../test/dummy' is not executable")
 }
 
 func TestNewCorosyncCollectorChecksQuorumtoolExecutableBits(t *testing.T) {
-	_, err := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/dummy")
+	_, err := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/dummy", false, log.NewNopLogger())
 
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "'../../test/dummy' is not executable")
 }
 
 func TestCorosyncCollector(t *testing.T) {
-	collector, _ := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/fake_corosync-quorumtool.sh")
+	collector, _ := NewCollector("../../test/fake_corosync-cfgtool.sh", "../../test/fake_corosync-quorumtool.sh", false, log.NewNopLogger())
 	assertcustom.Metrics(t, collector, "corosync.metrics")
 }
diff --git a/collector/default_collector.go b/collector/default_collector.go
@@ -2,9 +2,9 @@ package collector
 
 import (
 	"github.com/ClusterLabs/ha_cluster_exporter/internal/clock"
+	"github.com/go-kit/log"
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
-	config "github.com/spf13/viper"
 	"os"
 )
 
@@ -18,13 +18,17 @@ type DefaultCollector struct {
 	subsystem   string
 	descriptors map[string]*prometheus.Desc
 	Clock       clock.Clock
+	timestamps  bool
+	Logger      log.Logger
 }
 
-func NewDefaultCollector(subsystem string) DefaultCollector {
+func NewDefaultCollector(subsystem string, timestamps bool, logger log.Logger) DefaultCollector {
 	return DefaultCollector{
 		subsystem,
 		make(map[string]*prometheus.Desc),
 		&clock.SystemClock{},
+		timestamps,
+		logger,
 	}
 }
 
@@ -67,7 +71,7 @@ func (c *DefaultCollector) MakeCounterMetric(name string, value float64, labelVa
 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") {
+	if c.timestamps == true {
 		metric = prometheus.NewMetricWithTimestamp(c.Clock.Now(), metric)
 	}
 	return metric

diff --git a/collector/default_collector_test.go b/collector/default_collector_test.go
@@ -3,15 +3,15 @@ package collector
 import (
 	"testing"
 
+	"github.com/go-kit/log"
 	dto "github.com/prometheus/client_model/go"
-	config "github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/ClusterLabs/ha_cluster_exporter/internal/clock"
 )
 
 func TestMetricFactory(t *testing.T) {
-	SUT := NewDefaultCollector("test")
+	SUT := NewDefaultCollector("test", false, log.NewNopLogger())
 	SUT.SetDescriptor("test_metric", "", nil)
 
 	metric := SUT.MakeGaugeMetric("test_metric", 1)
@@ -20,10 +20,8 @@ func TestMetricFactory(t *testing.T) {
 }
 
 func TestMetricFactoryWithTimestamp(t *testing.T) {
-	config.Set("enable-timestamps", true)
-	defer config.Set("enable-timestamps", false)
 
-	SUT := NewDefaultCollector("test")
+	SUT := NewDefaultCollector("test", true, log.NewNopLogger())
 	SUT.Clock = &clock.StoppedClock{}
 	SUT.SetDescriptor("test_metric", "", nil)
 

diff --git a/collector/drbd/drbd.go b/collector/drbd/drbd.go
@@ -8,9 +8,10 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/go-kit/log"
+	"github.com/go-kit/log/level"
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
-	log "github.com/sirupsen/logrus"
 
 	"github.com/ClusterLabs/ha_cluster_exporter/collector"
 )
@@ -47,14 +48,14 @@ type drbdStatus struct {
 	} `json:"connections"`
 }
 
-func NewCollector(drbdSetupPath string, drbdSplitBrainPath string) (*drbdCollector, error) {
+func NewCollector(drbdSetupPath string, drbdSplitBrainPath string, timestamps bool, logger log.Logger) (*drbdCollector, error) {
 	err := collector.CheckExecutables(drbdSetupPath)
 	if err != nil {
 		return nil, errors.Wrapf(err, "could not initialize '%s' collector", subsystem)
 	}
 
 	c := &drbdCollector{
-		collector.NewDefaultCollector(subsystem),
+		collector.NewDefaultCollector(subsystem, timestamps, logger),
 		drbdSetupPath,
 		drbdSplitBrainPath,
 	}
@@ -85,7 +86,7 @@ type drbdCollector struct {
 }
 
 func (c *drbdCollector) CollectWithError(ch chan<- prometheus.Metric) error {
-	log.Debugln("Collecting DRBD metrics...")
+	level.Debug(c.Logger).Log("msg", "Collecting DRBD metrics...")
 
 	c.recordDrbdSplitBrainMetric(ch)
 
@@ -117,13 +118,13 @@ func (c *drbdCollector) CollectWithError(ch chan<- prometheus.Metric) error {
 			}
 		}
 		if len(resource.Connections) == 0 {
-			log.Warnf("Could not retrieve connection info for resource '%s'\n", resource.Name)
+			level.Warn(c.Logger).Log("msg", "Could not retrieve connection info for resource "+resource.Name, "err", err)
 			continue
 		}
 		// a Resource can have multiple connection with different nodes
 		for _, conn := range resource.Connections {
 			if len(conn.PeerDevices) == 0 {
-				log.Warnf("Could not retrieve any peer device info for connection '%d'\n", conn.PeerNodeID)
+				level.Warn(c.Logger).Log("msg", "Could not retrieve any peer device info for connection "+resource.Name, "err", err)
 				continue
 			}
 			for _, peerDev := range conn.PeerDevices {
@@ -141,9 +142,11 @@ func (c *drbdCollector) CollectWithError(ch chan<- prometheus.Metric) error {
 }
 
 func (c *drbdCollector) Collect(ch chan<- prometheus.Metric) {
+	level.Debug(c.Logger).Log("msg", "Collecting DRBD metrics...")
+
 	err := c.CollectWithError(ch)
 	if err != nil {
-		log.Warnf("'%s' collector scrape failed: %s", c.GetSubsystem(), err)
+		level.Warn(c.Logger).Log("msg", c.GetSubsystem()+" collector scrape failed", "err", err)
 	}
 }
 

diff --git a/collector/drbd/drbd_test.go b/collector/drbd/drbd_test.go
@@ -4,6 +4,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/go-kit/log"
 	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/assert"
 
@@ -143,32 +144,32 @@ func TestDrbdParsing(t *testing.T) {
 }
 
 func TestNewDrbdCollector(t *testing.T) {
-	_, err := NewCollector("../../test/fake_drbdsetup.sh", "splitbrainpath")
+	_, err := NewCollector("../../test/fake_drbdsetup.sh", "splitbrainpath", false, log.NewNopLogger())
 
 	assert.Nil(t, err)
 }
 
 func TestNewDrbdCollectorChecksDrbdsetupExistence(t *testing.T) {
-	_, err := NewCollector("../../test/nonexistent", "splitbrainfake")
+	_, err := NewCollector("../../test/nonexistent", "splitbrainfake", false, log.NewNopLogger())
 
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "'../../test/nonexistent' does not exist")
 }
 
 func TestNewDrbdCollectorChecksDrbdsetupExecutableBits(t *testing.T) {
-	_, err := NewCollector("../../test/dummy", "splibrainfake")
+	_, err := NewCollector("../../test/dummy", "splibrainfake", false, log.NewNopLogger())
 
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "'../../test/dummy' is not executable")
 }
 
 func TestDRBDCollector(t *testing.T) {
-	collector, _ := NewCollector("../../test/fake_drbdsetup.sh", "fake")
+	collector, _ := NewCollector("../../test/fake_drbdsetup.sh", "fake", false, log.NewNopLogger())
 	assertcustom.Metrics(t, collector, "drbd.metrics")
 }
 
 func TestDRBDSplitbrainCollector(t *testing.T) {
-	collector, _ := NewCollector("../../test/fake_drbdsetup.sh", "../../test/drbd-splitbrain")
+	collector, _ := NewCollector("../../test/fake_drbdsetup.sh", "../../test/drbd-splitbrain", false, log.NewNopLogger())
 
 	expect := `
 	# HELP ha_cluster_drbd_split_brain Whether a split brain has been detected; 1 line per resource, per volume.

diff --git a/collector/instrumented_collector.go b/collector/instrumented_collector.go
@@ -1,13 +1,13 @@
 package collector
 
 import (
-	"github.com/prometheus/client_golang/prometheus"
-	log "github.com/sirupsen/logrus"
-
 	"github.com/ClusterLabs/ha_cluster_exporter/internal/clock"
+	"github.com/go-kit/log"
+	"github.com/go-kit/log/level"
+	"github.com/prometheus/client_golang/prometheus"
 )
 
-//go:generate mockgen -destination ../test/mock_collector/instrumented_collector.go github.com/ClusterLabs/ha_cluster_exporter/collector InstrumentableCollector
+//go:generate go run -mod=mod github.com/golang/mock/mockgen --build_flags=-mod=mod -package mock_collector -destination ../test/mock_collector/instrumented_collector.go github.com/ClusterLabs/ha_cluster_exporter/collector InstrumentableCollector
 
 // describes a collector that can return errors from collection cycles,
 // instead of the default Prometheus one, which has void Collect returns
@@ -22,9 +22,10 @@ type InstrumentedCollector struct {
 	Clock              clock.Clock
 	scrapeDurationDesc *prometheus.Desc
 	scrapeSuccessDesc  *prometheus.Desc
+	logger             log.Logger
 }
 
-func NewInstrumentedCollector(collector InstrumentableCollector) *InstrumentedCollector {
+func NewInstrumentedCollector(collector InstrumentableCollector, logger log.Logger) *InstrumentedCollector {
 	return &InstrumentedCollector{
 		collector,
 		&clock.SystemClock{},
@@ -44,6 +45,7 @@ func NewInstrumentedCollector(collector InstrumentableCollector) *InstrumentedCo
 				"collector": collector.GetSubsystem(),
 			},
 		),
+		logger,
 	}
 }
 
@@ -55,7 +57,7 @@ func (ic *InstrumentedCollector) Collect(ch chan<- prometheus.Metric) {
 	if err == nil {
 		success = 1
 	} else {
-		log.Warnf("'%s' collector scrape failed: %s", ic.collector.GetSubsystem(), err)
+		level.Warn(ic.logger).Log("msg", ic.collector.GetSubsystem()+" collector scrape failed", "err", err)
 	}
 	ch <- prometheus.MustNewConstMetric(ic.scrapeDurationDesc, prometheus.GaugeValue, duration.Seconds())
 	ch <- prometheus.MustNewConstMetric(ic.scrapeSuccessDesc, prometheus.GaugeValue, success)