remove nil checks around uses of autographer.stats

This removes the now unneeded checks to see if `autographer.stats` is nil wherever we gather stats. We also remove some nil checks in signer.StatsClient that are no longer workable as it accepts the statds.ClientInterface type. Some of the older code was checking for error returns that they probably shouldn't (logging about metrics problems tends to clutter up code and the alerting should already tell you you have a problem), but we leave those alone for now. Updates AUT-159
mozilla-services · Aug 1, 2024 · 3bffa70 · 3bffa70
1 parent 5f97ba3
commit 3bffa70
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 53 deletions.
diff --git a/authorize.go b/authorize.go
@@ -47,22 +47,18 @@ func (a *autographer) authorizeHeader(r *http.Request) (auth *hawk.Auth, userid
 		return nil, "", fmt.Errorf("missing Authorization header")
 	}
 	auth, err = hawk.ParseRequestHeader(r.Header.Get("Authorization"))
-	if a.stats != nil {
-		sendStatsErr := a.stats.Timing("hawk.header_parsed", time.Since(getRequestStartTime(r)), nil, 1.0)
-		if sendStatsErr != nil {
-			log.Warnf("Error sending hawk.header_parsed: %s", sendStatsErr)
-		}
+	sendStatsErr := a.stats.Timing("hawk.header_parsed", time.Since(getRequestStartTime(r)), nil, 1.0)
+	if sendStatsErr != nil {
+		log.Warnf("Error sending hawk.header_parsed: %s", sendStatsErr)
 	}
 	if err != nil {
 		return nil, "", err
 	}
 	userid = auth.Credentials.ID
 	auth, err = hawk.NewAuthFromRequest(r, a.lookupCred(userid), a.lookupNonce)
-	if a.stats != nil {
-		sendStatsErr := a.stats.Timing("hawk.auth_created", time.Since(getRequestStartTime(r)), nil, 1.0)
-		if sendStatsErr != nil {
-			log.Warnf("Error sending hawk.auth_created: %s", sendStatsErr)
-		}
+	sendStatsErr = a.stats.Timing("hawk.auth_created", time.Since(getRequestStartTime(r)), nil, 1.0)
+	if sendStatsErr != nil {
+		log.Warnf("Error sending hawk.auth_created: %s", sendStatsErr)
 	}
 	if err != nil {
 		return nil, "", err
@@ -73,17 +69,16 @@ func (a *autographer) authorizeHeader(r *http.Request) (auth *hawk.Auth, userid
 	}
 	hawk.MaxTimestampSkew = a.hawkMaxTimestampSkew
 	err = auth.Valid()
-	if a.stats != nil {
-		sendStatsErr := a.stats.Timing("hawk.validated", time.Since(getRequestStartTime(r)), nil, 1.0)
-		if sendStatsErr != nil {
-			log.Warnf("Error sending hawk.validated: %s", sendStatsErr)
-		}
-		skew := abs(auth.ActualTimestamp.Sub(auth.Timestamp))
-		sendStatsErr = a.stats.Timing("hawk.timestamp_skew", skew, nil, 1.0)
-		if sendStatsErr != nil {
-			log.Warnf("Error sending hawk.timestamp_skew: %s", sendStatsErr)
-		}
+	sendStatsErr = a.stats.Timing("hawk.validated", time.Since(getRequestStartTime(r)), nil, 1.0)
+	if sendStatsErr != nil {
+		log.Warnf("Error sending hawk.validated: %s", sendStatsErr)
 	}
+	skew := abs(auth.ActualTimestamp.Sub(auth.Timestamp))
+	sendStatsErr = a.stats.Timing("hawk.timestamp_skew", skew, nil, 1.0)
+	if sendStatsErr != nil {
+		log.Warnf("Error sending hawk.timestamp_skew: %s", sendStatsErr)
+	}
+
 	if err != nil {
 		return nil, "", err
 	}
@@ -95,11 +90,9 @@ func (a *autographer) authorizeHeader(r *http.Request) (auth *hawk.Auth, userid
 func (a *autographer) authorizeBody(auth *hawk.Auth, r *http.Request, body []byte) (err error) {
 	payloadhash := auth.PayloadHash(r.Header.Get("Content-Type"))
 	payloadhash.Write(body)
-	if a.stats != nil {
-		sendStatsErr := a.stats.Timing("hawk.payload_hashed", time.Since(getRequestStartTime(r)), nil, 1.0)
-		if sendStatsErr != nil {
-			log.Warnf("Error sending hawk.payload_hashed: %s", sendStatsErr)
-		}
+	sendStatsErr := a.stats.Timing("hawk.payload_hashed", time.Since(getRequestStartTime(r)), nil, 1.0)
+	if sendStatsErr != nil {
+		log.Warnf("Error sending hawk.payload_hashed: %s", sendStatsErr)
 	}
 	if !auth.ValidHash(payloadhash) {
 		return fmt.Errorf("payload validation failed")

diff --git a/handlers.go b/handlers.go
@@ -81,11 +81,9 @@ func (a *autographer) handleSignature(w http.ResponseWriter, r *http.Request) {
 	starttime := getRequestStartTime(r)
 	auth, userid, err := a.authorizeHeader(r)
 	if err != nil {
-		if a.stats != nil {
-			sendStatsErr := a.stats.Timing("hawk.authorize_header_failed", time.Since(starttime), nil, 1.0)
-			if sendStatsErr != nil {
-				log.Warnf("Error sending hawk.authorize_header_failed: %s", sendStatsErr)
-			}
+		sendStatsErr := a.stats.Timing("hawk.authorize_header_failed", time.Since(starttime), nil, 1.0)
+		if sendStatsErr != nil {
+			log.Warnf("Error sending hawk.authorize_header_failed: %s", sendStatsErr)
 		}
 		httpError(w, r, http.StatusUnauthorized, "authorization verification failed: %v", err)
 		return
@@ -110,23 +108,19 @@ func (a *autographer) handleSignature(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	err = a.authorizeBody(auth, r, body)
-	if a.stats != nil {
-		sendStatsErr := a.stats.Timing("authorize_finished", time.Since(starttime), nil, 1.0)
-		if sendStatsErr != nil {
-			log.Warnf("Error sending authorize_finished: %s", sendStatsErr)
-		}
+	sendStatsErr := a.stats.Timing("authorize_finished", time.Since(starttime), nil, 1.0)
+	if sendStatsErr != nil {
+		log.Warnf("Error sending authorize_finished: %s", sendStatsErr)
 	}
 	if err != nil {
 		httpError(w, r, http.StatusUnauthorized, "authorization verification failed: %v", err)
 		return
 	}
 	var sigreqs []formats.SignatureRequest
 	err = json.Unmarshal(body, &sigreqs)
-	if a.stats != nil {
-		sendStatsErr := a.stats.Timing("body_unmarshaled", time.Since(starttime), nil, 1.0)
-		if sendStatsErr != nil {
-			log.Warnf("Error sending body_unmarshaled: %s", sendStatsErr)
-		}
+	sendStatsErr = a.stats.Timing("body_unmarshaled", time.Since(starttime), nil, 1.0)
+	if sendStatsErr != nil {
+		log.Warnf("Error sending body_unmarshaled: %s", sendStatsErr)
 	}
 	if err != nil {
 		httpError(w, r, http.StatusBadRequest, "failed to parse request body: %v", err)

diff --git a/main.go b/main.go
@@ -405,11 +405,9 @@ func (a *autographer) addSigners(signerConfs []signer.Configuration) error {
 			statsClient *signer.StatsClient
 			err         error
 		)
-		if a.stats != nil {
-			statsClient, err = signer.NewStatsClient(signerConf, a.stats)
-			if statsClient == nil || err != nil {
-				return fmt.Errorf("failed to add signer stats client %q or got back nil statsClient: %w", signerConf.ID, err)
-			}
+		statsClient, err = signer.NewStatsClient(signerConf, a.stats)
+		if statsClient == nil || err != nil {
+			return fmt.Errorf("failed to add signer stats client %q or got back nil statsClient: %w", signerConf.ID, err)
 		}
 		// give the database handler to the signer configuration
 		if a.db != nil {

diff --git a/signer/signer.go b/signer/signer.go
@@ -542,10 +542,6 @@ func NewStatsClient(signerConfig Configuration, stats statsd.ClientInterface) (*
 // a statsd gauge with the given name, int value cast to float64, tags
 // for the signer, and sampling rate of 1
 func (s *StatsClient) SendGauge(name string, value int) {
-	if s.stats == nil {
-		log.Warnf("xpi: statsd client is nil. Could not send gauge %s with value %v", name, value)
-		return
-	}
 	err := s.stats.Gauge(name, float64(value), s.signerTags, 1)
 	if err != nil {
 		log.Warnf("Error sending gauge %s: %s", name, err)
@@ -557,10 +553,6 @@ func (s *StatsClient) SendGauge(name string, value int) {
 // converted to ms, cast to float64, tags for the signer, and sampling
 // rate of 1
 func (s *StatsClient) SendHistogram(name string, value time.Duration) {
-	if s.stats == nil {
-		log.Warnf("xpi: statsd client is nil. Could not send histogram %s with value %s", name, value)
-		return
-	}
 	err := s.stats.Histogram(name, float64(value/time.Millisecond), s.signerTags, 1)
 	if err != nil {
 		log.Warnf("Error sending histogram %s: %s", name, err)

diff --git a/signer/xpi/xpi.go b/signer/xpi/xpi.go
@@ -128,6 +128,8 @@ type XPISigner struct {
 
 // New initializes an XPI signer using a configuration
 func New(conf signer.Configuration, stats *signer.StatsClient) (s *XPISigner, err error) {
+	// TODO(AUT-160): instead of doing nil checks for stats all over XPISigner,
+	// we could just check it here once or provide a null object version of it for tests.
 	s = new(XPISigner)
 	if conf.Type != Type {
 		return nil, fmt.Errorf("xpi: invalid type %q, must be %q", conf.Type, Type)