enforce max series for metrics queries #4525
Conversation
ie-pham
requested review from
knylander-grafana,
joe-elliott,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
stoewer and
javiermolinar
as code owners
|
the way this is implemented, tempo will truncate the final results at the frontend level. We can implement it in a way that will return as soon as 1000 series is reached regardless of how many data points are in each series to exit early. Not sure which we prefer. |
| PathSearchTagValuesV2 = "/api/v2/search/tag/{" + MuxVarTagName + "}/values" | ||
| PathSearchTagsV2 = "/api/v2/search/tags" | ||
| PathTracesV2 = "/api/v2/traces/{traceID}" | ||
| PathMetricsQueryInstantV2 = "/api/v2/metrics/query" |
There was a problem hiding this comment.
let's add these /v2/metrics endpoints to the API Reference page in docs.
There was a problem hiding this comment.
why are we adding a v2 metrics api?
| @@ -58,11 +58,7 @@ message TraceByIDRequest { | |||
| message TraceByIDResponse { | |||
| Trace trace = 1; | |||
| TraceByIDMetrics metrics = 2; | |||
| enum Status { | |||
There was a problem hiding this comment.
I am not sure if taking the enum out is backwards compatible or not, I think it should be okay. but can you verify that tracebyidv2 endpoint is working as expected with grafana.
I think e2e tests would catch this, but I am not sure if we are testing it at the proto level
| @@ -1,5 +1,7 @@ | |||
| ## main / unreleased | |||
|
|
|||
| * [CHANGE] Enforce max series in response for metrics queries [#4525](https://github.com/grafana/tempo/pull/4525) (@ie-pham) | |||
There was a problem hiding this comment.
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.
| @@ -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.
| [max_response_series: <int> | default 1000] | |
| [max_response_series: <int> | default = 1000] |
to match other default values in the doc.
| @@ -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.
we can pass maxSeries as 0 to disable it, and skip the setMaxSeries variable.
no strong preference here tho, okay with this as well.
| @@ -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.
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: {} | rate by (span:id)
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.
i raised the question here #4525 (comment)
the problem is the results come in indeterministically - so if we exit as soon as we hit the max series, we could have a response where each series just has one data point which isn't very uesful. but yes it would save us for out performing.
There was a problem hiding this comment.
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.
| PathSearchTagValuesV2 = "/api/v2/search/tag/{" + MuxVarTagName + "}/values" | ||
| PathSearchTagsV2 = "/api/v2/search/tags" | ||
| PathTracesV2 = "/api/v2/traces/{traceID}" | ||
| PathMetricsQueryInstantV2 = "/api/v2/metrics/query" |
There was a problem hiding this comment.
why are we adding a v2 metrics api?
| PARTIAL = 1; | ||
| } | ||
| Status status = 3; | ||
| PartialStatus status = 3; |
There was a problem hiding this comment.
we don't have to do a V2 - it's just extra precautions for when we roll out and Grafana is not yet using the new protos.
What this PR does: Add config to enforce max time series returned in a metrics query. This is enforced only on the query front-end side.
new config: max_response_series <default 1000>
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]