chore(zipkin-exporter): relay on the status code for the request but still read the response body. (open-telemetry#1328)

* chore(zipkin-exporter): relay on the status code for the request but still read the response body.

* fix(zipkin-exporter): fix tests.

* chore(zipkin-exporter): adds changelog.

* chore: 202 -> http.StatusAccepted

Co-authored-by: Chris Bandy <[email protected]>

* chore: 202 -> http.StatusAccepted

Co-authored-by: Anthony Mirabella <[email protected]>

Co-authored-by: Chris Bandy <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>

Author: 4 people

CHANGELOG.md

@@ -12,6 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - Move the OpenCensus example into `example` directory. (#1359)
 - `NewExporter` and `Start` functions in `go.opentelemetry.io/otel/exporters/otlp` now receive `context.Context` as a first parameter. (#1357)
+- Zipkin exporter relies on the status code for success rather than body read but still read the response body. (#1328)
 
 ## [0.14.0] - 2020-11-19
 

exporters/trace/zipkin/zipkin.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"log"
 	"net/http"
@@ -112,14 +113,14 @@ func NewRawExporter(collectorURL, serviceName string, opts ...Option) (*Exporter
 // NewExportPipeline sets up a complete export pipeline
 // with the recommended setup for trace provider
 func NewExportPipeline(collectorURL, serviceName string, opts ...Option) (*sdktrace.TracerProvider, error) {
-	exp, err := NewRawExporter(collectorURL, serviceName, opts...)
+	exporter, err := NewRawExporter(collectorURL, serviceName, opts...)
 	if err != nil {
 		return nil, err
 	}
 
-	tp := sdktrace.NewTracerProvider(sdktrace.WithBatcher(exp))
-	if exp.o.config != nil {
-		tp.ApplyConfig(*exp.o.config)
+	tp := sdktrace.NewTracerProvider(sdktrace.WithBatcher(exporter))
+	if exporter.o.config != nil {
+		tp.ApplyConfig(*exporter.o.config)
 	}
 
 	return tp, err
@@ -166,19 +167,21 @@ func (e *Exporter) ExportSpans(ctx context.Context, batch []*export.SpanData) er
 	if err != nil {
 		return e.errf("request to %s failed: %v", e.url, err)
 	}
-	e.logf("zipkin responded with status %d", resp.StatusCode)
+	defer resp.Body.Close()
 
-	_, err = ioutil.ReadAll(resp.Body)
+	// Zipkin API returns a 202 on success and the content of the body isn't interesting
+	// but it is still being read because according to https://golang.org/pkg/net/http/#Response
+	// > The default HTTP client's Transport may not reuse HTTP/1.x "keep-alive" TCP connections
+	// > if the Body is not read to completion and closed.
+	_, err = io.Copy(ioutil.Discard, resp.Body)
 	if err != nil {
-		// Best effort to clean up here.
-		resp.Body.Close()
 		return e.errf("failed to read response body: %v", err)
 	}
 
-	err = resp.Body.Close()
-	if err != nil {
-		return e.errf("failed to close response body: %v", err)
+	if resp.StatusCode != http.StatusAccepted {
+		return e.errf("failed to send spans to zipkin server with status %d", resp.StatusCode)
 	}
+
 	return nil
 }
 

exporters/trace/zipkin/zipkin_test.go

@@ -192,6 +192,7 @@ func (c *mockZipkinCollector) handler(w http.ResponseWriter, r *http.Request) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	c.models = append(c.models, models...)
+	w.WriteHeader(http.StatusAccepted)
 }
 
 func (c *mockZipkinCollector) Close() {
@@ -340,17 +341,15 @@ func TestExportSpans(t *testing.T) {
 	ctx := context.Background()
 	require.Len(t, ls.Messages, 0)
 	require.NoError(t, exporter.ExportSpans(ctx, spans[0:1]))
-	require.Len(t, ls.Messages, 2)
+	require.Len(t, ls.Messages, 1)
 	require.Contains(t, ls.Messages[0], "send a POST request")
-	require.Contains(t, ls.Messages[1], "zipkin responded")
 	ls.Messages = nil
 	require.NoError(t, exporter.ExportSpans(ctx, nil))
 	require.Len(t, ls.Messages, 1)
 	require.Contains(t, ls.Messages[0], "no spans to export")
 	ls.Messages = nil
 	require.NoError(t, exporter.ExportSpans(ctx, spans[1:2]))
 	require.Contains(t, ls.Messages[0], "send a POST request")
-	require.Contains(t, ls.Messages[1], "zipkin responded")
 	checkFunc := func() bool {
 		return collector.ModelsLen() == len(models)
 	}