Fix review comments

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
jaegertracing · Nov 15, 2017 · ca70179 · ca70179
1 parent 43b8461
commit ca70179
Show file tree

Hide file tree

Showing 6 changed files with 19 additions and 27 deletions.
diff --git a/cmd/collector/app/zipkin/http_handler.go b/cmd/collector/app/zipkin/http_handler.go
@@ -46,12 +46,12 @@ type APIHandler struct {
 // NewAPIHandler returns a new APIHandler
 func NewAPIHandler(
 	zipkinSpansHandler app.ZipkinSpansHandler,
-) (*APIHandler, error) {
+) *APIHandler {
 	swaggerSpec, _ := loads.Analyzed(restapi.SwaggerJSON, "")
 	return &APIHandler{
 		zipkinSpansHandler: zipkinSpansHandler,
 		zipkinV2Formats:    operations.NewZipkinAPI(swaggerSpec).Formats(),
-	}, nil
+	}
 }
 
 // RegisterRoutes registers Zipkin routes
@@ -69,6 +69,7 @@ func (aH *APIHandler) saveSpans(w http.ResponseWriter, r *http.Request) {
 			http.Error(w, fmt.Sprintf(app.UnableToReadBodyErrFormat, err), http.StatusBadRequest)
 			return
 		}
+		defer gz.Close()
 		bRead = gz
 	}
 
@@ -110,6 +111,7 @@ func (aH *APIHandler) saveSpansV2(w http.ResponseWriter, r *http.Request) {
 			http.Error(w, fmt.Sprintf(app.UnableToReadBodyErrFormat, err), http.StatusBadRequest)
 			return
 		}
+		defer gz.Close()
 		bRead = gz
 	}
 
