Skip to content

Separate HTTP Instrumentation for Querier #2708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 17, 2020
Merged

Separate HTTP Instrumentation for Querier #2708

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff9eaf5
separate instrumentation for the querier
jtlisi Jun 10, 2020
49d2a7b
check for prefix in integration tests
jtlisi Jun 10, 2020
9e9267c
fix linting issues
jtlisi Jun 10, 2020
201d2e4
refactor per PR comments
jtlisi Jun 13, 2020
d8a03fc
fix liniting errors
jtlisi Jun 13, 2020
a8654cc
update the changelog
jtlisi Jun 14, 2020
d64a519
make registerQueryAPI private
jtlisi Jun 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion integration/asserts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var (
serviceMetricsPrefixes = map[ServiceType][]string{
Distributor: {},
Ingester: {"!cortex_ingester_client", "cortex_ingester"}, // The metrics prefix cortex_ingester_client may be used by other components so we ignore it.
Querier: {},
Querier: {"cortex_querier"},
QueryFrontend: {"cortex_frontend", "cortex_query_frontend"},
TableManager: {},
AlertManager: {"cortex_alertmanager"},
Expand Down
80 changes: 51 additions & 29 deletions pkg/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ import (

"github.com/go-kit/kit/log"
"github.com/go-kit/kit/log/level"
"github.com/gorilla/mux"
"github.com/prometheus/common/route"
"github.com/prometheus/prometheus/config"
"github.com/prometheus/prometheus/promql"
"github.com/prometheus/prometheus/storage"
v1 "github.com/prometheus/prometheus/web/api/v1"
"github.com/weaveworks/common/instrument"
"github.com/weaveworks/common/middleware"
"github.com/weaveworks/common/server"

"github.com/gorilla/mux"

"github.com/cortexproject/cortex/pkg/alertmanager"
"github.com/cortexproject/cortex/pkg/chunk/purger"
"github.com/cortexproject/cortex/pkg/compactor"
Expand Down Expand Up @@ -65,17 +65,29 @@ type API struct {
authMiddleware middleware.Func
server *server.Server
logger log.Logger

querierRequestDuration *prometheus.HistogramVec
}

func New(cfg Config, s *server.Server, logger log.Logger) (*API, error) {
func New(cfg Config, s *server.Server, logger log.Logger, reg prometheus.Registerer) (*API, error) {
// Ensure the encoded path is used. Required for the rules API
s.HTTP.UseEncodedPath()

// Prometheus histograms for requests.
querierRequestDuration := prometheus.NewHistogramVec(prometheus.HistogramOpts{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply use promauto.With(reg).NewHistogramVec().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered moving this to RegisterQuerier()? The metric would be registered only if RegisterQuerier() is used and the change isolated there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid passing a prometheus.Registerer to that function. However, maybe it would make more sense to store the Registerer in the API struct and and initiate the metric is in the RegisterQuerier function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, or you can pass the metric itself to the RegisterQuerier() (ie. in Thanos it's quite a common design pattern).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Cool I updated it to pass a metric as a parameter.

Namespace: "cortex",
Name: "querier_request_duration_seconds",
Help: "Time (in seconds) spent serving HTTP requests to the querier.",
Buckets: instrument.DefBuckets,
}, []string{"method", "route", "status_code", "ws"})
reg.MustRegister(querierRequestDuration)

api := &API{
cfg: cfg,
authMiddleware: cfg.HTTPAuthMiddleware,
server: s,
logger: logger,
cfg: cfg,
authMiddleware: cfg.HTTPAuthMiddleware,
server: s,
logger: logger,
querierRequestDuration: querierRequestDuration,
}

// If no authentication middleware is present in the config, use the default authentication middleware.
Expand Down Expand Up @@ -288,10 +300,17 @@ func (a *API) RegisterQuerier(queryable storage.Queryable, engine *promql.Engine
router = a.server.HTTP
}

// Use a separate metric for the querier in order to differentiate requests from the query-frontend when
// running Cortex as a single binary.
inst := middleware.Instrument{
Duration: a.querierRequestDuration,
RouteMatcher: router,
}

promRouter := route.New().WithPrefix(a.cfg.ServerPrefix + a.cfg.PrometheusHTTPPrefix + "/api/v1")
api.Register(promRouter)
cacheGenHeaderMiddleware := getHTTPCacheGenNumberHeaderSetterMiddleware(tombstonesLoader)
promHandler := fakeRemoteAddr(cacheGenHeaderMiddleware.Wrap(promRouter))
promHandler := fakeRemoteAddr(inst.Wrap(cacheGenHeaderMiddleware.Wrap(promRouter)))

a.registerRouteWithRouter(router, a.cfg.PrometheusHTTPPrefix+"/api/v1/read", querier.RemoteReadHandler(queryable), true, "POST")
a.registerRouteWithRouter(router, a.cfg.PrometheusHTTPPrefix+"/api/v1/query", promHandler, true, "GET", "POST")
Expand All @@ -305,7 +324,7 @@ func (a *API) RegisterQuerier(queryable storage.Queryable, engine *promql.Engine

legacyPromRouter := route.New().WithPrefix(a.cfg.ServerPrefix + a.cfg.LegacyHTTPPrefix + "/api/v1")
api.Register(legacyPromRouter)
legacyPromHandler := fakeRemoteAddr(cacheGenHeaderMiddleware.Wrap(legacyPromRouter))
legacyPromHandler := fakeRemoteAddr(inst.Wrap(cacheGenHeaderMiddleware.Wrap(legacyPromRouter)))

a.registerRouteWithRouter(router, a.cfg.LegacyHTTPPrefix+"/api/v1/read", querier.RemoteReadHandler(queryable), true, "POST")
a.registerRouteWithRouter(router, a.cfg.LegacyHTTPPrefix+"/api/v1/query", legacyPromHandler, true, "GET", "POST")
Expand All @@ -331,31 +350,34 @@ func (a *API) RegisterQuerier(queryable storage.Queryable, engine *promql.Engine
}))
}

// RegisterQueryAPI registers the Prometheus routes supported by the
// Cortex querier service. Currently this can not be registered simultaneously
// with the Querier.
func (a *API) RegisterQueryAPI(handler http.Handler) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this refactoring related to the scope of the PR? I'm not against it if you need it in a project vendoring Cortex but, if it's not required, probably the previous version was more clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you comment on this? I'm fine if you need it in a project vendoring Cortex, otherwise the previous version looks more explicit to me.

a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/read", handler, true, "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/query", handler, true, "GET", "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/query_range", handler, true, "GET", "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/labels", handler, true, "GET", "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/label/{name}/values", handler, true, "GET")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/series", handler, true, "GET", "POST", "DELETE")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/metadata", handler, true, "GET")

// Register Legacy Routers
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/read", handler, true, "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/query", handler, true, "GET", "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/query_range", handler, true, "GET", "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/labels", handler, true, "GET", "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/label/{name}/values", handler, true, "GET")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/series", handler, true, "GET", "POST", "DELETE")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/metadata", handler, true, "GET")
}

// RegisterQueryFrontend registers the Prometheus routes supported by the
// Cortex querier service. Currently this can not be registered simultaneously
// with the Querier.
func (a *API) RegisterQueryFrontend(f *frontend.Frontend) {
frontend.RegisterFrontendServer(a.server.GRPC, f)

// Previously the frontend handled all calls to the provided prefix. Instead explicit
// routing is used since it will be required to enable the frontend to be run as part
// of a single binary in the future.
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/read", f.Handler(), true, "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/query", f.Handler(), true, "GET", "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/query_range", f.Handler(), true, "GET", "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/labels", f.Handler(), true, "GET", "POST")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/label/{name}/values", f.Handler(), true, "GET")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/series", f.Handler(), true, "GET", "POST", "DELETE")
a.RegisterRoute(a.cfg.PrometheusHTTPPrefix+"/api/v1/metadata", f.Handler(), true, "GET")

// Register Legacy Routers
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/read", f.Handler(), true, "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/query", f.Handler(), true, "GET", "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/query_range", f.Handler(), true, "GET", "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/labels", f.Handler(), true, "GET", "POST")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/label/{name}/values", f.Handler(), true, "GET")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/series", f.Handler(), true, "GET", "POST", "DELETE")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/api/v1/metadata", f.Handler(), true, "GET")
a.RegisterQueryAPI(f.Handler())
}

// RegisterServiceMapHandler registers the Cortex structs service handler
Expand Down
2 changes: 1 addition & 1 deletion pkg/cortex/modules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (t *Cortex) initAPI() (services.Service, error) {
t.Cfg.API.ServerPrefix = t.Cfg.Server.PathPrefix
t.Cfg.API.LegacyHTTPPrefix = t.Cfg.HTTPPrefix

a, err := api.New(t.Cfg.API, t.Server, util.Logger)
a, err := api.New(t.Cfg.API, t.Server, util.Logger, prometheus.DefaultRegisterer)
if err != nil {
return nil, err
}
Expand Down