-
Notifications
You must be signed in to change notification settings - Fork 510
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
Query Frontend: Job weights #4076
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
modules/frontend/queue/queue.go
Outdated
} | ||
totalWeight += weight | ||
|
||
if totalWeight >= requestedCount { |
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 this makes sense. I suppose what we're saying here is that we request of this batch a certain high water mark of work that we're willing to take, and the weight increases the notion of complexity for a single item above this threshold. Implicitly here I suppose is that weight and requestedCount are of the same unit of measure.
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.
Implicitly here I suppose is that weight and requestedCount are of the same unit of measure.
yes! currently all jobs fill a single "slot" in the batch. the "weight" is basically just making it fill more slots.
modules/frontend/weights/weights.go
Outdated
} | ||
} | ||
|
||
if conditions > 4 { // yay, magic! |
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.
A fine starting point. I was wonder if each condition is weight++, and maybe each regex is weight+2 or some such. It means for the queue logic that if any condition is present, we'll never consume the entire requested batch. 🤔
if query.Has("query") { | ||
traceQLQuery = query.Get("query") | ||
} | ||
if traceQLQuery != "" { |
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 ease of reading i prefer.
if traceQLQuery == "" {
req.SetWeight(TraceQLSearchWeight)
return
}
...
this reduces nesting of the code below and very clearly communicates the logic taken when the query is not found
What this PR does:
The query frontend treats all jobs as the same size when it farms them out to the queriers. This can cause querier instability b/c some jobs actually require quite a bit more resources to execute. By assigning weights to jobs we can reduce the amount each querier is asked to do will hopefully:
Other changes
parseQuery
andcreateFetchSpansRequest
to consolidate on theCompile
function inpkg/traceql
TODO
PRTODO
Testing so far
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]