feat: simplify query related modules #3320

jtlisi · 2020-10-11T19:15:14Z

What this PR does:

Simplify the querier handler code by consolidating the querier specific latency metrics and the custom handler for the internal querier into a single function.
Separates the querier module into two modules: querier and queryable.
Decouples the instantiation of the queryrange tripperware from the query-frontend

The goal of this PR is to simplify the setup of the querier and make it clearer how requests are routed to the querier when running with or without the query-frontend enabled simultaneously and to decouple the initialization of the default queryable and the initialization of the request processing/handling code.

I did my best in this PR to comment/clarify how the routing for the querier is instantiated when running standalone or as a part of single-binary.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

@jtlisi I tried my best to follow this refactoring but I found it a bit hard. The new design looks good to me, but I find difficult to understand if we're introducing any regression. I left few questions.

pkg/cortex/modules.go

pracucci · 2020-10-15T15:30:49Z

pkg/api/handlers.go

Am I missing anything, or promRouter and legacyPromRouter don't have middlewares attached?

The purpose for these routers is that they are compatible with the upstream Prometheus API struct route registration function. But we end up needing two of them because we register the routes on two separate path prefixes. Since we consolidate them both under another router that we instantiate in this function, it's easier to register the middleware with that router instead of individually with both of these routers.

We could wrap these two routers individually with the middleware as we did previously. However, that seems a bit redundant.

pkg/cortex/cortex.go

pkg/cortex/modules.go

pracucci · 2020-10-15T15:50:50Z

pkg/cortex/modules.go

Suggested change

// If queries processed using the external HTTP Server, we need wrap the internalHandler with

// If queries are processed using the external HTTP Server, we need wrap the internalHandler with

Also, what's internalHandler?

In this case the internalHandler is the router that's returned by NewQuerierHandler. I say internalHandler in the sense that it is not the main router that is registered with the weaveworks common HTTP server to handle external requests. Any request forwarded to the internalHandler must either go through the external HTTP server router first or be passed to it from the frontend worker.

Maybe a clearer term would be internalQuerierRouter?

pracucci · 2020-10-15T15:55:13Z

pkg/api/api.go

Who register these now?

These routes are registered with the two routers when we call api.Register on them. We end up having to register two routers because the upstream prometheus route library and registration function requires us to register separately for each path prefix.

Previously we explicitly registered every route in the Prometheus API to query handler. However, now we only explicitly register the metadata and read routes because they aren't handled by the upstream API struct. For the rest we register the prefix and since the promHandler and legacyPromHandler are themselves routers, the requests should be forwarded correctly.

Also I should note that the only way to hit these routes is by a request being routed to the internalQuerierHandler which can only happen if it hit's the routes registered in RegisterQueryAPI, so no new routes should have been exposed and no existing routes should have been removed. This PR just simplified the router that already was only acting as a middleman to the upstream Prometheus routers we already registered with the API struct.

jtlisi · 2020-10-16T15:45:44Z

@pracucci I added a diagram that I hope makes things a bit clearer. Let me know if you find it helpful or not. Otherwise I can remove it since it adds a good amount of LOC.

pstibrany

Makes sense to me why we want this, although it looks bit scary. 🤞

pkg/api/middlewares.go

pstibrany · 2020-10-23T14:29:45Z

pkg/cortex/modules.go

