From b98364db6d1a7cf8c883aa92f07acfb3216f7261 Mon Sep 17 00:00:00 2001 From: Annanay Date: Fri, 1 Feb 2019 23:25:35 +0530 Subject: [PATCH 01/17] [query] Separate common utils of query-service from APIHandler Signed-off-by: Annanay --- cmd/query/app/handler.go | 58 +++++++++++++++----------------- cmd/query/app/handler_options.go | 10 +++--- cmd/query/app/query_service.go | 35 +++++++++++++++++++ 3 files changed, 67 insertions(+), 36 deletions(-) create mode 100644 cmd/query/app/query_service.go diff --git a/cmd/query/app/handler.go b/cmd/query/app/handler.go index 512dcde1a76..71f6e9ab444 100644 --- a/cmd/query/app/handler.go +++ b/cmd/query/app/handler.go @@ -78,23 +78,19 @@ func NewRouter() *mux.Router { // APIHandler implements the query service public API by registering routes at httpPrefix type APIHandler struct { - spanReader spanstore.Reader - archiveSpanReader spanstore.Reader - archiveSpanWriter spanstore.Writer - dependencyReader dependencystore.Reader - adjuster adjuster.Adjuster - logger *zap.Logger - queryParser queryParser - basePath string - apiPrefix string - tracer opentracing.Tracer + queryService QueryService + queryParser queryParser + basePath string + apiPrefix string } // NewAPIHandler returns an APIHandler func NewAPIHandler(spanReader spanstore.Reader, dependencyReader dependencystore.Reader, options ...HandlerOption) *APIHandler { aH := &APIHandler{ - spanReader: spanReader, - dependencyReader: dependencyReader, + queryService: QueryService{ + spanReader: spanReader, + dependencyReader: dependencyReader, + }, queryParser: queryParser{ traceQueryLookbackDuration: defaultTraceQueryLookbackDuration, timeNow: time.Now, @@ -107,14 +103,14 @@ func NewAPIHandler(spanReader spanstore.Reader, dependencyReader dependencystore if aH.apiPrefix == "" { aH.apiPrefix = defaultAPIPrefix } - if aH.adjuster == nil { - aH.adjuster = adjuster.Sequence(StandardAdjusters...) + if aH.queryService.adjuster == nil { + aH.queryService.adjuster = adjuster.Sequence(StandardAdjusters...) } - if aH.logger == nil { - aH.logger = zap.NewNop() + if aH.queryService.logger == nil { + aH.queryService.logger = zap.NewNop() } - if aH.tracer == nil { - aH.tracer = opentracing.NoopTracer{} + if aH.queryService.tracer == nil { + aH.queryService.tracer = opentracing.NoopTracer{} } return aH } @@ -140,7 +136,7 @@ func (aH *APIHandler) handleFunc( ) *mux.Route { route = aH.route(route, args...) traceMiddleware := nethttp.Middleware( - aH.tracer, + aH.queryService.tracer, http.HandlerFunc(f), nethttp.OperationNameFunc(func(r *http.Request) string { return route @@ -154,7 +150,7 @@ func (aH *APIHandler) route(route string, args ...interface{}) string { } func (aH *APIHandler) getServices(w http.ResponseWriter, r *http.Request) { - services, err := aH.spanReader.GetServices(r.Context()) + services, err := aH.queryService.spanReader.GetServices(r.Context()) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -169,7 +165,7 @@ func (aH *APIHandler) getOperationsLegacy(w http.ResponseWriter, r *http.Request vars := mux.Vars(r) // given how getOperationsLegacy is bound to URL route, serviceParam cannot be empty service, _ := url.QueryUnescape(vars[serviceParam]) - operations, err := aH.spanReader.GetOperations(r.Context(), service) + operations, err := aH.queryService.spanReader.GetOperations(r.Context(), service) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -187,7 +183,7 @@ func (aH *APIHandler) getOperations(w http.ResponseWriter, r *http.Request) { return } } - operations, err := aH.spanReader.GetOperations(r.Context(), service) + operations, err := aH.queryService.spanReader.GetOperations(r.Context(), service) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -212,7 +208,7 @@ func (aH *APIHandler) search(w http.ResponseWriter, r *http.Request) { return } } else { - tracesFromStorage, err = aH.spanReader.FindTraces(r.Context(), &tQuery.TraceQueryParameters) + tracesFromStorage, err = aH.queryService.spanReader.FindTraces(r.Context(), &tQuery.TraceQueryParameters) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -238,7 +234,7 @@ func (aH *APIHandler) tracesByIDs(ctx context.Context, traceIDs []model.TraceID) var errors []structuredError retMe := make([]*model.Trace, 0, len(traceIDs)) for _, traceID := range traceIDs { - if trace, err := trace(ctx, traceID, aH.spanReader, aH.archiveSpanReader); err != nil { + if trace, err := trace(ctx, traceID, aH.queryService.spanReader, aH.queryService.archiveSpanReader); err != nil { if err != spanstore.ErrTraceNotFound { return nil, nil, err } @@ -272,7 +268,7 @@ func (aH *APIHandler) dependencies(w http.ResponseWriter, r *http.Request) { } endTs := time.Unix(0, 0).Add(time.Duration(endTsMillis) * time.Millisecond) - dependencies, err := aH.dependencyReader.GetDependencies(endTs, lookback) + dependencies, err := aH.queryService.dependencyReader.GetDependencies(endTs, lookback) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -288,7 +284,7 @@ func (aH *APIHandler) convertModelToUI(trace *model.Trace, adjust bool) (*ui.Tra var errors []error if adjust { var err error - trace, err = aH.adjuster.Adjust(trace) + trace, err = aH.queryService.adjuster.Adjust(trace) if err != nil { errors = append(errors, err) } @@ -353,7 +349,7 @@ func (aH *APIHandler) parseTraceID(w http.ResponseWriter, r *http.Request) (mode // getTrace implements the REST API /traces/{trace-id} func (aH *APIHandler) getTrace(w http.ResponseWriter, r *http.Request) { - aH.getTraceFromReaders(w, r, aH.spanReader, aH.archiveSpanReader) + aH.getTraceFromReaders(w, r, aH.queryService.spanReader, aH.queryService.archiveSpanReader) } // getTraceFromReader parses trace ID from the path, loads the trace from specified Reader, @@ -430,14 +426,14 @@ func trace( // archiveTrace implements the REST API POST:/archive/{trace-id}. // It reads the trace from the main Reader and saves it to archive Writer. func (aH *APIHandler) archiveTrace(w http.ResponseWriter, r *http.Request) { - if aH.archiveSpanWriter == nil { + if aH.queryService.archiveSpanWriter == nil { aH.handleError(w, errNoArchiveSpanStorage, http.StatusInternalServerError) return } - aH.withTraceFromReader(w, r, aH.spanReader, nil, func(trace *model.Trace) { + aH.withTraceFromReader(w, r, aH.queryService.spanReader, nil, func(trace *model.Trace) { var writeErrors []error for _, span := range trace.Spans { - err := aH.archiveSpanWriter.WriteSpan(span) + err := aH.queryService.archiveSpanWriter.WriteSpan(span) if err != nil { writeErrors = append(writeErrors, err) } @@ -459,7 +455,7 @@ func (aH *APIHandler) handleError(w http.ResponseWriter, err error, statusCode i return false } if statusCode == http.StatusInternalServerError { - aH.logger.Error("HTTP handler, Internal Server Error", zap.Error(err)) + aH.queryService.logger.Error("HTTP handler, Internal Server Error", zap.Error(err)) } structuredResp := structuredResponse{ Errors: []structuredError{ diff --git a/cmd/query/app/handler_options.go b/cmd/query/app/handler_options.go index 49cec6feec0..4c39a7e771e 100644 --- a/cmd/query/app/handler_options.go +++ b/cmd/query/app/handler_options.go @@ -36,14 +36,14 @@ type handlerOptions struct{} // which is used to emit logs. func (handlerOptions) Logger(logger *zap.Logger) HandlerOption { return func(apiHandler *APIHandler) { - apiHandler.logger = logger + apiHandler.queryService.logger = logger } } // Adjusters creates a HandlerOption that initializes the sequence of Adjusters on the APIHandler, func (handlerOptions) Adjusters(adjusters ...adjuster.Adjuster) HandlerOption { return func(apiHandler *APIHandler) { - apiHandler.adjuster = adjuster.Sequence(adjusters...) + apiHandler.queryService.adjuster = adjuster.Sequence(adjusters...) } } @@ -71,20 +71,20 @@ func (handlerOptions) QueryLookbackDuration(queryLookbackDuration time.Duration) // ArchiveSpanReader creates a HandlerOption that initializes lookback duration func (handlerOptions) ArchiveSpanReader(reader spanstore.Reader) HandlerOption { return func(apiHandler *APIHandler) { - apiHandler.archiveSpanReader = reader + apiHandler.queryService.archiveSpanReader = reader } } // ArchiveSpanWriter creates a HandlerOption that initializes lookback duration func (handlerOptions) ArchiveSpanWriter(writer spanstore.Writer) HandlerOption { return func(apiHandler *APIHandler) { - apiHandler.archiveSpanWriter = writer + apiHandler.queryService.archiveSpanWriter = writer } } // Tracer creates a HandlerOption that initializes OpenTracing tracer func (handlerOptions) Tracer(tracer opentracing.Tracer) HandlerOption { return func(apiHandler *APIHandler) { - apiHandler.tracer = tracer + apiHandler.queryService.tracer = tracer } } diff --git a/cmd/query/app/query_service.go b/cmd/query/app/query_service.go new file mode 100644 index 00000000000..73672af32ee --- /dev/null +++ b/cmd/query/app/query_service.go @@ -0,0 +1,35 @@ +// Copyright (c) 2019 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package app + +import ( + "github.com/opentracing/opentracing-go" + "go.uber.org/zap" + + "github.com/jaegertracing/jaeger/model/adjuster" + "github.com/jaegertracing/jaeger/storage/dependencystore" + "github.com/jaegertracing/jaeger/storage/spanstore" +) + +// QueryService contains span utils required by the query-service. +type QueryService struct { + spanReader spanstore.Reader + archiveSpanReader spanstore.Reader + archiveSpanWriter spanstore.Writer + dependencyReader dependencystore.Reader + adjuster adjuster.Adjuster + logger *zap.Logger + tracer opentracing.Tracer +} From ea173da270a17c9c3771c691134a1687dc03c55c Mon Sep 17 00:00:00 2001 From: Annanay Date: Sun, 3 Feb 2019 00:55:34 +0530 Subject: [PATCH 02/17] Make a new querysvc to encapsulate query-service utilities Signed-off-by: Annanay --- cmd/query/app/handler_options.go | 28 +---- cmd/query/app/{handler.go => http_handler.go} | 69 ++++------- .../{handler_test.go => http_handler_test.go} | 7 +- cmd/query/app/query_service.go | 35 ------ cmd/query/app/{ => querysvc}/adjusters.go | 2 +- cmd/query/app/querysvc/query_service.go | 111 ++++++++++++++++++ .../app/querysvc/query_service_options.go | 49 ++++++++ 7 files changed, 189 insertions(+), 112 deletions(-) rename cmd/query/app/{handler.go => http_handler.go} (85%) rename cmd/query/app/{handler_test.go => http_handler_test.go} (98%) delete mode 100644 cmd/query/app/query_service.go rename cmd/query/app/{ => querysvc}/adjusters.go (98%) create mode 100644 cmd/query/app/querysvc/query_service.go create mode 100644 cmd/query/app/querysvc/query_service_options.go diff --git a/cmd/query/app/handler_options.go b/cmd/query/app/handler_options.go index 4c39a7e771e..aa458c5f5f0 100644 --- a/cmd/query/app/handler_options.go +++ b/cmd/query/app/handler_options.go @@ -19,9 +19,6 @@ import ( "github.com/opentracing/opentracing-go" "go.uber.org/zap" - - "github.com/jaegertracing/jaeger/model/adjuster" - "github.com/jaegertracing/jaeger/storage/spanstore" ) // HandlerOption is a function that sets some option on the APIHandler @@ -36,14 +33,7 @@ type handlerOptions struct{} // which is used to emit logs. func (handlerOptions) Logger(logger *zap.Logger) HandlerOption { return func(apiHandler *APIHandler) { - apiHandler.queryService.logger = logger - } -} - -// Adjusters creates a HandlerOption that initializes the sequence of Adjusters on the APIHandler, -func (handlerOptions) Adjusters(adjusters ...adjuster.Adjuster) HandlerOption { - return func(apiHandler *APIHandler) { - apiHandler.queryService.adjuster = adjuster.Sequence(adjusters...) + apiHandler.logger = logger } } @@ -68,23 +58,9 @@ func (handlerOptions) QueryLookbackDuration(queryLookbackDuration time.Duration) } } -// ArchiveSpanReader creates a HandlerOption that initializes lookback duration -func (handlerOptions) ArchiveSpanReader(reader spanstore.Reader) HandlerOption { - return func(apiHandler *APIHandler) { - apiHandler.queryService.archiveSpanReader = reader - } -} - -// ArchiveSpanWriter creates a HandlerOption that initializes lookback duration -func (handlerOptions) ArchiveSpanWriter(writer spanstore.Writer) HandlerOption { - return func(apiHandler *APIHandler) { - apiHandler.queryService.archiveSpanWriter = writer - } -} - // Tracer creates a HandlerOption that initializes OpenTracing tracer func (handlerOptions) Tracer(tracer opentracing.Tracer) HandlerOption { return func(apiHandler *APIHandler) { - apiHandler.queryService.tracer = tracer + apiHandler.tracer = tracer } } diff --git a/cmd/query/app/handler.go b/cmd/query/app/http_handler.go similarity index 85% rename from cmd/query/app/handler.go rename to cmd/query/app/http_handler.go index 71f6e9ab444..842e1fc930c 100644 --- a/cmd/query/app/handler.go +++ b/cmd/query/app/http_handler.go @@ -29,8 +29,8 @@ import ( "github.com/pkg/errors" "go.uber.org/zap" + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/model" - "github.com/jaegertracing/jaeger/model/adjuster" uiconv "github.com/jaegertracing/jaeger/model/converter/json" ui "github.com/jaegertracing/jaeger/model/json" "github.com/jaegertracing/jaeger/pkg/multierror" @@ -78,19 +78,18 @@ func NewRouter() *mux.Router { // APIHandler implements the query service public API by registering routes at httpPrefix type APIHandler struct { - queryService QueryService + queryService *querysvc.QueryService queryParser queryParser basePath string apiPrefix string + logger *zap.Logger + tracer opentracing.Tracer } // NewAPIHandler returns an APIHandler func NewAPIHandler(spanReader spanstore.Reader, dependencyReader dependencystore.Reader, options ...HandlerOption) *APIHandler { aH := &APIHandler{ - queryService: QueryService{ - spanReader: spanReader, - dependencyReader: dependencyReader, - }, + queryService: querysvc.NewQueryService(spanReader, dependencyReader), queryParser: queryParser{ traceQueryLookbackDuration: defaultTraceQueryLookbackDuration, timeNow: time.Now, @@ -103,14 +102,8 @@ func NewAPIHandler(spanReader spanstore.Reader, dependencyReader dependencystore if aH.apiPrefix == "" { aH.apiPrefix = defaultAPIPrefix } - if aH.queryService.adjuster == nil { - aH.queryService.adjuster = adjuster.Sequence(StandardAdjusters...) - } - if aH.queryService.logger == nil { - aH.queryService.logger = zap.NewNop() - } - if aH.queryService.tracer == nil { - aH.queryService.tracer = opentracing.NoopTracer{} + if aH.tracer == nil { + aH.tracer = opentracing.NoopTracer{} } return aH } @@ -136,7 +129,7 @@ func (aH *APIHandler) handleFunc( ) *mux.Route { route = aH.route(route, args...) traceMiddleware := nethttp.Middleware( - aH.queryService.tracer, + aH.tracer, http.HandlerFunc(f), nethttp.OperationNameFunc(func(r *http.Request) string { return route @@ -150,7 +143,7 @@ func (aH *APIHandler) route(route string, args ...interface{}) string { } func (aH *APIHandler) getServices(w http.ResponseWriter, r *http.Request) { - services, err := aH.queryService.spanReader.GetServices(r.Context()) + services, err := aH.queryService.GetServices(r.Context()) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -165,7 +158,7 @@ func (aH *APIHandler) getOperationsLegacy(w http.ResponseWriter, r *http.Request vars := mux.Vars(r) // given how getOperationsLegacy is bound to URL route, serviceParam cannot be empty service, _ := url.QueryUnescape(vars[serviceParam]) - operations, err := aH.queryService.spanReader.GetOperations(r.Context(), service) + operations, err := aH.queryService.GetOperations(r.Context(), service) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -183,7 +176,7 @@ func (aH *APIHandler) getOperations(w http.ResponseWriter, r *http.Request) { return } } - operations, err := aH.queryService.spanReader.GetOperations(r.Context(), service) + operations, err := aH.queryService.GetOperations(r.Context(), service) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -208,7 +201,7 @@ func (aH *APIHandler) search(w http.ResponseWriter, r *http.Request) { return } } else { - tracesFromStorage, err = aH.queryService.spanReader.FindTraces(r.Context(), &tQuery.TraceQueryParameters) + tracesFromStorage, err = aH.queryService.FindTraces(r.Context(), &tQuery.TraceQueryParameters) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -234,7 +227,7 @@ func (aH *APIHandler) tracesByIDs(ctx context.Context, traceIDs []model.TraceID) var errors []structuredError retMe := make([]*model.Trace, 0, len(traceIDs)) for _, traceID := range traceIDs { - if trace, err := trace(ctx, traceID, aH.queryService.spanReader, aH.queryService.archiveSpanReader); err != nil { + if trace, err := aH.queryService.GetTrace(ctx, traceID); err != nil { if err != spanstore.ErrTraceNotFound { return nil, nil, err } @@ -268,7 +261,7 @@ func (aH *APIHandler) dependencies(w http.ResponseWriter, r *http.Request) { } endTs := time.Unix(0, 0).Add(time.Duration(endTsMillis) * time.Millisecond) - dependencies, err := aH.queryService.dependencyReader.GetDependencies(endTs, lookback) + dependencies, err := aH.queryService.GetDependencies(endTs, lookback) if aH.handleError(w, err, http.StatusInternalServerError) { return } @@ -284,7 +277,7 @@ func (aH *APIHandler) convertModelToUI(trace *model.Trace, adjust bool) (*ui.Tra var errors []error if adjust { var err error - trace, err = aH.queryService.adjuster.Adjust(trace) + trace, err = aH.queryService.Adjust(trace) if err != nil { errors = append(errors, err) } @@ -349,7 +342,7 @@ func (aH *APIHandler) parseTraceID(w http.ResponseWriter, r *http.Request) (mode // getTrace implements the REST API /traces/{trace-id} func (aH *APIHandler) getTrace(w http.ResponseWriter, r *http.Request) { - aH.getTraceFromReaders(w, r, aH.queryService.spanReader, aH.queryService.archiveSpanReader) + aH.getTraceFromReaders(w, r, aH.queryService) } // getTraceFromReader parses trace ID from the path, loads the trace from specified Reader, @@ -358,9 +351,8 @@ func (aH *APIHandler) getTraceFromReaders( w http.ResponseWriter, r *http.Request, reader spanstore.Reader, - backupReader spanstore.Reader, ) { - aH.withTraceFromReader(w, r, reader, backupReader, func(trace *model.Trace) { + aH.withTraceFromReader(w, r, reader, func(trace *model.Trace) { var uiErrors []structuredError uiTrace, uiErr := aH.convertModelToUI(trace, shouldAdjust(r)) if uiErr != nil { @@ -389,14 +381,13 @@ func (aH *APIHandler) withTraceFromReader( w http.ResponseWriter, r *http.Request, reader spanstore.Reader, - backupReader spanstore.Reader, process func(trace *model.Trace), ) { traceID, ok := aH.parseTraceID(w, r) if !ok { return } - trace, err := trace(r.Context(), traceID, reader, backupReader) + trace, err := reader.GetTrace(r.Context(), traceID) if err == spanstore.ErrTraceNotFound { aH.handleError(w, err, http.StatusNotFound) return @@ -407,33 +398,17 @@ func (aH *APIHandler) withTraceFromReader( process(trace) } -func trace( - ctx context.Context, - traceID model.TraceID, - reader spanstore.Reader, - backupReader spanstore.Reader, -) (*model.Trace, error) { - trace, err := reader.GetTrace(ctx, traceID) - if err == spanstore.ErrTraceNotFound { - if backupReader == nil { - return nil, err - } - trace, err = backupReader.GetTrace(ctx, traceID) - } - return trace, err -} - // archiveTrace implements the REST API POST:/archive/{trace-id}. // It reads the trace from the main Reader and saves it to archive Writer. func (aH *APIHandler) archiveTrace(w http.ResponseWriter, r *http.Request) { - if aH.queryService.archiveSpanWriter == nil { + if aH.queryService.CheckArchiveSpanWriter() == true { aH.handleError(w, errNoArchiveSpanStorage, http.StatusInternalServerError) return } - aH.withTraceFromReader(w, r, aH.queryService.spanReader, nil, func(trace *model.Trace) { + aH.withTraceFromReader(w, r, aH.queryService, func(trace *model.Trace) { var writeErrors []error for _, span := range trace.Spans { - err := aH.queryService.archiveSpanWriter.WriteSpan(span) + err := aH.queryService.WriteSpan(span) if err != nil { writeErrors = append(writeErrors, err) } @@ -455,7 +430,7 @@ func (aH *APIHandler) handleError(w http.ResponseWriter, err error, statusCode i return false } if statusCode == http.StatusInternalServerError { - aH.queryService.logger.Error("HTTP handler, Internal Server Error", zap.Error(err)) + aH.logger.Error("HTTP handler, Internal Server Error", zap.Error(err)) } structuredResp := structuredResponse{ Errors: []structuredError{ diff --git a/cmd/query/app/handler_test.go b/cmd/query/app/http_handler_test.go similarity index 98% rename from cmd/query/app/handler_test.go rename to cmd/query/app/http_handler_test.go index 0a29b38a29d..f0c791cd148 100644 --- a/cmd/query/app/handler_test.go +++ b/cmd/query/app/http_handler_test.go @@ -35,6 +35,7 @@ import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/model" "github.com/jaegertracing/jaeger/model/adjuster" ui "github.com/jaegertracing/jaeger/model/json" @@ -294,7 +295,7 @@ func TestGetTraceNotFound(t *testing.T) { func TestGetTraceAdjustmentFailure(t *testing.T) { server, readMock, _, _ := initializeTestServerWithHandler( - HandlerOptions.Adjusters( + querysvc.QueryServiceOptions.Adjusters( adjuster.Func(func(trace *model.Trace) (*model.Trace, error) { return trace, errAdjustment }), @@ -347,7 +348,7 @@ func TestSearchByTraceIDSuccess(t *testing.T) { func TestSearchByTraceIDSuccessWithArchive(t *testing.T) { archiveReadMock := &spanstoremocks.Reader{} - server, readMock, _ := initializeTestServer(HandlerOptions.ArchiveSpanReader(archiveReadMock)) + server, readMock, _ := initializeTestServer(querysvc.QueryServiceOptions.ArchiveSpanReader(archiveReadMock)) defer server.Close() readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). Return(nil, spanstore.ErrTraceNotFound).Twice() @@ -388,7 +389,7 @@ func TestSearchByTraceIDFailure(t *testing.T) { func TestSearchModelConversionFailure(t *testing.T) { server, readMock, _, _ := initializeTestServerWithOptions( - HandlerOptions.Adjusters( + querysvc.QueryServiceOptions.Adjusters( adjuster.Func(func(trace *model.Trace) (*model.Trace, error) { return trace, errAdjustment }), diff --git a/cmd/query/app/query_service.go b/cmd/query/app/query_service.go deleted file mode 100644 index 73672af32ee..00000000000 --- a/cmd/query/app/query_service.go +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright (c) 2019 The Jaeger Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package app - -import ( - "github.com/opentracing/opentracing-go" - "go.uber.org/zap" - - "github.com/jaegertracing/jaeger/model/adjuster" - "github.com/jaegertracing/jaeger/storage/dependencystore" - "github.com/jaegertracing/jaeger/storage/spanstore" -) - -// QueryService contains span utils required by the query-service. -type QueryService struct { - spanReader spanstore.Reader - archiveSpanReader spanstore.Reader - archiveSpanWriter spanstore.Writer - dependencyReader dependencystore.Reader - adjuster adjuster.Adjuster - logger *zap.Logger - tracer opentracing.Tracer -} diff --git a/cmd/query/app/adjusters.go b/cmd/query/app/querysvc/adjusters.go similarity index 98% rename from cmd/query/app/adjusters.go rename to cmd/query/app/querysvc/adjusters.go index d8a951170b9..fbcb1904a64 100644 --- a/cmd/query/app/adjusters.go +++ b/cmd/query/app/querysvc/adjusters.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package app +package querysvc import ( "github.com/jaegertracing/jaeger/model/adjuster" diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go new file mode 100644 index 00000000000..a2bba47a20d --- /dev/null +++ b/cmd/query/app/querysvc/query_service.go @@ -0,0 +1,111 @@ +// Copyright (c) 2019 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package querysvc + +import ( + "context" + "time" + + "github.com/jaegertracing/jaeger/model" + "github.com/jaegertracing/jaeger/model/adjuster" + "github.com/jaegertracing/jaeger/storage/dependencystore" + "github.com/jaegertracing/jaeger/storage/spanstore" +) + +// QueryService contains span utils required by the query-service. +type QueryService struct { + spanReader spanstore.Reader + archiveSpanReader spanstore.Reader + archiveSpanWriter spanstore.Writer + dependencyReader dependencystore.Reader + adjuster adjuster.Adjuster +} + +// NewQueryService returns a new QueryService. +func NewQueryService(spanReader spanstore.Reader, dependencyReader dependencystore.Reader, options ...QueryServiceOption) *QueryService { + qsvc := &QueryService{ + spanReader: spanReader, + dependencyReader: dependencyReader, + } + + for _, option := range options { + option(qsvc) + } + if qsvc.adjuster == nil { + qsvc.adjuster = adjuster.Sequence(StandardAdjusters...) + } + return qsvc +} + +// Implement the spanstore.Reader interface + +// GetTrace is the queryService implementation of spanstore.Reader.GetTrace +func (qs QueryService) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error) { + trace, err := qs.spanReader.GetTrace(ctx, traceID) + if err == spanstore.ErrTraceNotFound { + if qs.archiveSpanReader == nil { + return nil, err + } + trace, err = qs.archiveSpanReader.GetTrace(ctx, traceID) + } + return trace, err +} + +// GetServices is the queryService implementation of spanstore.Reader.GetServices +func (qs QueryService) GetServices(ctx context.Context) ([]string, error) { + return qs.spanReader.GetServices(ctx) +} + +// GetOperations is the queryService implementation of spanstore.Reader.GetOperations +func (qs QueryService) GetOperations(ctx context.Context, service string) ([]string, error) { + return qs.spanReader.GetOperations(ctx, service) +} + +// FindTraces is the queryService implementation of spanstore.Reader.FindTraces +func (qs QueryService) FindTraces(ctx context.Context, query *spanstore.TraceQueryParameters) ([]*model.Trace, error) { + return qs.spanReader.FindTraces(ctx, query) +} + +// FindTraceIDs is the queryService implementation of spanstore.Reader.FindTraceIDs +func (qs QueryService) FindTraceIDs(ctx context.Context, query *spanstore.TraceQueryParameters) ([]model.TraceID, error) { + return qs.spanReader.FindTraceIDs(ctx, query) +} + +// Implement the spanstore.Writer interface + +// WriteSpan is the queryService implementation of spanstore.Writer.WriteSpan +func (qs QueryService) WriteSpan(span *model.Span) error { + return qs.archiveSpanWriter.WriteSpan(span) +} + +// CheckArchiveSpanWriter checks if archiveSpanWriter is nil. +func (qs QueryService) CheckArchiveSpanWriter() bool { + return qs.archiveSpanWriter == nil +} + + +// Implement the adjuster.Adjuster interface + +// Adjust implements adjuster.Adjuster.Adjust +func (qs QueryService) Adjust(trace *model.Trace) (*model.Trace, error) { + return qs.adjuster.Adjust(trace) +} + +// Implement the dependencystore.Reader interface + +// GetDependencies implements dependencystore.Reader.GetDependencies +func (qs QueryService) GetDependencies(endTs time.Time, lookback time.Duration) ([]model.DependencyLink, error) { + return qs.dependencyReader.GetDependencies(endTs, lookback) +} \ No newline at end of file diff --git a/cmd/query/app/querysvc/query_service_options.go b/cmd/query/app/querysvc/query_service_options.go new file mode 100644 index 00000000000..d9f9e3049c1 --- /dev/null +++ b/cmd/query/app/querysvc/query_service_options.go @@ -0,0 +1,49 @@ +// Copyright (c) 2019 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package querysvc + +import ( + "github.com/jaegertracing/jaeger/model/adjuster" + "github.com/jaegertracing/jaeger/storage/spanstore" +) + +// QueryServiceOption is a function that sets some option on the QueryService +type QueryServiceOption func(qsvc *QueryService) + +// QueryServiceOptions is a factory for all available QueryServiceOptions +var QueryServiceOptions queryServiceOptions + +type queryServiceOptions struct{} + +// Adjusters creates a QueryServiceOption that initializes the sequence of Adjusters on the QueryService. +func (queryServiceOptions) Adjusters(adjusters ...adjuster.Adjuster) QueryServiceOption { + return func(queryService *QueryService) { + queryService.adjuster = adjuster.Sequence(adjusters...) + } +} + +// ArchiveSpanReader creates a QueryServiceOption that initializes an ArchiveSpanReader on the QueryService. +func (queryServiceOptions) ArchiveSpanReader(reader spanstore.Reader) QueryServiceOption { + return func(queryService *QueryService) { + queryService.archiveSpanReader = reader + } +} + +// ArchiveSpanWriter creates a QueryServiceOption that initializes an ArchiveSpanWriter on the QueryService +func (queryServiceOptions) ArchiveSpanWriter(writer spanstore.Writer) QueryServiceOption { + return func(queryService *QueryService) { + queryService.archiveSpanWriter = writer + } +} \ No newline at end of file From ba76b1b31279fb7742696887971f23ce748136da Mon Sep 17 00:00:00 2001 From: Annanay Date: Mon, 4 Feb 2019 00:53:18 +0530 Subject: [PATCH 03/17] Addressed comments, refactored Signed-off-by: Annanay --- cmd/query/app/http_handler.go | 98 +++++++------------ cmd/query/app/querysvc/query_service.go | 60 +++++++----- .../app/querysvc/query_service_options.go | 30 +----- cmd/query/main.go | 18 ++-- 4 files changed, 88 insertions(+), 118 deletions(-) diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index 842e1fc930c..80a40ee9318 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -34,7 +34,6 @@ import ( uiconv "github.com/jaegertracing/jaeger/model/converter/json" ui "github.com/jaegertracing/jaeger/model/json" "github.com/jaegertracing/jaeger/pkg/multierror" - "github.com/jaegertracing/jaeger/storage/dependencystore" "github.com/jaegertracing/jaeger/storage/spanstore" ) @@ -48,10 +47,6 @@ const ( defaultAPIPrefix = "api" ) -var ( - errNoArchiveSpanStorage = errors.New("archive span storage was not configured") -) - // HTTPHandler handles http requests type HTTPHandler interface { RegisterRoutes(router *mux.Router) @@ -87,9 +82,9 @@ type APIHandler struct { } // NewAPIHandler returns an APIHandler -func NewAPIHandler(spanReader spanstore.Reader, dependencyReader dependencystore.Reader, options ...HandlerOption) *APIHandler { +func NewAPIHandler(queryService *querysvc.QueryService, options ...HandlerOption) *APIHandler { aH := &APIHandler{ - queryService: querysvc.NewQueryService(spanReader, dependencyReader), + queryService: queryService, queryParser: queryParser{ traceQueryLookbackDuration: defaultTraceQueryLookbackDuration, timeNow: time.Now, @@ -351,37 +346,6 @@ func (aH *APIHandler) getTraceFromReaders( w http.ResponseWriter, r *http.Request, reader spanstore.Reader, -) { - aH.withTraceFromReader(w, r, reader, func(trace *model.Trace) { - var uiErrors []structuredError - uiTrace, uiErr := aH.convertModelToUI(trace, shouldAdjust(r)) - if uiErr != nil { - uiErrors = append(uiErrors, *uiErr) - } - - structuredRes := structuredResponse{ - Data: []*ui.Trace{ - uiTrace, - }, - Errors: uiErrors, - } - aH.writeJSON(w, r, &structuredRes) - }) -} - -func shouldAdjust(r *http.Request) bool { - raw := r.FormValue("raw") - isRaw, _ := strconv.ParseBool(raw) - return !isRaw -} - -// withTraceFromReader tries to load a trace from Reader and if successful -// execute process() function passing it that trace. -func (aH *APIHandler) withTraceFromReader( - w http.ResponseWriter, - r *http.Request, - reader spanstore.Reader, - process func(trace *model.Trace), ) { traceID, ok := aH.parseTraceID(w, r) if !ok { @@ -395,34 +359,48 @@ func (aH *APIHandler) withTraceFromReader( if aH.handleError(w, err, http.StatusInternalServerError) { return } - process(trace) + + var uiErrors []structuredError + uiTrace, uiErr := aH.convertModelToUI(trace, shouldAdjust(r)) + if uiErr != nil { + uiErrors = append(uiErrors, *uiErr) + } + + structuredRes := structuredResponse{ + Data: []*ui.Trace{ + uiTrace, + }, + Errors: uiErrors, + } + aH.writeJSON(w, r, &structuredRes) +} + +func shouldAdjust(r *http.Request) bool { + raw := r.FormValue("raw") + isRaw, _ := strconv.ParseBool(raw) + return !isRaw } // archiveTrace implements the REST API POST:/archive/{trace-id}. -// It reads the trace from the main Reader and saves it to archive Writer. +// It passes the traceID to queryService.ArchiveTrace for writing. func (aH *APIHandler) archiveTrace(w http.ResponseWriter, r *http.Request) { - if aH.queryService.CheckArchiveSpanWriter() == true { - aH.handleError(w, errNoArchiveSpanStorage, http.StatusInternalServerError) + traceID, ok := aH.parseTraceID(w, r) + if !ok { return } - aH.withTraceFromReader(w, r, aH.queryService, func(trace *model.Trace) { - var writeErrors []error - for _, span := range trace.Spans { - err := aH.queryService.WriteSpan(span) - if err != nil { - writeErrors = append(writeErrors, err) - } - } - err := multierror.Wrap(writeErrors) - if aH.handleError(w, err, http.StatusInternalServerError) { - return - } - structuredRes := structuredResponse{ - Data: []string{}, // doens't matter, just want an empty array - Errors: []structuredError{}, - } - aH.writeJSON(w, r, &structuredRes) - }) + + // QueryService.ArchiveTrace can now archive this traceID. + err := aH.queryService.ArchiveTrace(r.Context(), &traceID) + if err != nil { + aH.handleError(w, err, http.StatusInternalServerError) + return + } + + structuredRes := structuredResponse{ + Data: []string{}, // doens't matter, just want an empty array + Errors: []structuredError{}, + } + aH.writeJSON(w, r, &structuredRes) } func (aH *APIHandler) handleError(w http.ResponseWriter, err error, statusCode int) bool { diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go index a2bba47a20d..771c0e48a37 100644 --- a/cmd/query/app/querysvc/query_service.go +++ b/cmd/query/app/querysvc/query_service.go @@ -16,35 +16,37 @@ package querysvc import ( "context" + "errors" "time" "github.com/jaegertracing/jaeger/model" "github.com/jaegertracing/jaeger/model/adjuster" + "github.com/jaegertracing/jaeger/pkg/multierror" "github.com/jaegertracing/jaeger/storage/dependencystore" "github.com/jaegertracing/jaeger/storage/spanstore" ) +var ( + errNoArchiveSpanStorage = errors.New("archive span storage was not configured") +) + // QueryService contains span utils required by the query-service. type QueryService struct { - spanReader spanstore.Reader - archiveSpanReader spanstore.Reader - archiveSpanWriter spanstore.Writer - dependencyReader dependencystore.Reader - adjuster adjuster.Adjuster + spanReader spanstore.Reader + dependencyReader dependencystore.Reader + options QueryServiceOptions } // NewQueryService returns a new QueryService. -func NewQueryService(spanReader spanstore.Reader, dependencyReader dependencystore.Reader, options ...QueryServiceOption) *QueryService { +func NewQueryService(spanReader spanstore.Reader, dependencyReader dependencystore.Reader, options QueryServiceOptions) *QueryService { qsvc := &QueryService{ spanReader: spanReader, dependencyReader: dependencyReader, + options: options, } - for _, option := range options { - option(qsvc) - } - if qsvc.adjuster == nil { - qsvc.adjuster = adjuster.Sequence(StandardAdjusters...) + if qsvc.options.adjuster == nil { + qsvc.options.adjuster = adjuster.Sequence(StandardAdjusters...) } return qsvc } @@ -55,10 +57,10 @@ func NewQueryService(spanReader spanstore.Reader, dependencyReader dependencysto func (qs QueryService) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error) { trace, err := qs.spanReader.GetTrace(ctx, traceID) if err == spanstore.ErrTraceNotFound { - if qs.archiveSpanReader == nil { + if qs.options.archiveSpanReader == nil { return nil, err } - trace, err = qs.archiveSpanReader.GetTrace(ctx, traceID) + trace, err = qs.options.archiveSpanReader.GetTrace(ctx, traceID) } return trace, err } @@ -83,24 +85,32 @@ func (qs QueryService) FindTraceIDs(ctx context.Context, query *spanstore.TraceQ return qs.spanReader.FindTraceIDs(ctx, query) } -// Implement the spanstore.Writer interface - -// WriteSpan is the queryService implementation of spanstore.Writer.WriteSpan -func (qs QueryService) WriteSpan(span *model.Span) error { - return qs.archiveSpanWriter.WriteSpan(span) -} +// ArchiveTrace is the queryService utility to archive traces. +func (qs QueryService) ArchiveTrace(ctx context.Context, traceID *model.TraceID) error { + if qs.options.archiveSpanReader == nil { + return errNoArchiveSpanStorage + } + trace, err := qs.GetTrace(ctx, *traceID) + if err != nil { + return err + } -// CheckArchiveSpanWriter checks if archiveSpanWriter is nil. -func (qs QueryService) CheckArchiveSpanWriter() bool { - return qs.archiveSpanWriter == nil + var writeErrors []error + for _, span := range trace.Spans { + err := qs.options.archiveSpanWriter.WriteSpan(span) + if err != nil { + writeErrors = append(writeErrors, err) + } + } + multierr := multierror.Wrap(writeErrors) + return multierr } - // Implement the adjuster.Adjuster interface // Adjust implements adjuster.Adjuster.Adjust func (qs QueryService) Adjust(trace *model.Trace) (*model.Trace, error) { - return qs.adjuster.Adjust(trace) + return qs.options.adjuster.Adjust(trace) } // Implement the dependencystore.Reader interface @@ -108,4 +118,4 @@ func (qs QueryService) Adjust(trace *model.Trace) (*model.Trace, error) { // GetDependencies implements dependencystore.Reader.GetDependencies func (qs QueryService) GetDependencies(endTs time.Time, lookback time.Duration) ([]model.DependencyLink, error) { return qs.dependencyReader.GetDependencies(endTs, lookback) -} \ No newline at end of file +} diff --git a/cmd/query/app/querysvc/query_service_options.go b/cmd/query/app/querysvc/query_service_options.go index d9f9e3049c1..ad4418e8ac9 100644 --- a/cmd/query/app/querysvc/query_service_options.go +++ b/cmd/query/app/querysvc/query_service_options.go @@ -19,31 +19,9 @@ import ( "github.com/jaegertracing/jaeger/storage/spanstore" ) -// QueryServiceOption is a function that sets some option on the QueryService -type QueryServiceOption func(qsvc *QueryService) - // QueryServiceOptions is a factory for all available QueryServiceOptions -var QueryServiceOptions queryServiceOptions - -type queryServiceOptions struct{} - -// Adjusters creates a QueryServiceOption that initializes the sequence of Adjusters on the QueryService. -func (queryServiceOptions) Adjusters(adjusters ...adjuster.Adjuster) QueryServiceOption { - return func(queryService *QueryService) { - queryService.adjuster = adjuster.Sequence(adjusters...) - } +type QueryServiceOptions struct { + archiveSpanReader spanstore.Reader + archiveSpanWriter spanstore.Writer + adjuster adjuster.Adjuster } - -// ArchiveSpanReader creates a QueryServiceOption that initializes an ArchiveSpanReader on the QueryService. -func (queryServiceOptions) ArchiveSpanReader(reader spanstore.Reader) QueryServiceOption { - return func(queryService *QueryService) { - queryService.archiveSpanReader = reader - } -} - -// ArchiveSpanWriter creates a QueryServiceOption that initializes an ArchiveSpanWriter on the QueryService -func (queryServiceOptions) ArchiveSpanWriter(writer spanstore.Writer) QueryServiceOption { - return func(queryService *QueryService) { - queryService.archiveSpanWriter = writer - } -} \ No newline at end of file diff --git a/cmd/query/main.go b/cmd/query/main.go index f66a4ec938c..bb35b4456d0 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -35,6 +35,7 @@ import ( "github.com/jaegertracing/jaeger/cmd/env" "github.com/jaegertracing/jaeger/cmd/flags" "github.com/jaegertracing/jaeger/cmd/query/app" + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/pkg/config" "github.com/jaegertracing/jaeger/pkg/healthcheck" pMetrics "github.com/jaegertracing/jaeger/pkg/metrics" @@ -115,15 +116,18 @@ func main() { if err != nil { logger.Fatal("Failed to create dependency reader", zap.Error(err)) } + queryServiceOptions := archiveOptions(storageFactory, logger) + queryService := querysvc.NewQueryService( + spanReader, + dependencyReader, + *queryServiceOptions) apiHandlerOptions := []app.HandlerOption{ app.HandlerOptions.Logger(logger), app.HandlerOptions.Tracer(tracer), } - apiHandlerOptions = append(apiHandlerOptions, archiveOptions(storageFactory, logger)...) apiHandler := app.NewAPIHandler( - spanReader, - dependencyReader, + queryService, apiHandlerOptions...) r := app.NewRouter() if queryOpts.BasePath != "/" { @@ -177,7 +181,7 @@ func main() { } } -func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) []app.HandlerOption { +func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) *querysvc.QueryServiceOptions { archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) if !ok { logger.Info("Archive storage not supported by the factory") @@ -201,8 +205,8 @@ func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) []app.H logger.Error("Cannot init archive storage writer", zap.Error(err)) return nil } - return []app.HandlerOption{ - app.HandlerOptions.ArchiveSpanReader(reader), - app.HandlerOptions.ArchiveSpanWriter(writer), + return &querysvc.QueryServiceOptions{ + archiveSpanReader: reader, + archiveSpanWriter: writer, } } From 772d75e70faed27a1b53634f367a68493e7a65a2 Mon Sep 17 00:00:00 2001 From: Annanay Date: Tue, 5 Feb 2019 00:09:20 +0530 Subject: [PATCH 04/17] Cleaned function signatures, removed extra query_service_options file Signed-off-by: Annanay --- cmd/query/app/http_handler.go | 19 ++++--------- cmd/query/app/querysvc/query_service.go | 20 +++++++------- .../app/querysvc/query_service_options.go | 27 ------------------- cmd/query/main.go | 16 +++++------ 4 files changed, 23 insertions(+), 59 deletions(-) delete mode 100644 cmd/query/app/querysvc/query_service_options.go diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index 80a40ee9318..c1bf3c74fac 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -336,22 +336,14 @@ func (aH *APIHandler) parseTraceID(w http.ResponseWriter, r *http.Request) (mode } // getTrace implements the REST API /traces/{trace-id} -func (aH *APIHandler) getTrace(w http.ResponseWriter, r *http.Request) { - aH.getTraceFromReaders(w, r, aH.queryService) -} - -// getTraceFromReader parses trace ID from the path, loads the trace from specified Reader, +// It parses trace ID from the path, fetches the trace from QueryService, // formats it in the UI JSON format, and responds to the client. -func (aH *APIHandler) getTraceFromReaders( - w http.ResponseWriter, - r *http.Request, - reader spanstore.Reader, -) { +func (aH *APIHandler) getTrace(w http.ResponseWriter, r *http.Request) { traceID, ok := aH.parseTraceID(w, r) if !ok { return } - trace, err := reader.GetTrace(r.Context(), traceID) + trace, err := aH.queryService.GetTrace(r.Context(), traceID) if err == spanstore.ErrTraceNotFound { aH.handleError(w, err, http.StatusNotFound) return @@ -390,9 +382,8 @@ func (aH *APIHandler) archiveTrace(w http.ResponseWriter, r *http.Request) { } // QueryService.ArchiveTrace can now archive this traceID. - err := aH.queryService.ArchiveTrace(r.Context(), &traceID) - if err != nil { - aH.handleError(w, err, http.StatusInternalServerError) + err := aH.queryService.ArchiveTrace(r.Context(), traceID) + if aH.handleError(w, err, http.StatusInternalServerError) { return } diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go index 771c0e48a37..366daaa5785 100644 --- a/cmd/query/app/querysvc/query_service.go +++ b/cmd/query/app/querysvc/query_service.go @@ -30,6 +30,13 @@ var ( errNoArchiveSpanStorage = errors.New("archive span storage was not configured") ) +// QueryServiceOptions has optional members of QueryService +type QueryServiceOptions struct { + archiveSpanReader spanstore.Reader + archiveSpanWriter spanstore.Writer + adjuster adjuster.Adjuster +} + // QueryService contains span utils required by the query-service. type QueryService struct { spanReader spanstore.Reader @@ -51,8 +58,6 @@ func NewQueryService(spanReader spanstore.Reader, dependencyReader dependencysto return qsvc } -// Implement the spanstore.Reader interface - // GetTrace is the queryService implementation of spanstore.Reader.GetTrace func (qs QueryService) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error) { trace, err := qs.spanReader.GetTrace(ctx, traceID) @@ -86,11 +91,11 @@ func (qs QueryService) FindTraceIDs(ctx context.Context, query *spanstore.TraceQ } // ArchiveTrace is the queryService utility to archive traces. -func (qs QueryService) ArchiveTrace(ctx context.Context, traceID *model.TraceID) error { +func (qs QueryService) ArchiveTrace(ctx context.Context, traceID model.TraceID) error { if qs.options.archiveSpanReader == nil { return errNoArchiveSpanStorage } - trace, err := qs.GetTrace(ctx, *traceID) + trace, err := qs.GetTrace(ctx, traceID) if err != nil { return err } @@ -102,19 +107,14 @@ func (qs QueryService) ArchiveTrace(ctx context.Context, traceID *model.TraceID) writeErrors = append(writeErrors, err) } } - multierr := multierror.Wrap(writeErrors) - return multierr + return multierror.Wrap(writeErrors) } -// Implement the adjuster.Adjuster interface - // Adjust implements adjuster.Adjuster.Adjust func (qs QueryService) Adjust(trace *model.Trace) (*model.Trace, error) { return qs.options.adjuster.Adjust(trace) } -// Implement the dependencystore.Reader interface - // GetDependencies implements dependencystore.Reader.GetDependencies func (qs QueryService) GetDependencies(endTs time.Time, lookback time.Duration) ([]model.DependencyLink, error) { return qs.dependencyReader.GetDependencies(endTs, lookback) diff --git a/cmd/query/app/querysvc/query_service_options.go b/cmd/query/app/querysvc/query_service_options.go deleted file mode 100644 index ad4418e8ac9..00000000000 --- a/cmd/query/app/querysvc/query_service_options.go +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) 2019 The Jaeger Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package querysvc - -import ( - "github.com/jaegertracing/jaeger/model/adjuster" - "github.com/jaegertracing/jaeger/storage/spanstore" -) - -// QueryServiceOptions is a factory for all available QueryServiceOptions -type QueryServiceOptions struct { - archiveSpanReader spanstore.Reader - archiveSpanWriter spanstore.Writer - adjuster adjuster.Adjuster -} diff --git a/cmd/query/main.go b/cmd/query/main.go index bb35b4456d0..4c3c2942a07 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -120,7 +120,7 @@ func main() { queryService := querysvc.NewQueryService( spanReader, dependencyReader, - *queryServiceOptions) + queryServiceOptions) apiHandlerOptions := []app.HandlerOption{ app.HandlerOptions.Logger(logger), @@ -181,31 +181,31 @@ func main() { } } -func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) *querysvc.QueryServiceOptions { +func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) if !ok { logger.Info("Archive storage not supported by the factory") - return nil + return querysvc.QueryServiceOptions{} } reader, err := archiveFactory.CreateArchiveSpanReader() if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return nil + return querysvc.QueryServiceOptions{} } if err != nil { logger.Error("Cannot init archive storage reader", zap.Error(err)) - return nil + return querysvc.QueryServiceOptions{} } writer, err := archiveFactory.CreateArchiveSpanWriter() if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return nil + return querysvc.QueryServiceOptions{} } if err != nil { logger.Error("Cannot init archive storage writer", zap.Error(err)) - return nil + return querysvc.QueryServiceOptions{} } - return &querysvc.QueryServiceOptions{ + return querysvc.QueryServiceOptions{ archiveSpanReader: reader, archiveSpanWriter: writer, } From 8dd2a5869ae5a313e98ddf7c07e0ca3375064c04 Mon Sep 17 00:00:00 2001 From: Annanay Date: Thu, 7 Feb 2019 01:09:16 +0530 Subject: [PATCH 05/17] Addressed comments, fixed tests Signed-off-by: Annanay --- cmd/query/app/handler_archive_test.go | 11 +++---- cmd/query/app/http_handler.go | 3 ++ cmd/query/app/http_handler_test.go | 40 ++++++++++++++++--------- cmd/query/app/querysvc/query_service.go | 20 ++++++------- cmd/query/main.go | 4 +-- 5 files changed, 47 insertions(+), 31 deletions(-) diff --git a/cmd/query/app/handler_archive_test.go b/cmd/query/app/handler_archive_test.go index 1f835dee197..e640081c000 100644 --- a/cmd/query/app/handler_archive_test.go +++ b/cmd/query/app/handler_archive_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/model" "github.com/jaegertracing/jaeger/storage/spanstore" spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks" @@ -42,7 +43,7 @@ func TestGetArchivedTrace_NotFound(t *testing.T) { assert.EqualError(t, err, `404 error from server: {"data":null,"total":0,"limit":0,"offset":0,"errors":[{"code":404,"msg":"trace not found"}]}`+"\n", ) - }, HandlerOptions.ArchiveSpanReader(archiveReader)) // nil is ok + }, querysvc.QueryServiceOptions{ArchiveSpanReader: archiveReader}) // nil is ok }) } } @@ -62,7 +63,7 @@ func TestGetArchivedTraceSuccess(t *testing.T) { assert.Len(t, response.Errors, 0) assert.Len(t, response.Traces, 1) assert.Equal(t, traceID.String(), string(response.Traces[0].TraceID)) - }, HandlerOptions.ArchiveSpanReader(mockReader)) + }, querysvc.QueryServiceOptions{ArchiveSpanReader: mockReader}) } func TestArchiveTrace_NoStorage(t *testing.T) { @@ -70,7 +71,7 @@ func TestArchiveTrace_NoStorage(t *testing.T) { var response structuredResponse err := postJSON(ts.server.URL+"/api/archive/"+mockTraceID.String(), []string{}, &response) assert.EqualError(t, err, `500 error from server: {"data":null,"total":0,"limit":0,"offset":0,"errors":[{"code":500,"msg":"archive span storage was not configured"}]}`+"\n") - }) + }, querysvc.QueryServiceOptions{}) } func TestArchiveTrace_Success(t *testing.T) { @@ -83,7 +84,7 @@ func TestArchiveTrace_Success(t *testing.T) { var response structuredResponse err := postJSON(ts.server.URL+"/api/archive/"+mockTraceID.String(), []string{}, &response) assert.NoError(t, err) - }, HandlerOptions.ArchiveSpanWriter(mockWriter)) + }, querysvc.QueryServiceOptions{ArchiveSpanWriter: mockWriter}) } func TestArchiveTrace_WriteErrors(t *testing.T) { @@ -96,5 +97,5 @@ func TestArchiveTrace_WriteErrors(t *testing.T) { var response structuredResponse err := postJSON(ts.server.URL+"/api/archive/"+mockTraceID.String(), []string{}, &response) assert.EqualError(t, err, `500 error from server: {"data":null,"total":0,"limit":0,"offset":0,"errors":[{"code":500,"msg":"[cannot save, cannot save]"}]}`+"\n") - }, HandlerOptions.ArchiveSpanWriter(mockWriter)) + }, querysvc.QueryServiceOptions{ArchiveSpanWriter: mockWriter}) } diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index c1bf3c74fac..b2b3cf47ed3 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -97,6 +97,9 @@ func NewAPIHandler(queryService *querysvc.QueryService, options ...HandlerOption if aH.apiPrefix == "" { aH.apiPrefix = defaultAPIPrefix } + if aH.logger == nil { + aH.logger = zap.NewNop() + } if aH.tracer == nil { aH.tracer = opentracing.NoopTracer{} } diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go index f0c791cd148..e0078e60a11 100644 --- a/cmd/query/app/http_handler_test.go +++ b/cmd/query/app/http_handler_test.go @@ -83,8 +83,9 @@ type structuredTraceResponse struct { Errors []structuredError `json:"errors"` } -func initializeTestServerWithHandler(options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader, *APIHandler) { +func initializeTestServerWithHandler(queryOptions querysvc.QueryServiceOptions, options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader, *APIHandler) { return initializeTestServerWithOptions( + queryOptions, append( []HandlerOption{ HandlerOptions.Logger(zap.NewNop()), @@ -98,17 +99,23 @@ func initializeTestServerWithHandler(options ...HandlerOption) (*httptest.Server ) } -func initializeTestServerWithOptions(options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader, *APIHandler) { +func initializeTestServerWithOptions(queryOptions querysvc.QueryServiceOptions, options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader, *APIHandler) { readStorage := &spanstoremocks.Reader{} dependencyStorage := &depsmocks.Reader{} + qs := querysvc.NewQueryService(readStorage, dependencyStorage, queryOptions) r := NewRouter() - handler := NewAPIHandler(readStorage, dependencyStorage, options...) + handler := NewAPIHandler(qs, options...) handler.RegisterRoutes(r) return httptest.NewServer(r), readStorage, dependencyStorage, handler } +func initializeTestServerWithQueryOptions(queryOptions querysvc.QueryServiceOptions, options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader) { + https, sr, dr, _ := initializeTestServerWithHandler(queryOptions, options...) + return https, sr, dr +} + func initializeTestServer(options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader) { - https, sr, dr, _ := initializeTestServerWithHandler(options...) + https, sr, dr, _ := initializeTestServerWithHandler(querysvc.QueryServiceOptions{}, options...) return https, sr, dr } @@ -119,8 +126,8 @@ type testServer struct { server *httptest.Server } -func withTestServer(t *testing.T, doTest func(s *testServer), options ...HandlerOption) { - server, spanReader, depReader, handler := initializeTestServerWithOptions(options...) +func withTestServer(t *testing.T, doTest func(s *testServer), queryOptions querysvc.QueryServiceOptions, options ...HandlerOption) { + server, spanReader, depReader, handler := initializeTestServerWithOptions(queryOptions, options...) s := &testServer{ spanReader: spanReader, dependencyReader: depReader, @@ -167,10 +174,13 @@ func TestLogOnServerError(t *testing.T) { l := &testLogger{ logs: &[]logData{}, } + readStorage := &spanstoremocks.Reader{} + dependencyStorage := &depsmocks.Reader{} + qs := querysvc.NewQueryService(readStorage, dependencyStorage, querysvc.QueryServiceOptions{}) apiHandlerOptions := []HandlerOption{ HandlerOptions.Logger(zap.New(l)), } - h := NewAPIHandler(&spanstoremocks.Reader{}, &depsmocks.Reader{}, apiHandlerOptions...) + h := NewAPIHandler(qs, apiHandlerOptions...) e := errors.New("test error") h.handleError(&testHttp.TestResponseWriter{}, e, http.StatusInternalServerError) require.Equal(t, 1, len(*l.logs)) @@ -295,11 +305,11 @@ func TestGetTraceNotFound(t *testing.T) { func TestGetTraceAdjustmentFailure(t *testing.T) { server, readMock, _, _ := initializeTestServerWithHandler( - querysvc.QueryServiceOptions.Adjusters( - adjuster.Func(func(trace *model.Trace) (*model.Trace, error) { + querysvc.QueryServiceOptions{ + Adjuster: adjuster.Func(func(trace *model.Trace) (*model.Trace, error) { return trace, errAdjustment }), - ), + }, ) defer server.Close() readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). @@ -348,7 +358,9 @@ func TestSearchByTraceIDSuccess(t *testing.T) { func TestSearchByTraceIDSuccessWithArchive(t *testing.T) { archiveReadMock := &spanstoremocks.Reader{} - server, readMock, _ := initializeTestServer(querysvc.QueryServiceOptions.ArchiveSpanReader(archiveReadMock)) + server, readMock, _ := initializeTestServerWithQueryOptions(querysvc.QueryServiceOptions{ + ArchiveSpanReader: archiveReadMock, + }) defer server.Close() readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). Return(nil, spanstore.ErrTraceNotFound).Twice() @@ -389,11 +401,11 @@ func TestSearchByTraceIDFailure(t *testing.T) { func TestSearchModelConversionFailure(t *testing.T) { server, readMock, _, _ := initializeTestServerWithOptions( - querysvc.QueryServiceOptions.Adjusters( - adjuster.Func(func(trace *model.Trace) (*model.Trace, error) { + querysvc.QueryServiceOptions{ + Adjuster: adjuster.Func(func(trace *model.Trace) (*model.Trace, error) { return trace, errAdjustment }), - ), + }, ) defer server.Close() readMock.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")). diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go index 366daaa5785..b7fff364b08 100644 --- a/cmd/query/app/querysvc/query_service.go +++ b/cmd/query/app/querysvc/query_service.go @@ -32,9 +32,9 @@ var ( // QueryServiceOptions has optional members of QueryService type QueryServiceOptions struct { - archiveSpanReader spanstore.Reader - archiveSpanWriter spanstore.Writer - adjuster adjuster.Adjuster + ArchiveSpanReader spanstore.Reader + ArchiveSpanWriter spanstore.Writer + Adjuster adjuster.Adjuster } // QueryService contains span utils required by the query-service. @@ -52,8 +52,8 @@ func NewQueryService(spanReader spanstore.Reader, dependencyReader dependencysto options: options, } - if qsvc.options.adjuster == nil { - qsvc.options.adjuster = adjuster.Sequence(StandardAdjusters...) + if qsvc.options.Adjuster == nil { + qsvc.options.Adjuster = adjuster.Sequence(StandardAdjusters...) } return qsvc } @@ -62,10 +62,10 @@ func NewQueryService(spanReader spanstore.Reader, dependencyReader dependencysto func (qs QueryService) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error) { trace, err := qs.spanReader.GetTrace(ctx, traceID) if err == spanstore.ErrTraceNotFound { - if qs.options.archiveSpanReader == nil { + if qs.options.ArchiveSpanReader == nil { return nil, err } - trace, err = qs.options.archiveSpanReader.GetTrace(ctx, traceID) + trace, err = qs.options.ArchiveSpanReader.GetTrace(ctx, traceID) } return trace, err } @@ -92,7 +92,7 @@ func (qs QueryService) FindTraceIDs(ctx context.Context, query *spanstore.TraceQ // ArchiveTrace is the queryService utility to archive traces. func (qs QueryService) ArchiveTrace(ctx context.Context, traceID model.TraceID) error { - if qs.options.archiveSpanReader == nil { + if qs.options.ArchiveSpanWriter == nil { return errNoArchiveSpanStorage } trace, err := qs.GetTrace(ctx, traceID) @@ -102,7 +102,7 @@ func (qs QueryService) ArchiveTrace(ctx context.Context, traceID model.TraceID) var writeErrors []error for _, span := range trace.Spans { - err := qs.options.archiveSpanWriter.WriteSpan(span) + err := qs.options.ArchiveSpanWriter.WriteSpan(span) if err != nil { writeErrors = append(writeErrors, err) } @@ -112,7 +112,7 @@ func (qs QueryService) ArchiveTrace(ctx context.Context, traceID model.TraceID) // Adjust implements adjuster.Adjuster.Adjust func (qs QueryService) Adjust(trace *model.Trace) (*model.Trace, error) { - return qs.options.adjuster.Adjust(trace) + return qs.options.Adjuster.Adjust(trace) } // GetDependencies implements dependencystore.Reader.GetDependencies diff --git a/cmd/query/main.go b/cmd/query/main.go index 4c3c2942a07..ead0e81e56c 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -206,7 +206,7 @@ func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysv return querysvc.QueryServiceOptions{} } return querysvc.QueryServiceOptions{ - archiveSpanReader: reader, - archiveSpanWriter: writer, + ArchiveSpanReader: reader, + ArchiveSpanWriter: writer, } } From e667c1274695d4fa4dd949e848441c5bc4b60562 Mon Sep 17 00:00:00 2001 From: Annanay Date: Thu, 7 Feb 2019 17:40:43 +0530 Subject: [PATCH 06/17] Addressed comments, fixed tests Signed-off-by: Annanay --- cmd/all-in-one/main.go | 41 ++++--------------------- cmd/query/app/http_handler.go | 2 +- cmd/query/app/querysvc/query_service.go | 2 +- cmd/query/app/utils.go | 39 +++++++++++++++++++++++ cmd/query/main.go | 32 +------------------ 5 files changed, 48 insertions(+), 68 deletions(-) create mode 100644 cmd/query/app/utils.go diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index 045ac906cd0..13debcd3b89 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -52,6 +52,7 @@ import ( "github.com/jaegertracing/jaeger/cmd/env" "github.com/jaegertracing/jaeger/cmd/flags" queryApp "github.com/jaegertracing/jaeger/cmd/query/app" + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/pkg/config" "github.com/jaegertracing/jaeger/pkg/healthcheck" pMetrics "github.com/jaegertracing/jaeger/pkg/metrics" @@ -142,7 +143,7 @@ func main() { startAgent(aOpts, repOpts, tchannelRepOpts, grpcRepOpts, cOpts, logger, metricsFactory) grpcServer := startCollector(cOpts, spanWriter, logger, metricsFactory, strategyStore, hc) - startQuery(qOpts, spanReader, dependencyReader, logger, rootMetricsFactory, metricsFactory, mBldr, hc, archiveOptions(storageFactory, logger)) + startQuery(qOpts, spanReader, dependencyReader, logger, rootMetricsFactory, metricsFactory, mBldr, hc, queryApp.ArchiveOptions(storageFactory, logger)) hc.Ready() <-signalsChannel logger.Info("Shutting down") @@ -341,7 +342,7 @@ func startQuery( baseFactory metrics.Factory, metricsBuilder *pMetrics.Builder, hc *healthcheck.HealthCheck, - handlerOpts []queryApp.HandlerOption, + queryOpts querysvc.QueryServiceOptions, ) { tracer, closer, err := jaegerClientConfig.Configuration{ Sampler: &jaegerClientConfig.SamplerConfig{ @@ -361,10 +362,10 @@ func startQuery( spanReader = storageMetrics.NewReadMetricsDecorator(spanReader, baseFactory.Namespace(metrics.NSOptions{Name: "query", Tags: nil})) - handlerOpts = append(handlerOpts, queryApp.HandlerOptions.Logger(logger), queryApp.HandlerOptions.Tracer(tracer)) + qs := querysvc.NewQueryService(spanReader, depReader, queryOpts) + handlerOpts := []HandlerOption{queryApp.HandlerOptions.Logger(logger), queryApp.HandlerOptions.Tracer(tracer)} apiHandler := queryApp.NewAPIHandler( - spanReader, - depReader, + qs, handlerOpts...) r := mux.NewRouter() @@ -405,33 +406,3 @@ func initSamplingStrategyStore( } return strategyStore } - -func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) []queryApp.HandlerOption { - archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) - if !ok { - logger.Info("Archive storage not supported by the factory") - return nil - } - reader, err := archiveFactory.CreateArchiveSpanReader() - if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { - logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return nil - } - if err != nil { - logger.Error("Cannot init archive storage reader", zap.Error(err)) - return nil - } - writer, err := archiveFactory.CreateArchiveSpanWriter() - if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { - logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return nil - } - if err != nil { - logger.Error("Cannot init archive storage writer", zap.Error(err)) - return nil - } - return []queryApp.HandlerOption{ - queryApp.HandlerOptions.ArchiveSpanReader(reader), - queryApp.HandlerOptions.ArchiveSpanWriter(writer), - } -} diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index b2b3cf47ed3..7f0c868ca04 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -386,7 +386,7 @@ func (aH *APIHandler) archiveTrace(w http.ResponseWriter, r *http.Request) { // QueryService.ArchiveTrace can now archive this traceID. err := aH.queryService.ArchiveTrace(r.Context(), traceID) - if aH.handleError(w, err, http.StatusInternalServerError) { + if aH.handleError(w, err, http.StatusNotFound) { return } diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go index b7fff364b08..0c7036209b3 100644 --- a/cmd/query/app/querysvc/query_service.go +++ b/cmd/query/app/querysvc/query_service.go @@ -110,7 +110,7 @@ func (qs QueryService) ArchiveTrace(ctx context.Context, traceID model.TraceID) return multierror.Wrap(writeErrors) } -// Adjust implements adjuster.Adjuster.Adjust +// Adjust applies adjusters to the trace. func (qs QueryService) Adjust(trace *model.Trace) (*model.Trace, error) { return qs.options.Adjuster.Adjust(trace) } diff --git a/cmd/query/app/utils.go b/cmd/query/app/utils.go new file mode 100644 index 00000000000..517efde34ff --- /dev/null +++ b/cmd/query/app/utils.go @@ -0,0 +1,39 @@ +package app + +import ( + "go.uber.org/zap" + + istorage "github.com/jaegertracing/jaeger/storage" + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" +) + +// ArchiveOptions returns an instance of QueryServiceOptions based on readers/writers created from storageFactory +func ArchiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { + archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) + if !ok { + logger.Info("Archive storage not supported by the factory") + return querysvc.QueryServiceOptions{} + } + reader, err := archiveFactory.CreateArchiveSpanReader() + if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + logger.Info("Archive storage not created", zap.String("reason", err.Error())) + return querysvc.QueryServiceOptions{} + } + if err != nil { + logger.Error("Cannot init archive storage reader", zap.Error(err)) + return querysvc.QueryServiceOptions{} + } + writer, err := archiveFactory.CreateArchiveSpanWriter() + if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + logger.Info("Archive storage not created", zap.String("reason", err.Error())) + return querysvc.QueryServiceOptions{} + } + if err != nil { + logger.Error("Cannot init archive storage writer", zap.Error(err)) + return querysvc.QueryServiceOptions{} + } + return querysvc.QueryServiceOptions { + ArchiveSpanReader: reader, + ArchiveSpanWriter: writer, + } +} \ No newline at end of file diff --git a/cmd/query/main.go b/cmd/query/main.go index ead0e81e56c..768ed2a44b7 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -116,7 +116,7 @@ func main() { if err != nil { logger.Fatal("Failed to create dependency reader", zap.Error(err)) } - queryServiceOptions := archiveOptions(storageFactory, logger) + queryServiceOptions := ArchiveOptions(storageFactory, logger) queryService := querysvc.NewQueryService( spanReader, dependencyReader, @@ -180,33 +180,3 @@ func main() { os.Exit(1) } } - -func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { - archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) - if !ok { - logger.Info("Archive storage not supported by the factory") - return querysvc.QueryServiceOptions{} - } - reader, err := archiveFactory.CreateArchiveSpanReader() - if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { - logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return querysvc.QueryServiceOptions{} - } - if err != nil { - logger.Error("Cannot init archive storage reader", zap.Error(err)) - return querysvc.QueryServiceOptions{} - } - writer, err := archiveFactory.CreateArchiveSpanWriter() - if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { - logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return querysvc.QueryServiceOptions{} - } - if err != nil { - logger.Error("Cannot init archive storage writer", zap.Error(err)) - return querysvc.QueryServiceOptions{} - } - return querysvc.QueryServiceOptions{ - ArchiveSpanReader: reader, - ArchiveSpanWriter: writer, - } -} From e7400a0513e4e62f503f7e4557aa5937ccf93b94 Mon Sep 17 00:00:00 2001 From: Annanay Date: Thu, 7 Feb 2019 18:55:44 +0530 Subject: [PATCH 07/17] Nit, fix tests Signed-off-by: Annanay --- cmd/all-in-one/main.go | 3 +-- cmd/query/main.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index 13debcd3b89..4ac7046db5a 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -60,7 +60,6 @@ import ( "github.com/jaegertracing/jaeger/pkg/version" ss "github.com/jaegertracing/jaeger/plugin/sampling/strategystore" "github.com/jaegertracing/jaeger/plugin/storage" - istorage "github.com/jaegertracing/jaeger/storage" "github.com/jaegertracing/jaeger/storage/dependencystore" "github.com/jaegertracing/jaeger/storage/spanstore" storageMetrics "github.com/jaegertracing/jaeger/storage/spanstore/metrics" @@ -363,7 +362,7 @@ func startQuery( spanReader = storageMetrics.NewReadMetricsDecorator(spanReader, baseFactory.Namespace(metrics.NSOptions{Name: "query", Tags: nil})) qs := querysvc.NewQueryService(spanReader, depReader, queryOpts) - handlerOpts := []HandlerOption{queryApp.HandlerOptions.Logger(logger), queryApp.HandlerOptions.Tracer(tracer)} + handlerOpts := []queryApp.HandlerOption{queryApp.HandlerOptions.Logger(logger), queryApp.HandlerOptions.Tracer(tracer)} apiHandler := queryApp.NewAPIHandler( qs, handlerOpts...) diff --git a/cmd/query/main.go b/cmd/query/main.go index 768ed2a44b7..4ade787aab5 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -42,7 +42,6 @@ import ( "github.com/jaegertracing/jaeger/pkg/recoveryhandler" "github.com/jaegertracing/jaeger/pkg/version" "github.com/jaegertracing/jaeger/plugin/storage" - istorage "github.com/jaegertracing/jaeger/storage" storageMetrics "github.com/jaegertracing/jaeger/storage/spanstore/metrics" ) @@ -116,7 +115,7 @@ func main() { if err != nil { logger.Fatal("Failed to create dependency reader", zap.Error(err)) } - queryServiceOptions := ArchiveOptions(storageFactory, logger) + queryServiceOptions := app.ArchiveOptions(storageFactory, logger) queryService := querysvc.NewQueryService( spanReader, dependencyReader, From 2e952225eea94a155d2d748eb96164c016b25e32 Mon Sep 17 00:00:00 2001 From: Annanay Date: Thu, 7 Feb 2019 19:15:41 +0530 Subject: [PATCH 08/17] Run make format Signed-off-by: Annanay --- cmd/query/app/utils.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/cmd/query/app/utils.go b/cmd/query/app/utils.go index 517efde34ff..ce048fdf1b7 100644 --- a/cmd/query/app/utils.go +++ b/cmd/query/app/utils.go @@ -1,10 +1,24 @@ +// Copyright (c) 2019 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package app import ( "go.uber.org/zap" - istorage "github.com/jaegertracing/jaeger/storage" "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" + istorage "github.com/jaegertracing/jaeger/storage" ) // ArchiveOptions returns an instance of QueryServiceOptions based on readers/writers created from storageFactory @@ -32,8 +46,8 @@ func ArchiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysv logger.Error("Cannot init archive storage writer", zap.Error(err)) return querysvc.QueryServiceOptions{} } - return querysvc.QueryServiceOptions { + return querysvc.QueryServiceOptions{ ArchiveSpanReader: reader, ArchiveSpanWriter: writer, } -} \ No newline at end of file +} From 5673311a947115640fac1a235ed9d3046de1f39f Mon Sep 17 00:00:00 2001 From: Annanay Date: Thu, 7 Feb 2019 23:52:15 +0530 Subject: [PATCH 09/17] Rename file, remove alias Signed-off-by: Annanay --- cmd/query/app/{utils.go => archive_options.go} | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) rename cmd/query/app/{utils.go => archive_options.go} (79%) diff --git a/cmd/query/app/utils.go b/cmd/query/app/archive_options.go similarity index 79% rename from cmd/query/app/utils.go rename to cmd/query/app/archive_options.go index ce048fdf1b7..1d9dd93a2a9 100644 --- a/cmd/query/app/utils.go +++ b/cmd/query/app/archive_options.go @@ -18,18 +18,18 @@ import ( "go.uber.org/zap" "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" - istorage "github.com/jaegertracing/jaeger/storage" + "github.com/jaegertracing/jaeger/storage" ) // ArchiveOptions returns an instance of QueryServiceOptions based on readers/writers created from storageFactory -func ArchiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { - archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) +func ArchiveOptions(storageFactory storage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { + archiveFactory, ok := storageFactory.(storage.ArchiveFactory) if !ok { logger.Info("Archive storage not supported by the factory") return querysvc.QueryServiceOptions{} } reader, err := archiveFactory.CreateArchiveSpanReader() - if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + if err == storage.ErrArchiveStorageNotConfigured || err == storage.ErrArchiveStorageNotSupported { logger.Info("Archive storage not created", zap.String("reason", err.Error())) return querysvc.QueryServiceOptions{} } @@ -38,7 +38,7 @@ func ArchiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysv return querysvc.QueryServiceOptions{} } writer, err := archiveFactory.CreateArchiveSpanWriter() - if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + if err == storage.ErrArchiveStorageNotConfigured || err == storage.ErrArchiveStorageNotSupported { logger.Info("Archive storage not created", zap.String("reason", err.Error())) return querysvc.QueryServiceOptions{} } From bcbb09a21bdf45c2021f739df355a50bed7c4cad Mon Sep 17 00:00:00 2001 From: Annanay Date: Fri, 8 Feb 2019 23:32:18 +0530 Subject: [PATCH 10/17] Add empty test for coverage Signed-off-by: Annanay --- cmd/query/app/http_handler.go | 6 +++++- cmd/query/app/querysvc/empty_test.go | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 cmd/query/app/querysvc/empty_test.go diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index 7f0c868ca04..4c54d2f350e 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -386,7 +386,11 @@ func (aH *APIHandler) archiveTrace(w http.ResponseWriter, r *http.Request) { // QueryService.ArchiveTrace can now archive this traceID. err := aH.queryService.ArchiveTrace(r.Context(), traceID) - if aH.handleError(w, err, http.StatusNotFound) { + if err == spanstore.ErrTraceNotFound { + aH.handleError(w, err, http.StatusNotFound) + return + } + if aH.handleError(w, err, http.StatusInternalServerError) { return } diff --git a/cmd/query/app/querysvc/empty_test.go b/cmd/query/app/querysvc/empty_test.go new file mode 100644 index 00000000000..b9792b2f5c6 --- /dev/null +++ b/cmd/query/app/querysvc/empty_test.go @@ -0,0 +1,15 @@ +// Copyright (c) 2019 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package querysvc From 3dbbee1ac7ee77f3dcc9698b6694e7d9b8db49c9 Mon Sep 17 00:00:00 2001 From: Annanay Date: Tue, 12 Feb 2019 10:19:40 +0530 Subject: [PATCH 11/17] Remove unreferenced function QueryService.FindTraceIDs Signed-off-by: Annanay --- cmd/query/app/querysvc/query_service.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go index 0c7036209b3..f740adb0a91 100644 --- a/cmd/query/app/querysvc/query_service.go +++ b/cmd/query/app/querysvc/query_service.go @@ -85,11 +85,6 @@ func (qs QueryService) FindTraces(ctx context.Context, query *spanstore.TraceQue return qs.spanReader.FindTraces(ctx, query) } -// FindTraceIDs is the queryService implementation of spanstore.Reader.FindTraceIDs -func (qs QueryService) FindTraceIDs(ctx context.Context, query *spanstore.TraceQueryParameters) ([]model.TraceID, error) { - return qs.spanReader.FindTraceIDs(ctx, query) -} - // ArchiveTrace is the queryService utility to archive traces. func (qs QueryService) ArchiveTrace(ctx context.Context, traceID model.TraceID) error { if qs.options.ArchiveSpanWriter == nil { From 578eb0b79c15516d9385f2026ef0ee9aab1296fe Mon Sep 17 00:00:00 2001 From: Annanay Date: Thu, 14 Feb 2019 01:07:56 +0530 Subject: [PATCH 12/17] WIP - Fix codecov coverage, add a few querysvc tests Signed-off-by: Annanay --- cmd/query/app/querysvc/empty_test.go | 15 --- cmd/query/app/querysvc/query_service_test.go | 117 +++++++++++++++++++ 2 files changed, 117 insertions(+), 15 deletions(-) delete mode 100644 cmd/query/app/querysvc/empty_test.go create mode 100644 cmd/query/app/querysvc/query_service_test.go diff --git a/cmd/query/app/querysvc/empty_test.go b/cmd/query/app/querysvc/empty_test.go deleted file mode 100644 index b9792b2f5c6..00000000000 --- a/cmd/query/app/querysvc/empty_test.go +++ /dev/null @@ -1,15 +0,0 @@ -// Copyright (c) 2019 The Jaeger Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package querysvc diff --git a/cmd/query/app/querysvc/query_service_test.go b/cmd/query/app/querysvc/query_service_test.go new file mode 100644 index 00000000000..258ced4624d --- /dev/null +++ b/cmd/query/app/querysvc/query_service_test.go @@ -0,0 +1,117 @@ +// Copyright (c) 2019 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package querysvc + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + + "github.com/jaegertracing/jaeger/model" + "github.com/jaegertracing/jaeger/model/adjuster" + depsmocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks" + "github.com/jaegertracing/jaeger/storage/spanstore" + spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks" +) + + +var ( + mockTraceID = model.NewTraceID(0, 123456) + mockTrace = &model.Trace{ + Spans: []*model.Span{ + { + TraceID: mockTraceID, + SpanID: model.NewSpanID(1), + Process: &model.Process{}, + }, + { + TraceID: mockTraceID, + SpanID: model.NewSpanID(2), + Process: &model.Process{}, + }, + }, + Warnings: []string{}, + } +) + +func initializeTestServiceWithArchiveOptions() (*QueryService, *spanstoremocks.Reader, *depsmocks.Reader, *spanstoremocks.Reader, *spanstoremocks.Writer) { + readStorage := &spanstoremocks.Reader{} + dependencyStorage := &depsmocks.Reader{} + archiveReadStorage := &spanstoremocks.Reader{} + archiveWriteStorage := &spanstoremocks.Writer{} + options := QueryServiceOptions{ + ArchiveSpanReader: archiveReadStorage, + ArchiveSpanWriter: archiveWriteStorage, + } + qs := NewQueryService(readStorage, dependencyStorage, options) + return qs, readStorage, dependencyStorage, archiveReadStorage, archiveWriteStorage +} + +func initializeTestServiceWithAdjustOption() (*QueryService, *spanstoremocks.Reader, *depsmocks.Reader) { + readStorage := &spanstoremocks.Reader{} + dependencyStorage := &depsmocks.Reader{} + options := QueryServiceOptions{ + Adjuster: adjuster.Sequence(StandardAdjusters...), + } + qs := NewQueryService(readStorage, dependencyStorage, options) + return qs, readStorage, dependencyStorage +} + +func initializeTestService() (*QueryService, *spanstoremocks.Reader, *depsmocks.Reader) { + readStorage := &spanstoremocks.Reader{} + dependencyStorage := &depsmocks.Reader{} + qs := NewQueryService(readStorage, dependencyStorage, QueryServiceOptions{}) + return qs, readStorage, dependencyStorage +} + +// Test QueryService.GetTrace() +func TestGetTraceSuccess(t *testing.T) { + qs, readMock, _ := initializeTestService() + readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(mockTrace, nil).Once() + + ctx := context.Background() + res, err := qs.GetTrace(context.WithValue(ctx, "foo", "bar"), mockTraceID) + assert.NoError(t, err) + assert.Equal(t, res, mockTrace) +} + +// Test QueryService.GetTrace() without ArchiveSpanReader +func TestGetTraceNotFound(t *testing.T) { + qs, readMock, _ := initializeTestService() + readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(nil, spanstore.ErrTraceNotFound).Once() + + ctx := context.Background() + _, err := qs.GetTrace(context.WithValue(ctx, "foo", "bar"), mockTraceID) + assert.Equal(t, err, spanstore.ErrTraceNotFound) +} + +// Test QueryService.GetTrace() with ArchiveSpanReader +func TestGetTraceFromArchiveStorage(t *testing.T) { + qs, readMock, _, readArchiveMock, _ := initializeTestServiceWithArchiveOptions() + readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(nil, spanstore.ErrTraceNotFound).Once() + readArchiveMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(mockTrace, nil).Once() + + ctx := context.Background() + res, err := qs.GetTrace(context.WithValue(ctx, "foo", "bar"), mockTraceID) + assert.NoError(t, err) + assert.Equal(t, res, mockTrace) +} From d48cc0c27b5079b8519cd89cf09355ae7dacf5c4 Mon Sep 17 00:00:00 2001 From: Annanay Date: Thu, 14 Feb 2019 01:10:38 +0530 Subject: [PATCH 13/17] Nit, fix imports Signed-off-by: Annanay --- cmd/query/app/querysvc/query_service_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/query/app/querysvc/query_service_test.go b/cmd/query/app/querysvc/query_service_test.go index 258ced4624d..891540c0320 100644 --- a/cmd/query/app/querysvc/query_service_test.go +++ b/cmd/query/app/querysvc/query_service_test.go @@ -16,7 +16,6 @@ package querysvc import ( "context" - "errors" "testing" "github.com/stretchr/testify/assert" From 34a4989d3acf8642f47a846fe52c75788693e3af Mon Sep 17 00:00:00 2001 From: Annanay Date: Fri, 15 Feb 2019 00:38:01 +0530 Subject: [PATCH 14/17] Fix codecov coverage with tests for querysvc Signed-off-by: Annanay --- cmd/all-in-one/main.go | 33 +++- cmd/query/app/archive_options.go | 53 ------- cmd/query/app/querysvc/query_service_test.go | 154 ++++++++++++++++++- cmd/query/main.go | 33 +++- 4 files changed, 212 insertions(+), 61 deletions(-) delete mode 100644 cmd/query/app/archive_options.go diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index 4ac7046db5a..40cdbec8b0d 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -60,6 +60,7 @@ import ( "github.com/jaegertracing/jaeger/pkg/version" ss "github.com/jaegertracing/jaeger/plugin/sampling/strategystore" "github.com/jaegertracing/jaeger/plugin/storage" + istorage "github.com/jaegertracing/jaeger/storage" "github.com/jaegertracing/jaeger/storage/dependencystore" "github.com/jaegertracing/jaeger/storage/spanstore" storageMetrics "github.com/jaegertracing/jaeger/storage/spanstore/metrics" @@ -142,7 +143,7 @@ func main() { startAgent(aOpts, repOpts, tchannelRepOpts, grpcRepOpts, cOpts, logger, metricsFactory) grpcServer := startCollector(cOpts, spanWriter, logger, metricsFactory, strategyStore, hc) - startQuery(qOpts, spanReader, dependencyReader, logger, rootMetricsFactory, metricsFactory, mBldr, hc, queryApp.ArchiveOptions(storageFactory, logger)) + startQuery(qOpts, spanReader, dependencyReader, logger, rootMetricsFactory, metricsFactory, mBldr, hc, archiveOptions(storageFactory, logger)) hc.Ready() <-signalsChannel logger.Info("Shutting down") @@ -405,3 +406,33 @@ func initSamplingStrategyStore( } return strategyStore } + +func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { + archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) + if !ok { + logger.Info("Archive storage not supported by the factory") + return querysvc.QueryServiceOptions{} + } + reader, err := archiveFactory.CreateArchiveSpanReader() + if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + logger.Info("Archive storage not created", zap.String("reason", err.Error())) + return querysvc.QueryServiceOptions{} + } + if err != nil { + logger.Error("Cannot init archive storage reader", zap.Error(err)) + return querysvc.QueryServiceOptions{} + } + writer, err := archiveFactory.CreateArchiveSpanWriter() + if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + logger.Info("Archive storage not created", zap.String("reason", err.Error())) + return querysvc.QueryServiceOptions{} + } + if err != nil { + logger.Error("Cannot init archive storage writer", zap.Error(err)) + return querysvc.QueryServiceOptions{} + } + return querysvc.QueryServiceOptions{ + ArchiveSpanReader: reader, + ArchiveSpanWriter: writer, + } +} \ No newline at end of file diff --git a/cmd/query/app/archive_options.go b/cmd/query/app/archive_options.go deleted file mode 100644 index 1d9dd93a2a9..00000000000 --- a/cmd/query/app/archive_options.go +++ /dev/null @@ -1,53 +0,0 @@ -// Copyright (c) 2019 The Jaeger Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package app - -import ( - "go.uber.org/zap" - - "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" - "github.com/jaegertracing/jaeger/storage" -) - -// ArchiveOptions returns an instance of QueryServiceOptions based on readers/writers created from storageFactory -func ArchiveOptions(storageFactory storage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { - archiveFactory, ok := storageFactory.(storage.ArchiveFactory) - if !ok { - logger.Info("Archive storage not supported by the factory") - return querysvc.QueryServiceOptions{} - } - reader, err := archiveFactory.CreateArchiveSpanReader() - if err == storage.ErrArchiveStorageNotConfigured || err == storage.ErrArchiveStorageNotSupported { - logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return querysvc.QueryServiceOptions{} - } - if err != nil { - logger.Error("Cannot init archive storage reader", zap.Error(err)) - return querysvc.QueryServiceOptions{} - } - writer, err := archiveFactory.CreateArchiveSpanWriter() - if err == storage.ErrArchiveStorageNotConfigured || err == storage.ErrArchiveStorageNotSupported { - logger.Info("Archive storage not created", zap.String("reason", err.Error())) - return querysvc.QueryServiceOptions{} - } - if err != nil { - logger.Error("Cannot init archive storage writer", zap.Error(err)) - return querysvc.QueryServiceOptions{} - } - return querysvc.QueryServiceOptions{ - ArchiveSpanReader: reader, - ArchiveSpanWriter: writer, - } -} diff --git a/cmd/query/app/querysvc/query_service_test.go b/cmd/query/app/querysvc/query_service_test.go index 891540c0320..4e13e314b88 100644 --- a/cmd/query/app/querysvc/query_service_test.go +++ b/cmd/query/app/querysvc/query_service_test.go @@ -16,7 +16,9 @@ package querysvc import ( "context" + "errors" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -28,8 +30,15 @@ import ( spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks" ) +const millisToNanosMultiplier = int64(time.Millisecond / time.Nanosecond) var ( + errStorageMsg = "Storage error" + errStorage = errors.New(errStorageMsg) + errAdjustment = errors.New("Adjustment error") + + defaultDependencyLookbackDuration = time.Hour * 24 + mockTraceID = model.NewTraceID(0, 123456) mockTrace = &model.Trace{ Spans: []*model.Span{ @@ -61,14 +70,16 @@ func initializeTestServiceWithArchiveOptions() (*QueryService, *spanstoremocks.R return qs, readStorage, dependencyStorage, archiveReadStorage, archiveWriteStorage } -func initializeTestServiceWithAdjustOption() (*QueryService, *spanstoremocks.Reader, *depsmocks.Reader) { +func initializeTestServiceWithAdjustOption() *QueryService { readStorage := &spanstoremocks.Reader{} dependencyStorage := &depsmocks.Reader{} options := QueryServiceOptions{ - Adjuster: adjuster.Sequence(StandardAdjusters...), + Adjuster: adjuster.Func(func(trace *model.Trace) (*model.Trace, error) { + return trace, errAdjustment + }), } qs := NewQueryService(readStorage, dependencyStorage, options) - return qs, readStorage, dependencyStorage + return qs } func initializeTestService() (*QueryService, *spanstoremocks.Reader, *depsmocks.Reader) { @@ -84,8 +95,9 @@ func TestGetTraceSuccess(t *testing.T) { readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). Return(mockTrace, nil).Once() + type contextKey string ctx := context.Background() - res, err := qs.GetTrace(context.WithValue(ctx, "foo", "bar"), mockTraceID) + res, err := qs.GetTrace(context.WithValue(ctx, contextKey("foo"), "bar"), mockTraceID) assert.NoError(t, err) assert.Equal(t, res, mockTrace) } @@ -96,8 +108,9 @@ func TestGetTraceNotFound(t *testing.T) { readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). Return(nil, spanstore.ErrTraceNotFound).Once() + type contextKey string ctx := context.Background() - _, err := qs.GetTrace(context.WithValue(ctx, "foo", "bar"), mockTraceID) + _, err := qs.GetTrace(context.WithValue(ctx, contextKey("foo"), "bar"), mockTraceID) assert.Equal(t, err, spanstore.ErrTraceNotFound) } @@ -109,8 +122,137 @@ func TestGetTraceFromArchiveStorage(t *testing.T) { readArchiveMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). Return(mockTrace, nil).Once() + type contextKey string ctx := context.Background() - res, err := qs.GetTrace(context.WithValue(ctx, "foo", "bar"), mockTraceID) + res, err := qs.GetTrace(context.WithValue(ctx, contextKey("foo"), "bar"), mockTraceID) assert.NoError(t, err) assert.Equal(t, res, mockTrace) } + +// Test QueryService.GetServices() for success. +func TestGetServices(t *testing.T) { + qs, readMock, _ := initializeTestService() + expectedServices := []string{"trifle", "bling"} + readMock.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil).Once() + + type contextKey string + ctx := context.Background() + actualServices, err := qs.GetServices(context.WithValue(ctx, contextKey("foo"), "bar")) + assert.NoError(t, err) + assert.Equal(t, expectedServices, actualServices) +} + +// Test QueryService.GetOperations() for success. +func TestGetOperations(t *testing.T) { + qs, readMock, _ := initializeTestService() + expectedOperations := []string{"", "get"} + readMock.On("GetOperations", mock.AnythingOfType("*context.valueCtx"), "abc/trifle").Return(expectedOperations, nil).Once() + + type contextKey string + ctx := context.Background() + actualOperations, err := qs.GetOperations(context.WithValue(ctx, contextKey("foo"), "bar"), "abc/trifle") + assert.NoError(t, err) + assert.Equal(t, expectedOperations, actualOperations) +} + +// Test QueryService.FindTraces() for success. +func TestFindTraces(t *testing.T) { + qs, readMock, _ := initializeTestService() + readMock.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")). + Return([]*model.Trace{mockTrace}, nil).Once() + + type contextKey string + ctx := context.Background() + duration, _ := time.ParseDuration("20ms") + params := &spanstore.TraceQueryParameters{ + ServiceName: "service", + OperationName: "operation", + StartTimeMax: time.Now(), + DurationMin: duration, + NumTraces: 200, + } + traces, err := qs.FindTraces(context.WithValue(ctx, contextKey("foo"), "bar"), params) + assert.NoError(t, err) + assert.Len(t, traces, 1) +} + +// Test QueryService.ArchiveTrace() with no ArchiveSpanWriter. +func TestArchiveTraceNoOptions(t *testing.T) { + qs, _, _ := initializeTestService() + + type contextKey string + ctx := context.Background() + err := qs.ArchiveTrace(context.WithValue(ctx, contextKey("foo"), "bar"), mockTraceID) + assert.Equal(t, errNoArchiveSpanStorage, err) +} + +// Test QueryService.ArchiveTrace() with ArchiveSpanWriter but invalid traceID. +func TestArchiveTraceWithInvalidTraceID(t *testing.T) { + qs, readMock, _, readArchiveMock, _ := initializeTestServiceWithArchiveOptions() + readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(nil, spanstore.ErrTraceNotFound).Once() + readArchiveMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(nil, spanstore.ErrTraceNotFound).Once() + + type contextKey string + ctx := context.Background() + err := qs.ArchiveTrace(context.WithValue(ctx, contextKey("foo"), "bar"), mockTraceID) + assert.Equal(t, spanstore.ErrTraceNotFound, err) +} + +// Test QueryService.ArchiveTrace(), save error with ArchiveSpanWriter. +func TestArchiveTraceWithArchiveWriterError(t *testing.T) { + qs, readMock, _, _, writeMock := initializeTestServiceWithArchiveOptions() + readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(mockTrace, nil).Once() + writeMock.On("WriteSpan", mock.AnythingOfType("*model.Span")). + Return(errors.New("cannot save")).Times(2) + + type contextKey string + ctx := context.Background() + multiErr := qs.ArchiveTrace(context.WithValue(ctx, contextKey("foo"), "bar"), mockTraceID) + assert.Len(t, multiErr, 2) + // There are two spans in the mockTrace, ArchiveTrace should return a wrapped error. + assert.EqualError(t, multiErr, "[cannot save, cannot save]") +} + +// Test QueryService.ArchiveTrace() with correctly configured ArchiveSpanWriter. +func TestArchiveTraceSuccess(t *testing.T) { + qs, readMock, _, _, writeMock := initializeTestServiceWithArchiveOptions() + readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(mockTrace, nil).Once() + writeMock.On("WriteSpan", mock.AnythingOfType("*model.Span")). + Return(nil).Times(2) + + type contextKey string + ctx := context.Background() + err := qs.ArchiveTrace(context.WithValue(ctx, contextKey("foo"), "bar"), mockTraceID) + assert.NoError(t, err) +} + +// Test QueryService.Adjust() +func TestTraceAdjustmentFailure(t *testing.T) { + qs := initializeTestServiceWithAdjustOption() + + _, err := qs.Adjust(mockTrace) + assert.Error(t, err) + assert.EqualValues(t, errAdjustment.Error(), err.Error()) +} + +// Test QueryService.GetDependencies() +func TestGetDependencies(t *testing.T) { + qs, _, depsMock := initializeTestService() + expectedDependencies := []model.DependencyLink{ + { + Parent: "killer", + Child: "queen", + CallCount: 12, + }, + } + endTs := time.Unix(0, 1476374248550*millisToNanosMultiplier) + depsMock.On("GetDependencies", endTs, defaultDependencyLookbackDuration).Return(expectedDependencies, nil).Times(1) + + actualDependencies, err := qs.GetDependencies(time.Unix(0, 1476374248550*millisToNanosMultiplier), defaultDependencyLookbackDuration) + assert.NoError(t, err) + assert.Equal(t, expectedDependencies, actualDependencies) +} \ No newline at end of file diff --git a/cmd/query/main.go b/cmd/query/main.go index 4ade787aab5..b7d25cdbddb 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -42,6 +42,7 @@ import ( "github.com/jaegertracing/jaeger/pkg/recoveryhandler" "github.com/jaegertracing/jaeger/pkg/version" "github.com/jaegertracing/jaeger/plugin/storage" + istorage "github.com/jaegertracing/jaeger/storage" storageMetrics "github.com/jaegertracing/jaeger/storage/spanstore/metrics" ) @@ -115,7 +116,7 @@ func main() { if err != nil { logger.Fatal("Failed to create dependency reader", zap.Error(err)) } - queryServiceOptions := app.ArchiveOptions(storageFactory, logger) + queryServiceOptions := archiveOptions(storageFactory, logger) queryService := querysvc.NewQueryService( spanReader, dependencyReader, @@ -179,3 +180,33 @@ func main() { os.Exit(1) } } + +func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysvc.QueryServiceOptions { + archiveFactory, ok := storageFactory.(istorage.ArchiveFactory) + if !ok { + logger.Info("Archive storage not supported by the factory") + return querysvc.QueryServiceOptions{} + } + reader, err := archiveFactory.CreateArchiveSpanReader() + if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + logger.Info("Archive storage not created", zap.String("reason", err.Error())) + return querysvc.QueryServiceOptions{} + } + if err != nil { + logger.Error("Cannot init archive storage reader", zap.Error(err)) + return querysvc.QueryServiceOptions{} + } + writer, err := archiveFactory.CreateArchiveSpanWriter() + if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported { + logger.Info("Archive storage not created", zap.String("reason", err.Error())) + return querysvc.QueryServiceOptions{} + } + if err != nil { + logger.Error("Cannot init archive storage writer", zap.Error(err)) + return querysvc.QueryServiceOptions{} + } + return querysvc.QueryServiceOptions{ + ArchiveSpanReader: reader, + ArchiveSpanWriter: writer, + } +} \ No newline at end of file From 72f039ec2e22d40116a15f34be024097e52674fe Mon Sep 17 00:00:00 2001 From: Annanay Date: Fri, 15 Feb 2019 00:57:54 +0530 Subject: [PATCH 15/17] Make format Signed-off-by: Annanay --- cmd/all-in-one/main.go | 2 +- cmd/query/app/querysvc/query_service_test.go | 8 ++++---- cmd/query/main.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index 40cdbec8b0d..a0c0e87ff95 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -435,4 +435,4 @@ func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysv ArchiveSpanReader: reader, ArchiveSpanWriter: writer, } -} \ No newline at end of file +} diff --git a/cmd/query/app/querysvc/query_service_test.go b/cmd/query/app/querysvc/query_service_test.go index 4e13e314b88..3e8ff1aa862 100644 --- a/cmd/query/app/querysvc/query_service_test.go +++ b/cmd/query/app/querysvc/query_service_test.go @@ -129,7 +129,7 @@ func TestGetTraceFromArchiveStorage(t *testing.T) { assert.Equal(t, res, mockTrace) } -// Test QueryService.GetServices() for success. +// Test QueryService.GetServices() for success. func TestGetServices(t *testing.T) { qs, readMock, _ := initializeTestService() expectedServices := []string{"trifle", "bling"} @@ -142,7 +142,7 @@ func TestGetServices(t *testing.T) { assert.Equal(t, expectedServices, actualServices) } -// Test QueryService.GetOperations() for success. +// Test QueryService.GetOperations() for success. func TestGetOperations(t *testing.T) { qs, readMock, _ := initializeTestService() expectedOperations := []string{"", "get"} @@ -155,7 +155,7 @@ func TestGetOperations(t *testing.T) { assert.Equal(t, expectedOperations, actualOperations) } -// Test QueryService.FindTraces() for success. +// Test QueryService.FindTraces() for success. func TestFindTraces(t *testing.T) { qs, readMock, _ := initializeTestService() readMock.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")). @@ -255,4 +255,4 @@ func TestGetDependencies(t *testing.T) { actualDependencies, err := qs.GetDependencies(time.Unix(0, 1476374248550*millisToNanosMultiplier), defaultDependencyLookbackDuration) assert.NoError(t, err) assert.Equal(t, expectedDependencies, actualDependencies) -} \ No newline at end of file +} diff --git a/cmd/query/main.go b/cmd/query/main.go index b7d25cdbddb..ead0e81e56c 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -209,4 +209,4 @@ func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) querysv ArchiveSpanReader: reader, ArchiveSpanWriter: writer, } -} \ No newline at end of file +} From d56745a9ca33ae736046dcbcfba2c2c4e6817549 Mon Sep 17 00:00:00 2001 From: Annanay Date: Sat, 16 Feb 2019 11:49:54 +0530 Subject: [PATCH 16/17] Fix codecov coverage for APIHandler Signed-off-by: Annanay --- cmd/query/app/handler_archive_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/cmd/query/app/handler_archive_test.go b/cmd/query/app/handler_archive_test.go index e640081c000..846d8024ac5 100644 --- a/cmd/query/app/handler_archive_test.go +++ b/cmd/query/app/handler_archive_test.go @@ -66,6 +66,32 @@ func TestGetArchivedTraceSuccess(t *testing.T) { }, querysvc.QueryServiceOptions{ArchiveSpanReader: mockReader}) } +// Test failure in parsing trace ID. +func TestArchiveTrace_BadTraceID(t *testing.T) { + withTestServer(t, func(ts *testServer) { + var response structuredResponse + err := postJSON(ts.server.URL+"/api/archive/badtraceid", []string{}, &response) + assert.Error(t, err) + }, querysvc.QueryServiceOptions{}) +} + +// Test return of 404 when trace is not found in APIHandler.archive +func TestArchiveTrace_TraceNotFound(t *testing.T) { + mockReader := &spanstoremocks.Reader{} + mockReader.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(nil, spanstore.ErrTraceNotFound).Once() + mockWriter := &spanstoremocks.Writer{} + // Not actually going to write the trace, so no need to define mockWriter action + withTestServer(t, func(ts *testServer) { + // make main reader return NotFound + ts.spanReader.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(nil, spanstore.ErrTraceNotFound).Once() + var response structuredResponse + err := postJSON(ts.server.URL+"/api/archive/" + mockTraceID.String(), []string{}, &response) + assert.EqualError(t, err, `404 error from server: {"data":null,"total":0,"limit":0,"offset":0,"errors":[{"code":404,"msg":"trace not found"}]}`+"\n") + }, querysvc.QueryServiceOptions{ArchiveSpanReader: mockReader, ArchiveSpanWriter: mockWriter}) +} + func TestArchiveTrace_NoStorage(t *testing.T) { withTestServer(t, func(ts *testServer) { var response structuredResponse From 20859e6d697bebec04224cc3a7f46b05a8c1285e Mon Sep 17 00:00:00 2001 From: Annanay Date: Mon, 18 Feb 2019 22:51:49 +0530 Subject: [PATCH 17/17] Remove duplicate function Signed-off-by: Annanay --- cmd/query/app/http_handler_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go index e0078e60a11..f48a583b25c 100644 --- a/cmd/query/app/http_handler_test.go +++ b/cmd/query/app/http_handler_test.go @@ -109,11 +109,6 @@ func initializeTestServerWithOptions(queryOptions querysvc.QueryServiceOptions, return httptest.NewServer(r), readStorage, dependencyStorage, handler } -func initializeTestServerWithQueryOptions(queryOptions querysvc.QueryServiceOptions, options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader) { - https, sr, dr, _ := initializeTestServerWithHandler(queryOptions, options...) - return https, sr, dr -} - func initializeTestServer(options ...HandlerOption) (*httptest.Server, *spanstoremocks.Reader, *depsmocks.Reader) { https, sr, dr, _ := initializeTestServerWithHandler(querysvc.QueryServiceOptions{}, options...) return https, sr, dr @@ -358,7 +353,7 @@ func TestSearchByTraceIDSuccess(t *testing.T) { func TestSearchByTraceIDSuccessWithArchive(t *testing.T) { archiveReadMock := &spanstoremocks.Reader{} - server, readMock, _ := initializeTestServerWithQueryOptions(querysvc.QueryServiceOptions{ + server, readMock, _, _ := initializeTestServerWithOptions(querysvc.QueryServiceOptions{ ArchiveSpanReader: archiveReadMock, }) defer server.Close()