-
Notifications
You must be signed in to change notification settings - Fork 529
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
ingest storage: per-query X-Read-Consistency
HTTP header
#7091
Conversation
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>
This reverts commit 605a710.
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
There was a problem hiding this 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.
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pkg/api/api.go
Outdated
// 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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>
There was a problem hiding this 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.
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>
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
propagationquerierapi.ReadConsistencyFromContext
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
query-frontend --> query-scheduler
cortex_query_frontend_queries_consistency_total{consistency=""}
is only incremented for query & query_range requests, not for any other requestquery-scheduler/query-frontend --> querier
querier --> ingester