From 080a7dd06f5906f55bdaccf53242d2b0f8966089 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Wed, 15 Nov 2017 11:26:25 +0100 Subject: [PATCH] Fix review comments Signed-off-by: Pavol Loffay --- cmd/collector/app/zipkin/http_handler.go | 9 +++++---- cmd/collector/app/zipkin/http_handler_test.go | 5 ++--- cmd/collector/app/zipkin/jsonv2.go | 13 ++++++------- cmd/collector/app/zipkin/jsonv2_test.go | 8 ++++---- cmd/collector/main.go | 5 +---- cmd/standalone/main.go | 6 +----- 6 files changed, 19 insertions(+), 27 deletions(-) diff --git a/cmd/collector/app/zipkin/http_handler.go b/cmd/collector/app/zipkin/http_handler.go index 0d15e4b7c11..88efb286da7 100644 --- 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 index 62fd1a23107..fee8f4d9ffe 100644 --- 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 index 0e67b70f022..cb2156b8fc3 100644 --- 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 index 1f94d2e6b37..0272614e5be 100644 --- 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 index 6d609893ce0..10f478fc79b 100644 --- a/cmd/collector/main.go +++ b/cmd/collector/main.go @@ -164,10 +164,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 index 7934c047211..e7ae727c494 100644 --- a/cmd/standalone/main.go +++ b/cmd/standalone/main.go @@ -186,11 +186,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))