-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Filter instant queries and shard them. #3984
Filter instant queries and shard them. #3984
Conversation
var params logql.Params | ||
var path string | ||
switch r.(type) { | ||
case *LokiRequest: | ||
params = paramsFromRequest(r) | ||
path = r.(*LokiRequest).GetPath() | ||
case *LokiInstantRequest: | ||
params = paramsFromInstantRequest(r) | ||
path = r.(*LokiInstantRequest).GetPath() |
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 wish LokiRequest
and LokiInstantRequest
would simply implement Params
. It's almost the same.
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.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.
LGTM
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.
Making great progress through some very complex code. Left some comments/suggestions - I think this isn't far from being merged :)
@@ -75,6 +75,48 @@ func (r *LokiRequest) LogToSpan(sp opentracing.Span) { | |||
|
|||
func (*LokiRequest) GetCachingOptions() (res queryrange.CachingOptions) { return } | |||
|
|||
func (r *LokiInstantRequest) GetStep() int64 { |
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 for later: we can probably serialize a lot of these methods into an embedded struct for reuse
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.
What do you mean?
@@ -426,3 +448,45 @@ func NewMetricTripperware( | |||
return next | |||
}, c, nil | |||
} | |||
|
|||
// NewInstantMetricTripperware creates a new frontend tripperware responsible for handling metric queries | |||
func NewInstantMetricTripperware( |
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.
Can we not use the ResultsCacheMiddleware
like we do in the metric tripperware?
Alright. It's working! We've got a speed up of almost one magnitude! |
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.
one nit, but LGTM. Nice work!
What this PR does / why we need it:
This patch also enables query sharding for instant queries in addition to the range query sharding.
Checklist