Use server span when determining status from code (#1973)

* Use server span when determining status from code

Switches to `SpanStatusFromHTTPStatusCodeAndSpanKind` with a span kind
of `SpanKindServer` so that 4xx status codes don't get labelled as
errors. Since these codes represent client errors, they make it
difficult to differentiate bad server behavior from bad client behavior.

* Add CHANGELOG entry

* Add test to verify that 4xx does not set error on span

Adds a test to verify that the span created by a request that returns
status "not found" is not an error span.

* Test that span code is explicitly unset

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

CHANGELOG.md

@@ -9,6 +9,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## [Unreleased]
 
 ### Fixed
+
+- Fix the `otelmux` middleware by using `SpanKindServer` when deciding the `SpanStatus`.
+  This makes `4xx` response codes to not be an error anymore. (#1973)
 - Fixed jaegerremote sampler not behaving properly with per operation strategy set. (#2137)
 
 ## [1.6.0/0.31.0] - 2022-03-28

instrumentation/github.com/gorilla/mux/otelmux/mux.go

@@ -144,7 +144,7 @@ func (tw traceware) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	defer putRRW(rrw)
 	tw.handler.ServeHTTP(rrw.writer, r2)
 	attrs := semconv.HTTPAttributesFromHTTPStatusCode(rrw.status)
-	spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCode(rrw.status)
+	spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCodeAndSpanKind(rrw.status, oteltrace.SpanKindServer)
 	span.SetAttributes(attrs...)
 	span.SetStatus(spanStatus, spanMessage)
 }

instrumentation/github.com/gorilla/mux/otelmux/test/mux_test.go

@@ -25,12 +25,16 @@ import (
 
 	"go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux"
 	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/codes"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 	"go.opentelemetry.io/otel/sdk/trace/tracetest"
 	"go.opentelemetry.io/otel/trace"
 )
 
 func ok(w http.ResponseWriter, _ *http.Request) {}
+func notfound(w http.ResponseWriter, _ *http.Request) {
+	http.Error(w, "not found", http.StatusNotFound)
+}
 
 func TestSDKIntegration(t *testing.T) {
 	sr := tracetest.NewSpanRecorder()
@@ -69,6 +73,33 @@ func TestSDKIntegration(t *testing.T) {
 	)
 }
 
+func TestNotFoundIsNotError(t *testing.T) {
+	sr := tracetest.NewSpanRecorder()
+	provider := sdktrace.NewTracerProvider()
+	provider.RegisterSpanProcessor(sr)
+
+	router := mux.NewRouter()
+	router.Use(otelmux.Middleware("foobar", otelmux.WithTracerProvider(provider)))
+	router.HandleFunc("/does/not/exist", notfound)
+
+	r0 := httptest.NewRequest("GET", "/does/not/exist", nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, r0)
+
+	require.Len(t, sr.Ended(), 1)
+	assertSpan(t, sr.Ended()[0],
+		"/does/not/exist",
+		trace.SpanKindServer,
+		attribute.String("http.server_name", "foobar"),
+		attribute.Int("http.status_code", http.StatusNotFound),
+		attribute.String("http.method", "GET"),
+		attribute.String("http.target", "/does/not/exist"),
+		attribute.String("http.route", "/does/not/exist"),
+	)
+	assert.Equal(t, sr.Ended()[0].Status().Code, codes.Unset)
+
+}
+
 func assertSpan(t *testing.T, span sdktrace.ReadOnlySpan, name string, kind trace.SpanKind, attrs ...attribute.KeyValue) {
 	assert.Equal(t, name, span.Name())
 	assert.Equal(t, trace.SpanKindServer, span.SpanKind())