-func (t *Cortex) initQueryFrontend() (serv services.Service, err error) {
+// initQueryFrontendTripperware instantiates the tripperware used by the query frontend
+// to optimize Prometheus query requests.
+func (t *Cortex) initQueryFrontendTripperware() (serv services.Service, err error) {


pstibrany · 2020-10-23T14:43:01Z

pkg/api/handlers.go

+	// A prefix is fine because external routes will be registered explicitly
+	router.PathPrefix(cfg.LegacyHTTPPrefix + "/api/v1/").Handler(legacyPromRouter)
+
+	// Since we have a new router and the request will not go trough the default server


Does this comment still apply?

I think it would make sense to reword it or remove it but keep the tracing instrumentation since it can be useful to have a span for this particular router

pstibrany · 2020-10-23T14:45:22Z

pkg/cortex/modules.go

+
+// initQuerier registers an internal HTTP router with a Prometheus API backed by the
+// Cortex Queryable. Then it does one of the following:
+// 1. Query-Frontend Enabled: If Cortex has an All or QueryFrontend target, the internal


To make sure comment is formatted properly, please put empty lines between paragraphs (or list items), and don't use extra space at the beginning of the lines (except for diagram below) -- such lines are formatted as code according to Go doc conventions.

https://blog.golang.org/godoc

I will update this to match the conventions

Thanks. It helps tools to format it properly. (eg. GoLand :))

pstibrany · 2020-10-23T14:51:28Z

pkg/cortex/modules.go

+		// If queries are processed using the external HTTP Server, we need wrap the internal querier with
+		// HTTP router with middleware to parse the tenant ID from the HTTP header and inject it into the
+		// request context.
+		internalQuerierRouter = middleware.AuthenticateUser.Wrap(internalQuerierRouter)


We didn't do this before, did we? Why is it needed now?

Actually, we did do this before because we previously used the registerRouteWithRouter function built into the API struct which applies this middleware. Now we just register the routes directly with with the router using the Path|PathPrefix function.

Signed-off-by: Jacob Lisi <[email protected]>

pracucci

LGTM, thanks!

pull-request-size bot added the size/L label Oct 11, 2020

jtlisi force-pushed the 20201011_decompose_query_modules branch from a0602fe to ba42f0d Compare October 12, 2020 14:16

jtlisi marked this pull request as ready for review October 12, 2020 19:08

jtlisi force-pushed the 20201011_decompose_query_modules branch from bdc90fd to 349e68d Compare October 12, 2020 19:09

pracucci reviewed Oct 15, 2020

View reviewed changes

jtlisi force-pushed the 20201011_decompose_query_modules branch from 1820ab9 to af7651a Compare October 15, 2020 19:35

pull-request-size bot added size/XL and removed size/L labels Oct 15, 2020

jtlisi force-pushed the 20201011_decompose_query_modules branch from ea6118f to 45851b2 Compare October 15, 2020 19:46

jtlisi requested a review from pracucci October 19, 2020 18:35

pstibrany approved these changes Oct 23, 2020

View reviewed changes

jtlisi added 5 commits October 25, 2020 15:04

feat: simplify query related modules

8d84b2a

Signed-off-by: Jacob Lisi <[email protected]>

refactor per PR comments

95945b6

Signed-off-by: Jacob Lisi <[email protected]>

fix rebase import

f72feb7

Signed-off-by: Jacob Lisi <[email protected]>

add route explanation diagram

81a929b

Signed-off-by: Jacob Lisi <[email protected]>

chore: address PR comments

a13a0c4

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi force-pushed the 20201011_decompose_query_modules branch from f9fc990 to a13a0c4 Compare October 25, 2020 19:04

fix: context package import issue after rebase

49ea64f

Signed-off-by: Jacob Lisi <[email protected]>

pracucci approved these changes Oct 26, 2020

View reviewed changes

pracucci merged commit 71a95f7 into cortexproject:master Oct 26, 2020

jtlisi deleted the 20201011_decompose_query_modules branch October 26, 2020 14:01

csmarchbanks mentioned this pull request Oct 30, 2020

cortex_querier_.* metrics no longer differentiates api/prom/v1 routes #3432

Closed

pstibrany mentioned this pull request Dec 1, 2020

Module initialization order seems to be wrong for querier #3550

Closed

	// If queries processed using the external HTTP Server, we need wrap the internalHandler with
	// If queries are processed using the external HTTP Server, we need wrap the internalHandler with

feat: simplify query related modules #3320

feat: simplify query related modules #3320

Uh oh!

Conversation

jtlisi commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi commented Oct 16, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtlisi commented Oct 11, 2020 •

edited

Loading

jtlisi Oct 15, 2020 •

edited

Loading