@@ -135,7 +137,7 @@ func (aH *APIHandler) saveSpansV2(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	tSpans, err := spansV2ToThrift(&spans)
+	tSpans, err := spansV2ToThrift(spans)
 	if err != nil {
 		http.Error(w, fmt.Sprintf(app.UnableToReadBodyErrFormat, err), http.StatusBadRequest)
 		return
@@ -154,7 +156,6 @@ func gunzip(r io.ReadCloser) (*gzip.Reader, error) {
 	if err != nil {
 		return nil, err
 	}
-	defer gz.Close()
 	return gz, nil
 }
 

diff --git a/cmd/collector/app/zipkin/http_handler_test.go b/cmd/collector/app/zipkin/http_handler_test.go
@@ -59,7 +59,7 @@ func (p *mockZipkinHandler) getSpans() []*zipkincore.Span {
 
 func initializeTestServer(err error) (*httptest.Server, *APIHandler) {
 	r := mux.NewRouter()
-	handler, _ := NewAPIHandler(&mockZipkinHandler{err: err})
+	handler := NewAPIHandler(&mockZipkinHandler{err: err})
 	handler.RegisterRoutes(r)
 	return httptest.NewServer(r), handler
 }
@@ -217,8 +217,7 @@ func TestDeserializeWithBadListStart(t *testing.T) {
 }
 
 func TestCannotReadBodyFromRequest(t *testing.T) {
-	handler, err := NewAPIHandler(&mockZipkinHandler{})
-	require.NoError(t, err)
+	handler := NewAPIHandler(&mockZipkinHandler{})
 	req, err := http.NewRequest(http.MethodPost, "whatever", &errReader{})
 	assert.NoError(t, err)
 	rw := dummyResponseWriter{}

diff --git a/cmd/collector/app/zipkin/jsonv2.go b/cmd/collector/app/zipkin/jsonv2.go
@@ -20,10 +20,10 @@ import (
 	"github.com/jaegertracing/jaeger/thrift-gen/zipkincore"
 )
 
-func spansV2ToThrift(spans *models.ListOfSpans) ([]*zipkincore.Span, error) {
+func spansV2ToThrift(spans models.ListOfSpans) ([]*zipkincore.Span, error) {
 	var tSpans []*zipkincore.Span
-	for _, span := range *spans {
-		tSpan, err := spanV2ToThrift(*span)
+	for _, span := range spans {
+		tSpan, err := spanV2ToThrift(span)
 		if err != nil {
 			return nil, err
 		}
@@ -32,7 +32,7 @@ func spansV2ToThrift(spans *models.ListOfSpans) ([]*zipkincore.Span, error) {
 	return tSpans, nil
 }
 
-func spanV2ToThrift(s models.Span) (*zipkincore.Span, error) {
+func spanV2ToThrift(s *models.Span) (*zipkincore.Span, error) {
 	id, err := model.SpanIDFromString(cutLongID(*s.ID))
 	if err != nil {
 		return nil, err
@@ -156,16 +156,15 @@ func endpointV2ToThrift(e *models.Endpoint) (*zipkincore.Endpoint, error) {
 }
 
 func annoV2ToThrift(a *models.Annotation, e *zipkincore.Endpoint) *zipkincore.Annotation {
-	ta := &zipkincore.Annotation{
+	return &zipkincore.Annotation{
 		Value:     a.Value,
 		Timestamp: a.Timestamp,
 		Host:      e,
 	}
-	return ta
 }
 
 func tagsToThrift(tags models.Tags, localE *zipkincore.Endpoint) []*zipkincore.BinaryAnnotation {
-	var bAnnos []*zipkincore.BinaryAnnotation
+	bAnnos := make([]*zipkincore.BinaryAnnotation, 0, len(tags))
 	for k, v := range tags {
 		ba := &zipkincore.BinaryAnnotation{
 			Key:            k,

diff --git a/cmd/collector/app/zipkin/jsonv2_test.go b/cmd/collector/app/zipkin/jsonv2_test.go
@@ -30,7 +30,7 @@ import (
 func TestFixtures(t *testing.T) {
 	var spans models.ListOfSpans
 	loadJSON(t, fmt.Sprintf("fixtures/zipkin_01.json"), &spans)
-	tSpans, err := spansV2ToThrift(&spans)
+	tSpans, err := spansV2ToThrift(spans)
 	require.NoError(t, err)
 	assert.Equal(t, len(tSpans), 1)
 	var pid int64 = 1
@@ -95,7 +95,7 @@ func TestErrIds(t *testing.T) {
 		{span: models.Span{ID: &idOk, TraceID: &idOk, ParentID: idWrong}},
 	}
 	for _, test := range tests {
-		tSpan, err := spanV2ToThrift(test.span)
+		tSpan, err := spanV2ToThrift(&test.span)
 		require.Error(t, err)
 		require.Nil(t, tSpan)
 		assert.Equal(t, err.Error(), "strconv.ParseUint: parsing \"z\": invalid syntax")
@@ -112,7 +112,7 @@ func TestErrEndpoints(t *testing.T) {
 		{span: models.Span{ID: &id, TraceID: &id, RemoteEndpoint: &endp}},
 	}
 	for _, test := range tests {
-		tSpan, err := spanV2ToThrift(test.span)
+		tSpan, err := spanV2ToThrift(&test.span)
 		require.Error(t, err)
 		require.Nil(t, tSpan)
 		assert.Equal(t, err.Error(), "wrong ipv4")
@@ -121,7 +121,7 @@ func TestErrEndpoints(t *testing.T) {
 
 func TestErrSpans(t *testing.T) {
 	id := "z"
-	tSpans, err := spansV2ToThrift(&models.ListOfSpans{&models.Span{ID: &id}})
+	tSpans, err := spansV2ToThrift(models.ListOfSpans{&models.Span{ID: &id}})
 	require.Error(t, err)
 	require.Nil(t, tSpans)
 	assert.Equal(t, err.Error(), "strconv.ParseUint: parsing \"z\": invalid syntax")

diff --git a/cmd/collector/main.go b/cmd/collector/main.go
@@ -154,10 +154,7 @@ func startZipkinHTTPAPI(
 	recoveryHandler func(http.Handler) http.Handler,
 ) {
 	if zipkinPort != 0 {
-		zHandler, err := zipkin.NewAPIHandler(zipkinSpansHandler)
-		if err != nil {
-			logger.Fatal("Failed to initialize Zipkin handler", zap.Error(err))
-		}
+		zHandler := zipkin.NewAPIHandler(zipkinSpansHandler)
 		r := mux.NewRouter()
 		zHandler.RegisterRoutes(r)
 

diff --git a/cmd/standalone/main.go b/cmd/standalone/main.go
@@ -178,11 +178,7 @@ func startZipkinHTTPAPI(
 ) {
 	if zipkinPort != 0 {
 		r := mux.NewRouter()
-		zHandler, err := zipkin.NewAPIHandler(zipkinSpansHandler)
-		if err != nil {
-			logger.Fatal("Failed to initialize Zipkin handler", zap.Error(err))
-		}
-
+		zHandler := zipkin.NewAPIHandler(zipkinSpansHandler)
 		zHandler.RegisterRoutes(r)
 		httpPortStr := ":" + strconv.Itoa(zipkinPort)
 		logger.Info("Listening for Zipkin HTTP traffic", zap.Int("zipkin.http-port", zipkinPort))