Use empty string as the fault metric method label for HTTP requests. (#693)

* Comment out labels with high cardinality.

The remaining labels have maximums of 3, 2, 2 and 2 values respectively.

* Updated to keep clientName since it's local and not upstream.

* Remove rather than comment out.

No need to keep these around.

* Update to use an empty string as the method label for http requests.

* Add comment.

Author: HiramSilvey

httpbp/client_middlewares.go

@@ -15,6 +15,7 @@ import (
 	"github.com/avast/retry-go"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/reddit/baseplate.go/headerbp"
+	"github.com/reddit/baseplate.go/internal/faults"
 	"github.com/reddit/baseplate.go/secrets"
 
 	"github.com/reddit/baseplate.go/breakerbp"
@@ -467,7 +468,15 @@ func (c clientFaultMiddleware) Middleware() ClientMiddleware {
 			address := req.URL.Hostname()
 			method := strings.TrimPrefix(req.URL.Path, "/")
 			header := httpHeader(req.Header)
-			return c.injector.InjectWithAbortOverride(req.Context(), address, method, &header, resume, abort)
+			return c.injector.Inject(req.Context(),
+				faults.InjectParameters[*http.Response]{
+					Address:     address,
+					Method:      method,
+					MethodLabel: "",
+					Headers:     &header,
+					Resume:      resume,
+					Abort:       abort,
+				})
 		})
 	}
 }

internal/faults/faults.go

@@ -139,15 +139,30 @@ func NewInjector[T any](clientName, callerName string, abortCodeMin, abortCodeMa
 	return i
 }
 
-// InjectWithAbortOverride injects a fault using the provided fault function
-// on the outgoing request if it matches the header configuration.
-func (i *Injector[T]) InjectWithAbortOverride(ctx context.Context, address, method string, headers Headers, resume Resume[T], abort Abort[T]) (T, error) {
+// InjectParameters contains the parameters needed to match and inject a fault
+// into the outgoing request.
+type InjectParameters[T any] struct {
+	Address     string
+	Method      string
+	MethodLabel string
+	Headers     Headers
+	Resume      Resume[T]
+	Abort       Abort[T]
+}
+
+// Inject injects a fault using the Injector default fault function on the
+// outgoing request if it matches the header configuration.
+func (i *Injector[T]) Inject(ctx context.Context, params InjectParameters[T]) (T, error) {
+	if params.Abort == nil {
+		params.Abort = i.defaultAbort
+	}
+
 	delayed := false
 	totalReqsCounter := func(success, aborted bool) prometheus.Counter {
 		return totalRequests.WithLabelValues(
 			i.clientName,
-			address,
-			method,
+			params.Address,
+			params.MethodLabel,
 			i.callerName,
 			strconv.FormatBool(success),
 			strconv.FormatBool(delayed),
@@ -166,47 +181,41 @@ func (i *Injector[T]) InjectWithAbortOverride(ctx context.Context, address, meth
 		}
 	}
 
-	faultHeaderValues, err := headers.LookupValues(ctx, FaultHeader)
+	faultHeaderValues, err := params.Headers.LookupValues(ctx, FaultHeader)
 	if err != nil {
 		infof("error looking up the values of header %q: %v", FaultHeader, err)
 		totalReqsCounter(true, false).Inc()
-		return resume()
+		return params.Resume()
 	}
 
-	faultConfiguration, err := parseMatchingFaultConfiguration(faultHeaderValues, getCanonicalAddress(address), method, i.abortCodeMin, i.abortCodeMax)
+	faultConfiguration, err := parseMatchingFaultConfiguration(faultHeaderValues, getCanonicalAddress(params.Address), params.Method, i.abortCodeMin, i.abortCodeMax)
 	if err != nil {
 		warnf("error parsing fault header %q: %v", FaultHeader, err)
 
 		if faultConfiguration == nil {
 			totalReqsCounter(false, false).Inc()
-			return resume()
+			return params.Resume()
 		}
 	}
 	if faultConfiguration == nil {
 		totalReqsCounter(true, false).Inc()
-		return resume()
+		return params.Resume()
 	}
 
 	if faultConfiguration.Delay > 0 && i.selected(faultConfiguration.DelayPercentage) {
 		if err := i.sleep(ctx, faultConfiguration.Delay); err != nil {
 			warnf("error when delaying request: %v", err)
 			totalReqsCounter(false, false).Inc()
-			return resume()
+			return params.Resume()
 		}
 		delayed = true
 	}
 
 	if faultConfiguration.AbortCode != -1 && i.selected(faultConfiguration.AbortPercentage) {
 		totalReqsCounter(true, true).Inc()
-		return abort(faultConfiguration.AbortCode, faultConfiguration.AbortMessage)
+		return params.Abort(faultConfiguration.AbortCode, faultConfiguration.AbortMessage)
 	}
 
 	totalReqsCounter(true, false).Inc()
-	return resume()
-}
-
-// Inject injects a fault using the Injector default fault function on the
-// outgoing request if it matches the header configuration.
-func (i *Injector[T]) Inject(ctx context.Context, address, method string, headers Headers, resume Resume[T]) (T, error) {
-	return i.InjectWithAbortOverride(ctx, address, method, headers, resume, i.defaultAbort)
+	return params.Resume()
 }

internal/faults/faults_test.go

@@ -220,7 +220,12 @@ func TestInject(t *testing.T) {
 				return nil
 			}
 
-			resp, err := injector.Inject(context.Background(), address, method, &headers, resume)
+			resp, err := injector.Inject(context.Background(), InjectParameters[*response]{
+				Address: address,
+				Method:  method,
+				Headers: &headers,
+				Resume:  resume,
+			})
 
 			if err != nil {
 				t.Fatalf("expected no error, got %v", err)

thriftbp/client_middlewares.go

@@ -16,6 +16,7 @@ import (
 	"github.com/reddit/baseplate.go/ecinterface"
 	"github.com/reddit/baseplate.go/errorsbp"
 	"github.com/reddit/baseplate.go/headerbp"
+	"github.com/reddit/baseplate.go/internal/faults"
 	"github.com/reddit/baseplate.go/internal/gen-go/reddit/baseplate"
 	"github.com/reddit/baseplate.go/internal/thriftint"
 
@@ -469,7 +470,14 @@ func (c clientFaultMiddleware) Middleware() thrift.ClientMiddleware {
 					return next.Call(ctx, method, args, result)
 				}
 
-				return c.injector.Inject(ctx, c.address, method, &thriftHeaders{}, resume)
+				return c.injector.Inject(ctx,
+					faults.InjectParameters[thrift.ResponseMeta]{
+						Address:     c.address,
+						Method:      method,
+						MethodLabel: method,
+						Headers:     &thriftHeaders{},
+						Resume:      resume,
+					})
 			},
 		}
 	}