Fix handling of non-standard HTTP status codes (#3966)

* semconvutil: Fix HTTPClientStatus and HTTPServerStatus * Regenerete semconvutil * Update changelog
open-telemetry · Jun 8, 2023 · 5cc3715 · 5cc3715
1 parent 916ae40
commit 5cc3715
Show file tree

Hide file tree

Showing 17 changed files with 200 additions and 632 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - AWS XRay Remote Sampling to cap quotaBalance to 1x quota in `go.opentelemetry.io/contrib/samplers/aws/xray`. (#3651, #3652)
 - Do not panic when the HTTP request has the "Expect: 100-continue" header in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#3892)
+- Fix span status value set for non-standard HTTP status codes in modules listed below. (#3966)
+  - `go.opentelemetry.io/contrib/instrumentation/github.com/emicklei/go-restful/otelrestful`
+  - `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`
+  - `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux`
+  - `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`
+  - `go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron`
+  - `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`
+  - `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`
 
 ### Removed
 

diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go
@@ -437,83 +437,24 @@ func (c *httpConv) header(prefix string, h http.Header) []attribute.KeyValue {
 // ClientStatus returns a span status code and message for an HTTP status code
 // value received by a client.
 func (c *httpConv) ClientStatus(code int) (codes.Code, string) {
-	stat, valid := validateHTTPStatusCode(code)
-	if !valid {
-		return stat, fmt.Sprintf("Invalid HTTP status code %d", code)
+	if code < 100 || code >= 600 {
+		return codes.Error, fmt.Sprintf("Invalid HTTP status code %d", code)
 	}
-	return stat, ""
+	if code >= 400 {
+		return codes.Error, ""
+	}
+	return codes.Unset, ""
 }
 
 // ServerStatus returns a span status code and message for an HTTP status code
 // value returned by a server. Status codes in the 400-499 range are not
 // returned as errors.
 func (c *httpConv) ServerStatus(code int) (codes.Code, string) {
-	stat, valid := validateHTTPStatusCode(code)
-	if !valid {
-		return stat, fmt.Sprintf("Invalid HTTP status code %d", code)
-	}
-
-	if code/100 == 4 {
-		return codes.Unset, ""
-	}
-	return stat, ""
-}
-
-type codeRange struct {
-	fromInclusive int
-	toInclusive   int
-}
-
-func (r codeRange) contains(code int) bool {
-	return r.fromInclusive <= code && code <= r.toInclusive
-}
-
-var validRangesPerCategory = map[int][]codeRange{
-	1: {
-		{http.StatusContinue, http.StatusEarlyHints},
-	},
-	2: {
-		{http.StatusOK, http.StatusAlreadyReported},
-		{http.StatusIMUsed, http.StatusIMUsed},
-	},
-	3: {
-		{http.StatusMultipleChoices, http.StatusUseProxy},
-		{http.StatusTemporaryRedirect, http.StatusPermanentRedirect},
-	},
-	4: {
-		{http.StatusBadRequest, http.StatusTeapot}, // yes, teapot is so useful…
-		{http.StatusMisdirectedRequest, http.StatusUpgradeRequired},
-		{http.StatusPreconditionRequired, http.StatusTooManyRequests},
-		{http.StatusRequestHeaderFieldsTooLarge, http.StatusRequestHeaderFieldsTooLarge},
-		{http.StatusUnavailableForLegalReasons, http.StatusUnavailableForLegalReasons},
-	},
-	5: {
-		{http.StatusInternalServerError, http.StatusLoopDetected},
-		{http.StatusNotExtended, http.StatusNetworkAuthenticationRequired},
-	},
-}
-
-// validateHTTPStatusCode validates the HTTP status code and returns
-// corresponding span status code. If the `code` is not a valid HTTP status
-// code, returns span status Error and false.
-func validateHTTPStatusCode(code int) (codes.Code, bool) {
-	category := code / 100
-	ranges, ok := validRangesPerCategory[category]
-	if !ok {
-		return codes.Error, false
-	}
-	ok = false
-	for _, crange := range ranges {
-		ok = crange.contains(code)
-		if ok {
-			break
-		}
-	}
-	if !ok {
-		return codes.Error, false
+	if code < 100 || code >= 600 {
+		return codes.Error, fmt.Sprintf("Invalid HTTP status code %d", code)
 	}
-	if category > 0 && category < 4 {
-		return codes.Unset, true
+	if code >= 500 {
+		return codes.Error, ""
 	}
-	return codes.Error, true
+	return codes.Unset, ""
 }
diff --git a/...entation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go b/...entation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go
@@ -332,7 +332,7 @@ func TestHTTPClientStatus(t *testing.T) {
 		{http.StatusSeeOther, codes.Unset, false},
 		{http.StatusNotModified, codes.Unset, false},
 		{http.StatusUseProxy, codes.Unset, false},
-		{306, codes.Error, true},
+		{306, codes.Unset, false},
 		{http.StatusTemporaryRedirect, codes.Unset, false},
 		{http.StatusPermanentRedirect, codes.Unset, false},
 		{http.StatusBadRequest, codes.Error, false},
@@ -364,6 +364,7 @@ func TestHTTPClientStatus(t *testing.T) {
 		{http.StatusTooManyRequests, codes.Error, false},
 		{http.StatusRequestHeaderFieldsTooLarge, codes.Error, false},
 		{http.StatusUnavailableForLegalReasons, codes.Error, false},
+		{499, codes.Error, false},
 		{http.StatusInternalServerError, codes.Error, false},
 		{http.StatusNotImplemented, codes.Error, false},
 		{http.StatusBadGateway, codes.Error, false},
@@ -379,13 +380,15 @@ func TestHTTPClientStatus(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		c, msg := HTTPClientStatus(test.code)
-		assert.Equal(t, test.stat, c)
-		if test.msg && msg == "" {
-			t.Errorf("expected non-empty message for %d", test.code)
-		} else if !test.msg && msg != "" {
-			t.Errorf("expected empty message for %d, got: %s", test.code, msg)
-		}
+		t.Run(strconv.Itoa(test.code), func(t *testing.T) {
+			c, msg := HTTPClientStatus(test.code)
+			assert.Equal(t, test.stat, c)
+			if test.msg && msg == "" {
+				t.Errorf("expected non-empty message for %d", test.code)
+			} else if !test.msg && msg != "" {
+				t.Errorf("expected empty message for %d, got: %s", test.code, msg)
+			}
+		})
 	}
 }
 
@@ -416,7 +419,7 @@ func TestHTTPServerStatus(t *testing.T) {
 		{http.StatusSeeOther, codes.Unset, false},
 		{http.StatusNotModified, codes.Unset, false},
 		{http.StatusUseProxy, codes.Unset, false},
-		{306, codes.Error, true},
+		{306, codes.Unset, false},
 		{http.StatusTemporaryRedirect, codes.Unset, false},
 		{http.StatusPermanentRedirect, codes.Unset, false},
 		{http.StatusBadRequest, codes.Unset, false},
@@ -448,6 +451,7 @@ func TestHTTPServerStatus(t *testing.T) {
 		{http.StatusTooManyRequests, codes.Unset, false},
 		{http.StatusRequestHeaderFieldsTooLarge, codes.Unset, false},
 		{http.StatusUnavailableForLegalReasons, codes.Unset, false},
+		{499, codes.Unset, false},
 		{http.StatusInternalServerError, codes.Error, false},
 		{http.StatusNotImplemented, codes.Error, false},
 		{http.StatusBadGateway, codes.Error, false},

diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go
@@ -437,83 +437,24 @@ func (c *httpConv) header(prefix string, h http.Header) []attribute.KeyValue {
 // ClientStatus returns a span status code and message for an HTTP status code
 // value received by a client.
 func (c *httpConv) ClientStatus(code int) (codes.Code, string) {
-	stat, valid := validateHTTPStatusCode(code)
-	if !valid {
-		return stat, fmt.Sprintf("Invalid HTTP status code %d", code)
+	if code < 100 || code >= 600 {
+		return codes.Error, fmt.Sprintf("Invalid HTTP status code %d", code)
 	}
-	return stat, ""
+	if code >= 400 {
+		return codes.Error, ""
+	}
+	return codes.Unset, ""
 }
 
 // ServerStatus returns a span status code and message for an HTTP status code
 // value returned by a server. Status codes in the 400-499 range are not
 // returned as errors.
 func (c *httpConv) ServerStatus(code int) (codes.Code, string) {
-	stat, valid := validateHTTPStatusCode(code)
-	if !valid {
-		return stat, fmt.Sprintf("Invalid HTTP status code %d", code)
-	}
-
-	if code/100 == 4 {
-		return codes.Unset, ""
-	}
-	return stat, ""
-}
-
-type codeRange struct {
-	fromInclusive int
-	toInclusive   int
-}
-
-func (r codeRange) contains(code int) bool {
-	return r.fromInclusive <= code && code <= r.toInclusive
-}
-
-var validRangesPerCategory = map[int][]codeRange{
-	1: {
-		{http.StatusContinue, http.StatusEarlyHints},
-	},
-	2: {
-		{http.StatusOK, http.StatusAlreadyReported},
-		{http.StatusIMUsed, http.StatusIMUsed},
-	},
-	3: {
-		{http.StatusMultipleChoices, http.StatusUseProxy},
-		{http.StatusTemporaryRedirect, http.StatusPermanentRedirect},
-	},
-	4: {
-		{http.StatusBadRequest, http.StatusTeapot}, // yes, teapot is so useful…
-		{http.StatusMisdirectedRequest, http.StatusUpgradeRequired},
-		{http.StatusPreconditionRequired, http.StatusTooManyRequests},
-		{http.StatusRequestHeaderFieldsTooLarge, http.StatusRequestHeaderFieldsTooLarge},
-		{http.StatusUnavailableForLegalReasons, http.StatusUnavailableForLegalReasons},
-	},
-	5: {
-		{http.StatusInternalServerError, http.StatusLoopDetected},
-		{http.StatusNotExtended, http.StatusNetworkAuthenticationRequired},
-	},
-}
-
-// validateHTTPStatusCode validates the HTTP status code and returns
-// corresponding span status code. If the `code` is not a valid HTTP status
-// code, returns span status Error and false.
-func validateHTTPStatusCode(code int) (codes.Code, bool) {
-	category := code / 100
-	ranges, ok := validRangesPerCategory[category]
-	if !ok {
-		return codes.Error, false
-	}
-	ok = false
-	for _, crange := range ranges {
-		ok = crange.contains(code)
-		if ok {
-			break
-		}
-	}
-	if !ok {
-		return codes.Error, false
+	if code < 100 || code >= 600 {
+		return codes.Error, fmt.Sprintf("Invalid HTTP status code %d", code)
 	}
-	if category > 0 && category < 4 {
-		return codes.Unset, true
+	if code >= 500 {
+		return codes.Error, ""
 	}
-	return codes.Error, true
+	return codes.Unset, ""
 }
diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go
@@ -332,7 +332,7 @@ func TestHTTPClientStatus(t *testing.T) {
 		{http.StatusSeeOther, codes.Unset, false},
 		{http.StatusNotModified, codes.Unset, false},
 		{http.StatusUseProxy, codes.Unset, false},
-		{306, codes.Error, true},
+		{306, codes.Unset, false},
 		{http.StatusTemporaryRedirect, codes.Unset, false},
 		{http.StatusPermanentRedirect, codes.Unset, false},
 		{http.StatusBadRequest, codes.Error, false},
@@ -364,6 +364,7 @@ func TestHTTPClientStatus(t *testing.T) {
 		{http.StatusTooManyRequests, codes.Error, false},
 		{http.StatusRequestHeaderFieldsTooLarge, codes.Error, false},
 		{http.StatusUnavailableForLegalReasons, codes.Error, false},
+		{499, codes.Error, false},
 		{http.StatusInternalServerError, codes.Error, false},
 		{http.StatusNotImplemented, codes.Error, false},
 		{http.StatusBadGateway, codes.Error, false},
@@ -379,13 +380,15 @@ func TestHTTPClientStatus(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		c, msg := HTTPClientStatus(test.code)
-		assert.Equal(t, test.stat, c)
-		if test.msg && msg == "" {
-			t.Errorf("expected non-empty message for %d", test.code)
-		} else if !test.msg && msg != "" {
-			t.Errorf("expected empty message for %d, got: %s", test.code, msg)
-		}
+		t.Run(strconv.Itoa(test.code), func(t *testing.T) {
+			c, msg := HTTPClientStatus(test.code)
+			assert.Equal(t, test.stat, c)
+			if test.msg && msg == "" {
+				t.Errorf("expected non-empty message for %d", test.code)
+			} else if !test.msg && msg != "" {
+				t.Errorf("expected empty message for %d, got: %s", test.code, msg)
+			}
+		})
 	}
 }
 
@@ -416,7 +419,7 @@ func TestHTTPServerStatus(t *testing.T) {
 		{http.StatusSeeOther, codes.Unset, false},
 		{http.StatusNotModified, codes.Unset, false},
 		{http.StatusUseProxy, codes.Unset, false},
-		{306, codes.Error, true},
+		{306, codes.Unset, false},
 		{http.StatusTemporaryRedirect, codes.Unset, false},
 		{http.StatusPermanentRedirect, codes.Unset, false},
 		{http.StatusBadRequest, codes.Unset, false},
@@ -448,6 +451,7 @@ func TestHTTPServerStatus(t *testing.T) {
 		{http.StatusTooManyRequests, codes.Unset, false},
 		{http.StatusRequestHeaderFieldsTooLarge, codes.Unset, false},
 		{http.StatusUnavailableForLegalReasons, codes.Unset, false},
+		{499, codes.Unset, false},
 		{http.StatusInternalServerError, codes.Error, false},
 		{http.StatusNotImplemented, codes.Error, false},
 		{http.StatusBadGateway, codes.Error, false},