From eaa94e92b93a85d204940e17624d0c52ad633f19 Mon Sep 17 00:00:00 2001
From: Anthony Mirabella <a9@aneurysm9.com>
Date: Tue, 7 Jul 2020 12:00:21 -0400
Subject: [PATCH] Avoid setting span status to Unknown when no HTTP status is
 provided; stdlib assumes it to be `200 OK` (#908)

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
---
 CHANGELOG.md                           |  1 +
 instrumentation/othttp/handler.go      |  2 +-
 instrumentation/othttp/handler_test.go | 43 +++++++++++++++++++++++++-
 instrumentation/othttp/wrap.go         |  1 -
 internal/trace/mock_span.go            | 10 ++++--
 5 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1b5b6dc82b5..d8ac721a546 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ### Fixed
 
 - The B3 Single Header name is now correctly `b3` instead of the previous `X-B3`. (#881)
+- Ensure span status is not set to `Unknown` when no HTTP status code is provided as it is assumed to be `200 OK`. (#908)
 
 ## [0.7.0] - 2020-06-26
 
diff --git a/instrumentation/othttp/handler.go b/instrumentation/othttp/handler.go
index 5a17c6d5082..3353057b8c9 100644
--- a/instrumentation/othttp/handler.go
+++ b/instrumentation/othttp/handler.go
@@ -155,7 +155,6 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	h.handler.ServeHTTP(rww, r.WithContext(ctx))
 
 	setAfterServeAttributes(span, bw.read, rww.written, rww.statusCode, bw.err, rww.err)
-	span.SetStatus(standard.SpanStatusFromHTTPStatusCode(rww.statusCode))
 
 	// Add request metrics
 
@@ -185,6 +184,7 @@ func setAfterServeAttributes(span trace.Span, read, wrote int64, statusCode int,
 	}
 	if statusCode > 0 {
 		kv = append(kv, standard.HTTPAttributesFromHTTPStatusCode(statusCode)...)
+		span.SetStatus(standard.SpanStatusFromHTTPStatusCode(statusCode))
 	}
 	if werr != nil && werr != io.EOF {
 		kv = append(kv, WriteErrorKey.String(werr.Error()))
diff --git a/instrumentation/othttp/handler_test.go b/instrumentation/othttp/handler_test.go
index 0d89904db7b..eaf2723654a 100644
--- a/instrumentation/othttp/handler_test.go
+++ b/instrumentation/othttp/handler_test.go
@@ -22,9 +22,11 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/grpc/codes"
 
 	"go.opentelemetry.io/otel/api/kv"
 	"go.opentelemetry.io/otel/api/standard"
+	"go.opentelemetry.io/otel/api/trace"
 	mockmeter "go.opentelemetry.io/otel/internal/metric"
 	mocktrace "go.opentelemetry.io/otel/internal/trace"
 )
@@ -71,7 +73,6 @@ func TestHandlerBasics(t *testing.T) {
 		standard.HTTPFlavorKey.String(fmt.Sprintf("1.%d", r.ProtoMinor)),
 	}
 
-	standard.HTTPServerMetricAttributesFromHTTPRequest(operation, r)
 	assertMetricLabels(t, labelsToVerify, meterimpl.MeasurementBatches)
 
 	if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected {
@@ -91,3 +92,43 @@ func TestHandlerBasics(t *testing.T) {
 		t.Fatalf("got %q, expected %q", got, expected)
 	}
 }
+
+func TestHandlerNoWrite(t *testing.T) {
+	rr := httptest.NewRecorder()
+
+	var id uint64
+	tracer := mocktrace.MockTracer{StartSpanID: &id}
+
+	operation := "test_handler"
+	var span trace.Span
+
+	h := NewHandler(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			span = trace.SpanFromContext(r.Context())
+		}), operation,
+		WithTracer(&tracer),
+	)
+
+	r, err := http.NewRequest(http.MethodGet, "http://localhost/", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	h.ServeHTTP(rr, r)
+
+	if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected {
+		t.Fatalf("got %d, expected %d", got, expected)
+	}
+	if got := rr.Header().Get("Traceparent"); got != "" {
+		t.Fatal("expected empty trace header")
+	}
+	if got, expected := id, uint64(1); got != expected {
+		t.Fatalf("got %d, expected %d", got, expected)
+	}
+	if mockSpan, ok := span.(*mocktrace.MockSpan); ok {
+		if got, expected := mockSpan.Status, codes.OK; got != expected {
+			t.Fatalf("got %q, expected %q", got, expected)
+		}
+	} else {
+		t.Fatalf("Expected *moctrace.MockSpan, got %T", span)
+	}
+}
diff --git a/instrumentation/othttp/wrap.go b/instrumentation/othttp/wrap.go
index 02d9f74ed98..e1575457b29 100644
--- a/instrumentation/othttp/wrap.go
+++ b/instrumentation/othttp/wrap.go
@@ -76,7 +76,6 @@ func (w *respWriterWrapper) Header() http.Header {
 func (w *respWriterWrapper) Write(p []byte) (int, error) {
 	if !w.wroteHeader {
 		w.WriteHeader(http.StatusOK)
-		w.wroteHeader = true
 	}
 	n, err := w.ResponseWriter.Write(p)
 	n1 := int64(n)
diff --git a/internal/trace/mock_span.go b/internal/trace/mock_span.go
index a86653fea6e..7a7addba563 100644
--- a/internal/trace/mock_span.go
+++ b/internal/trace/mock_span.go
@@ -26,9 +26,11 @@ import (
 
 // MockSpan is a mock span used in association with MockTracer for testing purpose only.
 type MockSpan struct {
-	sc     apitrace.SpanContext
-	tracer apitrace.Tracer
-	Name   string
+	sc        apitrace.SpanContext
+	tracer    apitrace.Tracer
+	Name      string
+	Status    codes.Code
+	StatusMsg string
 }
 
 var _ apitrace.Span = (*MockSpan)(nil)
@@ -49,6 +51,8 @@ func (ms *MockSpan) IsRecording() bool {
 
 // SetStatus does nothing.
 func (ms *MockSpan) SetStatus(status codes.Code, msg string) {
+	ms.Status = status
+	ms.StatusMsg = msg
 }
 
 // SetError does nothing.