-
Notifications
You must be signed in to change notification settings - Fork 512
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
Changes from 9 commits
9f308cc
ab6dbfd
c11c5dc
579fe77
605a710
47190ea
08ef650
6cd2bcc
71ee875
dd76194
d98612e
bad87f1
b4995a2
ca30b12
e067fcc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,4 +138,4 @@ message CachedHTTPResponse { | |
message CachedHTTPHeader { | ||
string name = 1; | ||
string value = 2; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,13 +7,15 @@ import ( | |
"errors" | ||
"time" | ||
|
||
"github.com/grafana/dskit/tenant" | ||
"github.com/prometheus/client_golang/prometheus" | ||
"github.com/prometheus/client_golang/prometheus/promauto" | ||
"github.com/prometheus/prometheus/model/labels" | ||
"github.com/prometheus/prometheus/promql" | ||
"github.com/prometheus/prometheus/promql/parser" | ||
"github.com/prometheus/prometheus/storage" | ||
|
||
"github.com/grafana/mimir/pkg/querier/api" | ||
"github.com/grafana/mimir/pkg/querier/stats" | ||
) | ||
|
||
|
@@ -22,6 +24,7 @@ type queryStatsMiddleware struct { | |
nonAlignedQueries prometheus.Counter | ||
regexpMatcherCount prometheus.Counter | ||
regexpMatcherOptimizedCount prometheus.Counter | ||
consistencyCounter *prometheus.CounterVec | ||
next Handler | ||
} | ||
|
||
|
@@ -38,13 +41,18 @@ func newQueryStatsMiddleware(reg prometheus.Registerer, engine *promql.Engine) M | |
Name: "cortex_query_frontend_regexp_matcher_optimized_count", | ||
Help: "Total number of optimized regexp matchers", | ||
}) | ||
consistencyCounter := promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ | ||
Name: "cortex_query_frontend_queries_consistency_total", | ||
Help: "Total number of queries that explicitly request a level of consistency.", | ||
}, []string{"user", "consistency"}) | ||
|
||
return MiddlewareFunc(func(next Handler) Handler { | ||
return &queryStatsMiddleware{ | ||
engine: engine, | ||
nonAlignedQueries: nonAlignedQueries, | ||
regexpMatcherCount: regexpMatcherCount, | ||
regexpMatcherOptimizedCount: regexpMatcherOptimizedCount, | ||
consistencyCounter: consistencyCounter, | ||
next: next, | ||
} | ||
}) | ||
|
@@ -55,26 +63,32 @@ func (s queryStatsMiddleware) Do(ctx context.Context, req Request) (Response, er | |
s.nonAlignedQueries.Inc() | ||
} | ||
|
||
if expr, err := parser.ParseExpr(req.GetQuery()); err == nil { | ||
for _, selectors := range parser.ExtractSelectors(expr) { | ||
for _, matcher := range selectors { | ||
if matcher.Type != labels.MatchRegexp && matcher.Type != labels.MatchNotRegexp { | ||
continue | ||
} | ||
|
||
s.regexpMatcherCount.Inc() | ||
if matcher.IsRegexOptimized() { | ||
s.regexpMatcherOptimizedCount.Inc() | ||
} | ||
} | ||
} | ||
} | ||
|
||
s.regexpCounters(req) | ||
s.consistencyCounters(ctx) | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Have you considered calling it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean something like |
||
expr, err := parser.ParseExpr(req.GetQuery()) | ||
if err != nil { | ||
return | ||
} | ||
for _, selectors := range parser.ExtractSelectors(expr) { | ||
for _, matcher := range selectors { | ||
if matcher.Type != labels.MatchRegexp && matcher.Type != labels.MatchNotRegexp { | ||
continue | ||
} | ||
|
||
s.regexpMatcherCount.Inc() | ||
if matcher.IsRegexOptimized() { | ||
s.regexpMatcherOptimizedCount.Inc() | ||
} | ||
} | ||
} | ||
} | ||
|
||
var queryStatsErrQueryable = &storage.MockQueryable{MockQuerier: &storage.MockQuerier{SelectMockFunction: func(bool, *storage.SelectHints, ...*labels.Matcher) storage.SeriesSet { | ||
return storage.ErrSeriesSet(errors.New("cannot use query stats queryable for running queries")) | ||
}}} | ||
|
@@ -107,6 +121,17 @@ func (s queryStatsMiddleware) populateQueryDetails(ctx context.Context, req Requ | |
} | ||
} | ||
|
||
func (s queryStatsMiddleware) consistencyCounters(ctx context.Context) { | ||
dimitarvdimitrov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
consistency, ok := api.ReadConsistencyFromContext(ctx) | ||
if !ok { | ||
return | ||
} | ||
tenants, _ := tenant.TenantIDs(ctx) | ||
for _, tenantID := range tenants { | ||
s.consistencyCounter.WithLabelValues(tenantID, consistency).Inc() | ||
} | ||
} | ||
|
||
type QueryDetails struct { | ||
QuerierStats *stats.Stats | ||
|
||
|
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.
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?