fix: remove back pressure and add retryable error

raghu999 · raghu999 · commit b1c96fb628ff · 2025-11-16T00:45:59.000-05:00
diff --git a/exporter/exporterhelper/internal/experr/retryable_error.go b/exporter/exporterhelper/internal/experr/retryable_error.go
@@ -8,7 +8,7 @@ import (
 	"errors"
 	"net"
 	"net/http"
-	"strings"
+	"regexp"
 
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -29,18 +29,30 @@ const (
 	ReasonHTTP504               Reason = "http_504"
 	ReasonHTTPRetryAfter        Reason = "http_retry_after"
 	ReasonNetTimeout            Reason = "net_timeout"
-	ReasonTextBackpressure      Reason = "text_backpressure"
+	ReasonTextCircuitBreaker    Reason = "text:circuit_breaker"
+	ReasonTextOverload          Reason = "text:overload"
+	ReasonTextBackpressure      Reason = "text:backpressure"
 )
 
-// ClassifyBackpressure returns (isBackpressure, reason).
+var fallbackRegex = regexp.MustCompile(
+	`(?i)(429|too many requests)|` + // ReasonHTTP429
+		`(503|service unavailable)|` + // ReasonHTTP503
+		`(504|gateway timeout|upstream timeout)|` + // ReasonHTTP504
+		`(retry-after)|` + // ReasonHTTPRetryAfter
+		`(circuit breaker)|` + // ReasonTextCircuitBreaker
+		`(overload)|` + // ReasonTextOverload
+		`(backpressure)`, // ReasonTextBackpressure
+)
+
+// ClassifyErrorReason returns (isRetryable, reason).
 // The logic is strictly:
 //  1. Typed signals first (fast, no allocs)
-//  2. Fallback to a single lowercase + a few contains checks
+//  2. Fallback to a single regex match on the error string
 //
 // NOTE: This function runs only on error paths. Even then,
 // typed checks return early. Fallback string scanning is a
 // micro-cost compared to network I/O and retry backoff.
-func ClassifyBackpressure(err error) (bool, Reason) {
+func ClassifyErrorReason(err error) (bool, Reason) {
 	if err == nil {
 		return false, ""
 	}
@@ -88,30 +100,37 @@ func ClassifyBackpressure(err error) (bool, Reason) {
 		}
 	}
 
-	// 5) Fallback: cheap lowercase + tight substring checks.
-	// Keep this list intentionally small to minimize false positives.
-	s := strings.ToLower(err.Error())
-	if strings.Contains(s, "429") || strings.Contains(s, "too many requests") {
-		return true, ReasonHTTP429
+	// 5) Fallback: cheap regex match on error string.
+	matches := fallbackRegex.FindStringSubmatch(err.Error())
+	if matches == nil {
+		return false, ""
 	}
-	if strings.Contains(s, "503") || strings.Contains(s, "service unavailable") {
+
+	// The regex has 7 capture groups, one for each reason.
+	// FindStringSubmatch returns the full match at [0], then groups at [1]...[n].
+	// We check which group captured.
+	switch {
+	case matches[1] != "":
+		return true, ReasonHTTP429
+	case matches[2] != "":
 		return true, ReasonHTTP503
-	}
-	if strings.Contains(s, "504") || strings.Contains(s, "gateway timeout") || strings.Contains(s, "upstream timeout") {
+	case matches[3] != "":
 		return true, ReasonHTTP504
-	}
-	if strings.Contains(s, "retry-after") {
+	case matches[4] != "":
 		return true, ReasonHTTPRetryAfter
-	}
-	if strings.Contains(s, "circuit breaker") || strings.Contains(s, "overload") || strings.Contains(s, "backpressure") {
+	case matches[5] != "":
+		return true, ReasonTextCircuitBreaker
+	case matches[6] != "":
+		return true, ReasonTextOverload
+	case matches[7] != "":
 		return true, ReasonTextBackpressure
 	}
 
 	return false, ""
 }
 
-// IsBackpressure is a convenience wrapper.
-func IsBackpressure(err error) bool {
-	ok, _ := ClassifyBackpressure(err)
+// IsRetryableError is a convenience wrapper.
+func IsRetryableError(err error) bool {
+	ok, _ := ClassifyErrorReason(err)
 	return ok
 }
diff --git a/exporter/exporterhelper/internal/experr/retryable_error_test.go b/exporter/exporterhelper/internal/experr/retryable_error_test.go
@@ -39,19 +39,19 @@ func lower(msg string) error { return errors.New(strings.ToLower(msg)) }
 
 // ----- tests -----
 
-func TestClassifyBackpressure_TypedContextDeadline(t *testing.T) {
-	ok, reason := ClassifyBackpressure(context.DeadlineExceeded)
+func TestClassifyErrorReason_TypedContextDeadline(t *testing.T) {
+	ok, reason := ClassifyErrorReason(context.DeadlineExceeded)
 	assert.True(t, ok)
 	assert.Equal(t, ReasonDeadline, reason)
 }
 
-func TestClassifyBackpressure_TypedNetTimeout(t *testing.T) {
-	ok, reason := ClassifyBackpressure(netTimeoutErr{})
+func TestClassifyErrorReason_TypedNetTimeout(t *testing.T) {
+	ok, reason := ClassifyErrorReason(netTimeoutErr{})
 	assert.True(t, ok)
 	assert.Equal(t, ReasonNetTimeout, reason)
 }
 
-func TestClassifyBackpressure_TypedGRPC_StatusCodes(t *testing.T) {
+func TestClassifyErrorReason_TypedGRPC_StatusCodes(t *testing.T) {
 	cases := []struct {
 		err    error
 		reason Reason
@@ -62,13 +62,13 @@ func TestClassifyBackpressure_TypedGRPC_StatusCodes(t *testing.T) {
 	}
 
 	for _, tc := range cases {
-		ok, reason := ClassifyBackpressure(tc.err)
+		ok, reason := ClassifyErrorReason(tc.err)
 		assert.True(t, ok, tc.err)
 		assert.Equal(t, tc.reason, reason)
 	}
 }
 
-func TestClassifyBackpressure_TypedHTTP_StatusCodes(t *testing.T) {
+func TestClassifyErrorReason_TypedHTTP_StatusCodes(t *testing.T) {
 	cases := []struct {
 		code   int
 		header http.Header
@@ -83,64 +83,74 @@ func TestClassifyBackpressure_TypedHTTP_StatusCodes(t *testing.T) {
 	for _, tc := range cases {
 		resp := &http.Response{StatusCode: tc.code, Header: tc.header}
 		err := httpRespErr{resp: resp, msg: "transport error"}
-		ok, reason := ClassifyBackpressure(err)
+		ok, reason := ClassifyErrorReason(err)
 		assert.True(t, ok, tc)
 		assert.Equal(t, tc.reason, reason, tc)
 	}
 }
 
-func TestClassifyBackpressure_FallbackStrings_Positive(t *testing.T) {
-	// These exercise the lowercase + contains path.
+func TestClassifyErrorReason_FallbackRegex_Positive(t *testing.T) {
+	// These exercise the regex fallback path.
 	cases := []struct {
 		err    error
 		reason Reason
 	}{
 		{errors.New("HTTP 429 Too Many Requests"), ReasonHTTP429},
 		{lower("too many requests"), ReasonHTTP429},
+		{errors.New("some 429 error"), ReasonHTTP429},
 
 		{errors.New("received HTTP 503 Service Unavailable"), ReasonHTTP503},
 		{lower("service unavailable"), ReasonHTTP503},
+		{errors.New("503 blah"), ReasonHTTP503},
 
 		{errors.New("HTTP 504 Gateway Timeout"), ReasonHTTP504},
 		{lower("gateway timeout"), ReasonHTTP504},
 		{lower("upstream timeout"), ReasonHTTP504},
+		{errors.New("got a 504"), ReasonHTTP504},
 
 		{lower("retry-after header present"), ReasonHTTPRetryAfter},
+		{errors.New("Retry-After: 30"), ReasonHTTPRetryAfter},
+
+		{lower("circuit breaker open"), ReasonTextCircuitBreaker},
+		{errors.New("Circuit Breaker"), ReasonTextCircuitBreaker},
+
+		{lower("system overload"), ReasonTextOverload},
+		{errors.New("OVERLOAD"), ReasonTextOverload},
 
-		{lower("circuit breaker open"), ReasonTextBackpressure},
-		{lower("system overload"), ReasonTextBackpressure},
 		{lower("backpressure detected"), ReasonTextBackpressure},
+		{errors.New("BACKPRESSURE"), ReasonTextBackpressure},
 	}
 
 	for _, tc := range cases {
-		ok, reason := ClassifyBackpressure(tc.err)
+		ok, reason := ClassifyErrorReason(tc.err)
 		assert.True(t, ok, tc.err)
 		assert.Equal(t, tc.reason, reason, tc.err)
 	}
 }
 
-func TestIsBackpressure_Positive_Smoke(t *testing.T) {
+func TestIsRetryableError_Positive_Smoke(t *testing.T) {
 	// A quick smoke test through the convenience wrapper.
-	assert.True(t, IsBackpressure(status.Error(codes.ResourceExhausted, "x")))
-	assert.True(t, IsBackpressure(netTimeoutErr{}))
-	assert.True(t, IsBackpressure(context.DeadlineExceeded))
-	assert.True(t, IsBackpressure(errors.New("429")))
-	assert.True(t, IsBackpressure(errors.New("service unavailable")))
-	assert.True(t, IsBackpressure(errors.New("gateway timeout")))
+	assert.True(t, IsRetryableError(status.Error(codes.ResourceExhausted, "x")))
+	assert.True(t, IsRetryableError(netTimeoutErr{}))
+	assert.True(t, IsRetryableError(context.DeadlineExceeded))
+	assert.True(t, IsRetryableError(errors.New("429")))
+	assert.True(t, IsRetryableError(errors.New("service unavailable")))
+	assert.True(t, IsRetryableError(errors.New("gateway timeout")))
+	assert.True(t, IsRetryableError(errors.New("OVERLOAD")))
 }
 
-func TestIsBackpressure_NegativeCases(t *testing.T) {
+func TestIsRetryableError_NegativeCases(t *testing.T) {
 	cases := []error{
 		nil,
 		errors.New("some transient error"),
-		errors.New("internal server error"), // intentionally not treated as backpressure
-		errors.New("permission denied"),     // not backpressure
+		errors.New("internal server error"), // intentionally not treated as retryable
+		errors.New("permission denied"),     // not retryable
 		errors.New("bad gateway (502)"),     // not in our fallback allowlist
 		errors.New("request timeout (408)"), // not in our fallback allowlist
 		errors.New("proxy timeout (599)"),   // not in our fallback allowlist
 	}
 
 	for _, err := range cases {
-		assert.False(t, IsBackpressure(err), "expected not backpressure: %v", err)
+		assert.False(t, IsRetryableError(err), "expected not retryable: %v", err)
 	}
 }
diff --git a/exporter/exporterhelper/internal/queue_sender.go b/exporter/exporterhelper/internal/queue_sender.go
@@ -106,7 +106,7 @@ func NewQueueSender(
 			err := origExportFunc(ctx, req)
 			rtt := time.Since(startTime)
 
-			isBackpressure := experr.IsBackpressure(err)
+			isBackpressure := experr.IsRetryableError(err)
 
 			isSuccess := err == nil
 
