From a88ed97b7fd0a9683a8715670bf76d33d47b04eb Mon Sep 17 00:00:00 2001
From: Jeff Hodges <jeff@somethingsimilar.com>
Date: Mon, 29 Jul 2024 17:07:23 -0700
Subject: [PATCH] remove need for nil checks around statsd metrics

Currently, we have to check if `autographer.stats` is nil anywhere we
want to emit a statsd metric. This is error-prone and verbose. Work for
AUT-150 (and more) would be easier if we do this here.

This patch makes the `autographer.stats` field always non-nil. By
default, `autographer`s created with `newAutographer` will have a
`statsd.NoOpClient` object for its `stats` and if the autograph config
has a statsd server configured, it will be replaced with a real statsd
client when `autographer.addStats` is called. That addStats call will
now always occur in autograph's main function.

This moves the conditional for checking if the statsd server is
configured into autographer.addStats. However, it would probably be
better in the future for us to have this and the similar
`autograph.add*` methods moved into the `newAutographer` contructor.
This, though, at least makes the autographer type a lil safer on
construction.

This means that we use `statds.ClientInterface` instead of
`*statsd.Client` everywhere to fit both client types.

Along the way, we also have to fix up the main_test.go that was
reproducing that conditional from the main inside it. We also fix an
signer/xpi test that was depending on a statsd server running but not
actually needing it to finish.

This patch doesn't remove all of the nil checks. A follow up will be
made to do so.

Updates AUT-159
Updates AUT-150
---
 main.go                | 11 +++++------
 main_test.go           |  8 +++-----
 signer/signer.go       |  7 ++-----
 signer/xpi/xpi_test.go |  7 +------
 stats.go               | 17 ++++++++++++++---
 5 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/main.go b/main.go
index 2a899afa9..f0928c5b6 100755
--- a/main.go
+++ b/main.go
@@ -72,7 +72,7 @@ type configuration struct {
 // with all signers and permissions configured
 type autographer struct {
 	db                   *database.Handler
-	stats                *statsd.Client
+	stats                statsd.ClientInterface
 	nonces               *lru.Cache
 	debug                bool
 	heartbeatConf        *heartbeatConfig
@@ -166,11 +166,9 @@ func run(conf configuration, listen string, debug bool) {
 		ag.initHSM(conf)
 	}
 
-	if conf.Statsd.Addr != "" {
-		err = ag.addStats(conf)
-		if err != nil {
-			log.Fatal(err)
-		}
+	err = ag.addStats(conf)
+	if err != nil {
+		log.Fatal(err)
 	}
 
 	err = ag.addSigners(conf.Signers)
@@ -292,6 +290,7 @@ func newAutographer(cachesize int) (a *autographer) {
 	a.authBackend = newInMemoryAuthBackend()
 	a.nonces, err = lru.New(cachesize)
 	a.exit = make(chan interface{})
+	a.stats = &statsd.NoOpClient{}
 	if err != nil {
 		log.Fatal(err)
 	}
diff --git a/main_test.go b/main_test.go
index 3061808e7..75d5d5a40 100644
--- a/main_test.go
+++ b/main_test.go
@@ -40,11 +40,9 @@ func TestMain(m *testing.M) {
 	if err != nil {
 		log.Fatal(err)
 	}
-	if conf.Statsd.Addr != "" {
-		err = ag.addStats(conf)
-		if err != nil {
-			log.Fatal(err)
-		}
+	err = ag.addStats(conf)
+	if err != nil {
+		log.Fatal(err)
 	}
 	if conf.HawkTimestampValidity != "" {
 		ag.hawkMaxTimestampSkew, err = time.ParseDuration(conf.HawkTimestampValidity)
diff --git a/signer/signer.go b/signer/signer.go
index d57a607c0..3218790f1 100644
--- a/signer/signer.go
+++ b/signer/signer.go
@@ -523,14 +523,11 @@ type StatsClient struct {
 	signerTags []string
 
 	// stats is the statsd client for reporting metrics
-	stats *statsd.Client
+	stats statsd.ClientInterface
 }
 
 // NewStatsClient makes a new stats client
-func NewStatsClient(signerConfig Configuration, stats *statsd.Client) (*StatsClient, error) {
-	if stats == nil {
-		return nil, fmt.Errorf("xpi: statsd client is nil. Could not create StatsClient for signer %s", signerConfig.ID)
-	}
+func NewStatsClient(signerConfig Configuration, stats statsd.ClientInterface) (*StatsClient, error) {
 	return &StatsClient{
 		stats: stats,
 		signerTags: []string{
diff --git a/signer/xpi/xpi_test.go b/signer/xpi/xpi_test.go
index 012aa4d50..da83edcf2 100644
--- a/signer/xpi/xpi_test.go
+++ b/signer/xpi/xpi_test.go
@@ -57,12 +57,7 @@ func TestSignFile(t *testing.T) {
 					StatsSampleRate:        10 * time.Second,
 				}
 
-				statsdClient, err := statsd.NewBuffered("localhost:8135", 1)
-				if err != nil {
-					t.Fatalf("passing testcase %d: Error constructing statsdClient: %v", i, err)
-				}
-				statsdClient.Namespace = "test_autograph_stats_ns"
-				signerStatsClient, err := signer.NewStatsClient(testcase, statsdClient)
+				signerStatsClient, err := signer.NewStatsClient(testcase, &statsd.NoOpClient{})
 				if err != nil {
 					t.Fatalf("passing testcase %d: Error constructing signer.StatsdClient: %v", i, err)
 				}
diff --git a/stats.go b/stats.go
index e5071060a..1f7090797 100644
--- a/stats.go
+++ b/stats.go
@@ -18,8 +18,19 @@ func loadStatsd(conf configuration) (*statsd.Client, error) {
 	return statsdClient, nil
 }
 
-func (a *autographer) addStats(conf configuration) (err error) {
-	a.stats, err = loadStatsd(conf)
+func (a *autographer) addStats(conf configuration) error {
+	if conf.Statsd.Addr == "" {
+		// a.stats is set to a safe value in newAutographer, so we leave it
+		// alone and return.
+		log.Infof("Statsd left disabled as no `statsd.addr` was provided in config")
+		return nil
+	}
+
+	stats, err := loadStatsd(conf)
+	if err != nil {
+		return err
+	}
+	a.stats = stats
 	log.Infof("Statsd enabled at %s with namespace %s", conf.Statsd.Addr, conf.Statsd.Namespace)
-	return err
+	return nil
 }