Separate query-service functionality from http handler

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"
@@ -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 := []queryApp.HandlerOption{queryApp.HandlerOptions.Logger(logger), queryApp.HandlerOptions.Tracer(tracer)}
 	apiHandler := queryApp.NewAPIHandler(
-		spanReader,
-		depReader,
+		qs,
 		handlerOpts...)
 
 	r := mux.NewRouter()
@@ -406,32 +407,32 @@ func initSamplingStrategyStore(
 	return strategyStore
 }
 
-func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) []queryApp.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")
-		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 []queryApp.HandlerOption{
-		queryApp.HandlerOptions.ArchiveSpanReader(reader),
-		queryApp.HandlerOptions.ArchiveSpanWriter(writer),
+	return querysvc.QueryServiceOptions{
+		ArchiveSpanReader: reader,
+		ArchiveSpanWriter: writer,
 	}
 }

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,15 +63,41 @@ 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})
+}
+
+// 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
 		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 +110,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 +123,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})
 }

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
@@ -40,13 +37,6 @@ func (handlerOptions) Logger(logger *zap.Logger) HandlerOption {
 	}
 }
 
-// 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...)
-	}
-}
-
 // BasePath creates a HandlerOption that initializes the base path for all HTTP routes
 func (handlerOptions) BasePath(prefix string) HandlerOption {
 	return func(apiHandler *APIHandler) {
@@ -68,20 +58,6 @@ 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
-	}
-}
-
-// ArchiveSpanWriter creates a HandlerOption that initializes lookback duration
-func (handlerOptions) ArchiveSpanWriter(writer spanstore.Writer) HandlerOption {
-	return func(apiHandler *APIHandler) {
-		apiHandler.archiveSpanWriter = writer
-	}
-}
-
 // Tracer creates a HandlerOption that initializes OpenTracing tracer
 func (handlerOptions) Tracer(tracer opentracing.Tracer) HandlerOption {
 	return func(apiHandler *APIHandler) {