Skip to content
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.

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

ingest storage: per-query X-Read-Consistency HTTP header #7091

Merged
merged 15 commits into from
Jan 11, 2024

Conversation

dimitarvdimitrov
Copy link
Contributor

This PR builds on top of #7030

What this PR does

This PR allows to specify the consistency level of individual queries. This is controlled via the HTTP header X-Read-Consistency: strong|eventual. Because this is still experimental, it's not documented in user-visible docs (i.e. /docs)

X-Read-Consistency propagation

  • Within a component the value is kept in the context and can be accessed via querierapi.ReadConsistencyFromContext
  • Between Mimir components the header is propagated via an HTTP header or gRPC metadata

Within components

Propagating within components is somewhat trivial. It relies on the assumption that the original request context is retained and no place uses context.Background() (or an unrelated context).

Between components

Propagating between components is less trivial. Primarily because of creating phony requests, httpgrpc, and custom gRPC clients

  • client --> query-frontend

    • the dskit HTTP server has a middleware which extracts the HTTP header and injects it into the HTTP request context
  • query-frontend --> query-scheduler

    • query & query_range requests: the query-frontend adds the header to all httpgrpc requests
    • all other requests: the original HTTP request contains the HTTP header and that is injected in the httpgrpc request.
    • the query-scheduler treats httpgrpc requests as opaque, so it doesn't need to understand the header
    • Because of how all other requests are handler, the added metric cortex_query_frontend_queries_consistency_total{consistency=""} is only incremented for query & query_range requests, not for any other request
  • query-scheduler/query-frontend --> querier

    • after receiving the httpgrpc request from the query-frontend the querier passes it to a prometheus HTTP router, same router uses the same middleware that the query-frontend uses. This middleware parses the HTTP header and injects it into the request context
  • querier --> ingester

    • the ingester gRPC client has a unary interceptor which takes the consistency level from the context and injects it as gRPC metadata
    • the dskit gRPC server has a middleware which parses the metadata and injects the consistency level in the request context

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner January 10, 2024 13:33
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@pracucci pracucci self-requested a review January 10, 2024 22:44
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I'm mid review, but I need to leave now sorry. Just committing a comment.

pkg/api/api.go Outdated Show resolved Hide resolved
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pkg/api/api.go Outdated
Comment on lines 165 to 167
// Assuming that we only need consistency controls when the request is for a specific tenant.
// Not all requests that require a tenant also support consistency, but it doesn't hurt if we propagate it anyway.
handler = querierapi.ConsistencyMiddleware().Wrap(handler)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think my comment is the opposite. Why not just always inject it? Also look at who else reads AuthMiddleware cause I think should inject it also when directly querying queriers for consistency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just always inject it?

I mean, why only when auth is used? We could just always enable it. Then the fact many APIs currently don't support it is a separate discussion, but for consistency we should always enable it.

Alternatively, we could move to the other side of the specturum and just enable it for querier's API, so only for routes registered by RegisterQueryAPI().

I don't feel strong about this comment, but it still looks weird to bundle it to auth, given has nothing to do with auth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review. Can you check out how dd76194 looks like?

s.populateQueryDetails(ctx, req)

return s.next.Do(ctx, req)
}

func (s queryStatsMiddleware) regexpCounters(req Request) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] Have you considered calling it track....() and consistencyCounters -> track...() too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean something like trackReadConsistency and trackRegexpMatchers? 👍 done in d98612e

pkg/frontend/querymiddleware/stats.go Outdated Show resolved Hide resolved
pkg/ingester/active_series.go Show resolved Hide resolved
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I just left a comment about a couple of commented lines of code. We don't have any e2e test (which I believe would be the only way to test everything is wiring correctly), but I don't think that's a blocker. We can move forward and we can add "adding e2e tests" to an issue for the future.

pkg/querier/api/consistency.go Outdated Show resolved Hide resolved
pkg/querier/api/consistency.go Outdated Show resolved Hide resolved
pkg/querier/worker/frontend_processor.go Outdated Show resolved Hide resolved
pkg/querier/worker/scheduler_processor.go Outdated Show resolved Hide resolved
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov merged commit a2b6d4e into main Jan 11, 2024
28 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/ingest/per-query-consistency-control branch January 11, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants