-
Notifications
You must be signed in to change notification settings - Fork 530
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
enforce max series for metrics queries #4525
base: main
Are you sure you want to change the base?
Changes from all commits
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 | ||||
---|---|---|---|---|---|---|
|
@@ -696,6 +696,9 @@ query_frontend: | |||||
# Maximun number of exemplars per range query. Limited to 100. | ||||||
[max_exemplars: <int> | default = 100 ] | ||||||
|
||||||
# Maximum number of time series returned for a metrics query. | ||||||
[max_response_series: <int> | default 1000] | ||||||
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.
Suggested change
to match other default values in the doc. |
||||||
|
||||||
# query_backend_after controls where the query-frontend searches for traces. | ||||||
# Time ranges older than query_backend_after will be searched in the backend/object storage only. | ||||||
# Time ranges between query_backend_after and now will be queried from the metrics-generators. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package combiner | ||
|
||
import ( | ||
"fmt" | ||
"math" | ||
"slices" | ||
"sort" | ||
|
@@ -14,7 +15,7 @@ import ( | |
var _ GRPCCombiner[*tempopb.QueryRangeResponse] = (*genericCombiner[*tempopb.QueryRangeResponse])(nil) | ||
|
||
// NewQueryRange returns a query range combiner. | ||
func NewQueryRange(req *tempopb.QueryRangeRequest, trackDiffs bool) (Combiner, error) { | ||
func NewQueryRange(req *tempopb.QueryRangeRequest, trackDiffs bool, setMaxSeries bool, maxSeries int) (Combiner, error) { | ||
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. we can pass maxSeries as 0 to disable it, and skip the setMaxSeries variable. no strong preference here tho, okay with this as well. |
||
combiner, err := traceql.QueryRangeCombinerFor(req, traceql.AggregateModeFinal, trackDiffs) | ||
if err != nil { | ||
return nil, err | ||
|
@@ -43,6 +44,11 @@ func NewQueryRange(req *tempopb.QueryRangeRequest, trackDiffs bool) (Combiner, e | |
if resp == nil { | ||
resp = &tempopb.QueryRangeResponse{} | ||
} | ||
if setMaxSeries && len(resp.Series) > maxSeries { | ||
resp.Series = resp.Series[:maxSeries] | ||
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. IIUC, we are still collecting all the series and the dropping the extra data before we return them, and not exiting early here? right? it would be great if exited early when we hit this limit. It would be very useful in the cases where is q metrics query is returning high cardinality results, for example: Just the work of pulling the series from the blocks will be resource intensive, and can OOM all generators. 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. i raised the question here #4525 (comment) 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. cool, I am okay with current impl, and we can leve a note or todo that this can improved by making the results deterministic and exiting early. sidenote: I think Joe's ordered results work might help here. |
||
resp.Status = tempopb.PartialStatus_PARTIAL | ||
resp.Message = fmt.Sprintf("Response exceeds maximum series of %d, a partial response is returned", maxSeries) | ||
} | ||
sortResponse(resp) | ||
attachExemplars(req, resp) | ||
return resp, nil | ||
|
@@ -62,8 +68,8 @@ func NewQueryRange(req *tempopb.QueryRangeRequest, trackDiffs bool) (Combiner, e | |
return c, nil | ||
} | ||
|
||
func NewTypedQueryRange(req *tempopb.QueryRangeRequest, trackDiffs bool) (GRPCCombiner[*tempopb.QueryRangeResponse], error) { | ||
c, err := NewQueryRange(req, trackDiffs) | ||
func NewTypedQueryRange(req *tempopb.QueryRangeRequest, trackDiffs bool, setMaxSeries bool, maxSeries int) (GRPCCombiner[*tempopb.QueryRangeResponse], error) { | ||
c, err := NewQueryRange(req, trackDiffs, setMaxSeries, maxSeries) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
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: the changelog entry makes me think that this is a behaviour change in the current endpoints, while we are adding new v2 endpoints. can we update the entry to make it clear.