[query] Query handlers refactoring (#2872)

* Initial commit for proposed query handlers refactoring:
 * updated CustomHandler interface
 * unified how query routes are added. no need to pass router around and wrapping is done in one place.
 * routes are registered with names so they could be easily found later when custom handlers are registered.

* trying to make linter happy

* linter fixes

* revert old behaviour

* Make sure route methods are taken into account when adding and searching for named route.

* fixed code formatting

* [dbnode] Refactor wide query path (#2826)

* [dbnode] Introduce Aggregator type (#2840)

* [coordinator] Set default namespace tag to avoid colliding with commonly used "namespace" label (#2878)

* [coordinator] Set default namespace tag to avoid colliding with common "namespace" default value

* Use defined constant

* Add downsampler test case to demonstrate override namespace tag

Co-authored-by: Wesley Kim <[email protected]>

* Improve some slow tests (#2881)

* Improe some slow tests

* lint

* lint

* goddamn imports

* Changes after code review.

* [query] Remove dead code in prom package (#2871)

* Register separate route for each method.

* linter fixes

* removed code duplication in hasndler_test

* Fail if route was already registered.

* formatted code

* Update src/query/api/v1/httpd/handler_test.go

Co-authored-by: Vilius Pranckaitis <[email protected]>

* Update src/query/api/v1/httpd/handler_test.go

Co-authored-by: Vilius Pranckaitis <[email protected]>

* More handler tests.

Co-authored-by: arnikola <[email protected]>
Co-authored-by: Linas Medžiūnas <[email protected]>
Co-authored-by: Rob Skillington <[email protected]>
Co-authored-by: Wesley Kim <[email protected]>
Co-authored-by: Vytenis Darulis <[email protected]>
Co-authored-by: Vilius Pranckaitis <[email protected]>

Author: 7 people

src/query/api/v1/handler/database/common.go

@@ -21,16 +21,12 @@
 package database
 
 import (
-	"net/http"
-
 	clusterclient "github.com/m3db/m3/src/cluster/client"
 	dbconfig "github.com/m3db/m3/src/cmd/services/m3dbnode/config"
 	"github.com/m3db/m3/src/cmd/services/m3query/config"
+	"github.com/m3db/m3/src/query/api/v1/handler"
 	"github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions"
-	"github.com/m3db/m3/src/query/util/logging"
 	"github.com/m3db/m3/src/x/instrument"
-
-	"github.com/gorilla/mux"
 )
 
 // Handler represents a generic handler for namespace endpoints.
@@ -43,31 +39,27 @@ type Handler struct {
 
 // RegisterRoutes registers the namespace routes
 func RegisterRoutes(
-	r *mux.Router,
+	addRoute handler.AddRouteFn,
 	client clusterclient.Client,
 	cfg config.Configuration,
 	embeddedDbCfg *dbconfig.DBConfiguration,
 	defaults []handleroptions.ServiceOptionsDefault,
 	instrumentOpts instrument.Options,
 ) error {
-	wrapped := func(n http.Handler) http.Handler {
-		return logging.WithResponseTimeAndPanicErrorLogging(n, instrumentOpts)
-	}
-
 	createHandler, err := NewCreateHandler(client, cfg, embeddedDbCfg,
 		defaults, instrumentOpts)
 	if err != nil {
 		return err
 	}
 
-	r.HandleFunc(CreateURL,
-		wrapped(createHandler).ServeHTTP).
-		Methods(CreateHTTPMethod)
-
 	// Register the same handler under two different endpoints. This just makes explaining things in
 	// our documentation easier so we can separate out concepts, but share the underlying code.
-	r.HandleFunc(CreateURL, wrapped(createHandler).ServeHTTP).Methods(CreateHTTPMethod)
-	r.HandleFunc(CreateNamespaceURL, wrapped(createHandler).ServeHTTP).Methods(CreateNamespaceHTTPMethod)
+	if err := addRoute(CreateURL, createHandler, CreateHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(CreateNamespaceURL, createHandler, CreateNamespaceHTTPMethod); err != nil {
+		return err
+	}
 
 	return nil
 }

src/query/api/v1/handler/namespace/common.go

@@ -31,13 +31,11 @@ import (
 	"github.com/m3db/m3/src/cluster/kv"
 	nsproto "github.com/m3db/m3/src/dbnode/generated/proto/namespace"
 	"github.com/m3db/m3/src/dbnode/namespace"
+	"github.com/m3db/m3/src/query/api/v1/handler"
 	"github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions"
 	"github.com/m3db/m3/src/query/storage/m3"
-	"github.com/m3db/m3/src/query/util/logging"
 	"github.com/m3db/m3/src/x/instrument"
 	xhttp "github.com/m3db/m3/src/x/net/http"
-
-	"github.com/gorilla/mux"
 )
 
 const (
@@ -102,66 +100,93 @@ func Metadata(store kv.Store) ([]namespace.Metadata, int, error) {
 	return nsMap.Metadatas(), value.Version(), nil
 }
 
+type applyMiddlewareFn func(
+	svc handleroptions.ServiceNameAndDefaults,
+	w http.ResponseWriter,
+	r *http.Request,
+)
+
+type addRouteFn func(
+	path string,
+	applyMiddlewareFn applyMiddlewareFn,
+	methods ...string,
+) error
+
 // RegisterRoutes registers the namespace routes.
 func RegisterRoutes(
-	r *mux.Router,
+	addRouteFn handler.AddRouteFn,
 	client clusterclient.Client,
 	clusters m3.Clusters,
 	defaults []handleroptions.ServiceOptionsDefault,
 	instrumentOpts instrument.Options,
-) {
-	wrapped := func(n http.Handler) http.Handler {
-		return logging.WithResponseTimeAndPanicErrorLogging(n, instrumentOpts)
-	}
-	applyMiddleware := func(
-		f func(svc handleroptions.ServiceNameAndDefaults,
-			w http.ResponseWriter, r *http.Request),
-		defaults []handleroptions.ServiceOptionsDefault,
-	) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			svc := handleroptions.ServiceNameAndDefaults{
-				ServiceName: handleroptions.M3DBServiceName,
-				Defaults:    defaults,
-			}
-			f(svc, w, r)
-		})
-	}
+) error {
+	addRoute := applyMiddlewareToRoute(addRouteFn, defaults)
 
 	// Get M3DB namespaces.
-	getHandler := wrapped(
-		applyMiddleware(NewGetHandler(client, instrumentOpts).ServeHTTP, defaults))
-	r.HandleFunc(M3DBGetURL, getHandler.ServeHTTP).Methods(GetHTTPMethod)
+	getHandler := NewGetHandler(client, instrumentOpts).ServeHTTP
+	if err := addRoute(M3DBGetURL, getHandler, GetHTTPMethod); err != nil {
+		return err
+	}
 
 	// Add M3DB namespaces.
-	addHandler := wrapped(
-		applyMiddleware(NewAddHandler(client, instrumentOpts).ServeHTTP, defaults))
-	r.HandleFunc(M3DBAddURL, addHandler.ServeHTTP).Methods(AddHTTPMethod)
+	addHandler := NewAddHandler(client, instrumentOpts).ServeHTTP
+	if err := addRoute(M3DBAddURL, addHandler, AddHTTPMethod); err != nil {
+		return err
+	}
 
 	// Update M3DB namespaces.
-	updateHandler := wrapped(
-		applyMiddleware(NewUpdateHandler(client, instrumentOpts).ServeHTTP, defaults))
-	r.HandleFunc(M3DBUpdateURL, updateHandler.ServeHTTP).Methods(UpdateHTTPMethod)
+	updateHandler := NewUpdateHandler(client, instrumentOpts).ServeHTTP
+	if err := addRoute(M3DBUpdateURL, updateHandler, UpdateHTTPMethod); err != nil {
+		return err
+	}
 
 	// Delete M3DB namespaces.
-	deleteHandler := wrapped(
-		applyMiddleware(NewDeleteHandler(client, instrumentOpts).ServeHTTP, defaults))
-	r.HandleFunc(M3DBDeleteURL, deleteHandler.ServeHTTP).Methods(DeleteHTTPMethod)
+	deleteHandler := NewDeleteHandler(client, instrumentOpts).ServeHTTP
+	if err := addRoute(M3DBDeleteURL, deleteHandler, DeleteHTTPMethod); err != nil {
+		return err
+	}
 
 	// Deploy M3DB schemas.
-	schemaHandler := wrapped(
-		applyMiddleware(NewSchemaHandler(client, instrumentOpts).ServeHTTP, defaults))
-	r.HandleFunc(M3DBSchemaURL, schemaHandler.ServeHTTP).Methods(SchemaDeployHTTPMethod)
+	schemaHandler := NewSchemaHandler(client, instrumentOpts).ServeHTTP
+	if err := addRoute(M3DBSchemaURL, schemaHandler, SchemaDeployHTTPMethod); err != nil {
+		return err
+	}
 
 	// Reset M3DB schemas.
-	schemaResetHandler := wrapped(
-		applyMiddleware(NewSchemaResetHandler(client, instrumentOpts).ServeHTTP, defaults))
-	r.HandleFunc(M3DBSchemaURL, schemaResetHandler.ServeHTTP).Methods(DeleteHTTPMethod)
+	schemaResetHandler := NewSchemaResetHandler(client, instrumentOpts).ServeHTTP
+	if err := addRoute(M3DBSchemaURL, schemaResetHandler, DeleteHTTPMethod); err != nil {
+		return err
+	}
 
 	// Mark M3DB namespace as ready.
-	readyHandler := wrapped(
-		applyMiddleware(NewReadyHandler(client, clusters, instrumentOpts).ServeHTTP, defaults))
-	r.HandleFunc(M3DBReadyURL, readyHandler.ServeHTTP).Methods(ReadyHTTPMethod)
+	readyHandler := NewReadyHandler(client, clusters, instrumentOpts).ServeHTTP
+	if err := addRoute(M3DBReadyURL, readyHandler, ReadyHTTPMethod); err != nil {
+		return err
+	}
+
+	return nil
+}
 
+func applyMiddlewareToRoute(
+	addRouteFn handler.AddRouteFn,
+	defaults []handleroptions.ServiceOptionsDefault,
+) addRouteFn {
+	applyMiddleware := func(
+		applyMiddlewareFn applyMiddlewareFn,
+		defaults []handleroptions.ServiceOptionsDefault,
+	) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			svc := handleroptions.ServiceNameAndDefaults{
+				ServiceName: handleroptions.M3DBServiceName,
+				Defaults:    defaults,
+			}
+			applyMiddlewareFn(svc, w, r)
+		})
+	}
+
+	return func(path string, f applyMiddlewareFn, methods ...string) error {
+		return addRouteFn(path, applyMiddleware(f, defaults), methods...)
+	}
 }
 
 func validateNamespaceAggregationOptions(mds []namespace.Metadata) error {

src/query/api/v1/handler/placement/common.go

@@ -37,13 +37,12 @@ import (
 	"github.com/m3db/m3/src/cluster/services"
 	"github.com/m3db/m3/src/cluster/shard"
 	"github.com/m3db/m3/src/cmd/services/m3query/config"
+	"github.com/m3db/m3/src/query/api/v1/handler"
 	"github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions"
 	"github.com/m3db/m3/src/query/util/logging"
 	xerrors "github.com/m3db/m3/src/x/errors"
 	"github.com/m3db/m3/src/x/instrument"
 	xhttp "github.com/m3db/m3/src/x/net/http"
-
-	"github.com/gorilla/mux"
 )
 
 const (
@@ -222,72 +221,117 @@ func ConvertInstancesProto(instancesProto []*placementpb.Instance) ([]placement.
 
 // RegisterRoutes registers the placement routes
 func RegisterRoutes(
-	r *mux.Router,
+	addRoute handler.AddRouteFn,
 	defaults []handleroptions.ServiceOptionsDefault,
 	opts HandlerOptions,
-) {
+) error {
 	// Init
 	var (
 		initHandler = NewInitHandler(opts)
 		initFn      = applyMiddleware(initHandler.ServeHTTP, defaults, opts.instrumentOptions)
 	)
-	r.HandleFunc(M3DBInitURL, initFn).Methods(InitHTTPMethod)
-	r.HandleFunc(M3AggInitURL, initFn).Methods(InitHTTPMethod)
-	r.HandleFunc(M3CoordinatorInitURL, initFn).Methods(InitHTTPMethod)
+
+	if err := addRoute(M3DBInitURL, initFn, InitHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3AggInitURL, initFn, InitHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3CoordinatorInitURL, initFn, InitHTTPMethod); err != nil {
+		return err
+	}
 
 	// Get
 	var (
 		getHandler = NewGetHandler(opts)
 		getFn      = applyMiddleware(getHandler.ServeHTTP, defaults, opts.instrumentOptions)
 	)
-	r.HandleFunc(M3DBGetURL, getFn).Methods(GetHTTPMethod)
-	r.HandleFunc(M3AggGetURL, getFn).Methods(GetHTTPMethod)
-	r.HandleFunc(M3CoordinatorGetURL, getFn).Methods(GetHTTPMethod)
+	if err := addRoute(M3DBGetURL, getFn, GetHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3AggGetURL, getFn, GetHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3CoordinatorGetURL, getFn, GetHTTPMethod); err != nil {
+		return err
+	}
 
 	// Delete all
 	var (
 		deleteAllHandler = NewDeleteAllHandler(opts)
 		deleteAllFn      = applyMiddleware(deleteAllHandler.ServeHTTP, defaults, opts.instrumentOptions)
 	)
-	r.HandleFunc(M3DBDeleteAllURL, deleteAllFn).Methods(DeleteAllHTTPMethod)
-	r.HandleFunc(M3AggDeleteAllURL, deleteAllFn).Methods(DeleteAllHTTPMethod)
-	r.HandleFunc(M3CoordinatorDeleteAllURL, deleteAllFn).Methods(DeleteAllHTTPMethod)
+	if err := addRoute(M3DBDeleteAllURL, deleteAllFn, DeleteAllHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3AggDeleteAllURL, deleteAllFn, DeleteAllHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3CoordinatorDeleteAllURL, deleteAllFn, DeleteAllHTTPMethod); err != nil {
+		return err
+	}
 
 	// Add
 	var (
 		addHandler = NewAddHandler(opts)
 		addFn      = applyMiddleware(addHandler.ServeHTTP, defaults, opts.instrumentOptions)
 	)
-	r.HandleFunc(M3DBAddURL, addFn).Methods(AddHTTPMethod)
-	r.HandleFunc(M3AggAddURL, addFn).Methods(AddHTTPMethod)
-	r.HandleFunc(M3CoordinatorAddURL, addFn).Methods(AddHTTPMethod)
+	if err := addRoute(M3DBAddURL, addFn, AddHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3AggAddURL, addFn, AddHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3CoordinatorAddURL, addFn, AddHTTPMethod); err != nil {
+		return err
+	}
 
 	// Delete
 	var (
 		deleteHandler = NewDeleteHandler(opts)
 		deleteFn      = applyMiddleware(deleteHandler.ServeHTTP, defaults, opts.instrumentOptions)
 	)
-	r.HandleFunc(M3DBDeleteURL, deleteFn).Methods(DeleteHTTPMethod)
-	r.HandleFunc(M3AggDeleteURL, deleteFn).Methods(DeleteHTTPMethod)
-	r.HandleFunc(M3CoordinatorDeleteURL, deleteFn).Methods(DeleteHTTPMethod)
+	if err := addRoute(M3DBDeleteURL, deleteFn, DeleteHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3AggDeleteURL, deleteFn, DeleteHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3CoordinatorDeleteURL, deleteFn, DeleteHTTPMethod); err != nil {
+		return err
+	}
 
 	// Replace
 	var (
 		replaceHandler = NewReplaceHandler(opts)
 		replaceFn      = applyMiddleware(replaceHandler.ServeHTTP, defaults, opts.instrumentOptions)
 	)
-	r.HandleFunc(M3DBReplaceURL, replaceFn).Methods(ReplaceHTTPMethod)
-	r.HandleFunc(M3AggReplaceURL, replaceFn).Methods(ReplaceHTTPMethod)
-	r.HandleFunc(M3CoordinatorReplaceURL, replaceFn).Methods(ReplaceHTTPMethod)
+	if err := addRoute(M3DBReplaceURL, replaceFn, ReplaceHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3AggReplaceURL, replaceFn, ReplaceHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3CoordinatorReplaceURL, replaceFn, ReplaceHTTPMethod); err != nil {
+		return err
+	}
 
 	// Set
 	var (
 		setHandler = NewSetHandler(opts)
 		setFn      = applyMiddleware(setHandler.ServeHTTP, defaults, opts.instrumentOptions)
 	)
-	r.HandleFunc(M3DBSetURL, setFn).Methods(SetHTTPMethod)
-	r.HandleFunc(M3AggSetURL, setFn).Methods(SetHTTPMethod)
-	r.HandleFunc(M3CoordinatorSetURL, setFn).Methods(SetHTTPMethod)
+	if err := addRoute(M3DBSetURL, setFn, SetHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3AggSetURL, setFn, SetHTTPMethod); err != nil {
+		return err
+	}
+	if err := addRoute(M3CoordinatorSetURL, setFn, SetHTTPMethod); err != nil {
+		return err
+	}
+
+	return nil
 }
 
 func newPlacementCutoverNanosFn(
@@ -386,11 +430,11 @@ func applyMiddleware(
 	f func(svc handleroptions.ServiceNameAndDefaults, w http.ResponseWriter, r *http.Request),
 	defaults []handleroptions.ServiceOptionsDefault,
 	instrumentOpts instrument.Options,
-) func(w http.ResponseWriter, r *http.Request) {
+) http.Handler {
 	return logging.WithResponseTimeAndPanicErrorLoggingFunc(
 		parseServiceMiddleware(f, defaults),
 		instrumentOpts,
-	).ServeHTTP
+	)
 }
 
 func parseServiceMiddleware(