Skip to content

Commit 8094e16

Browse files
ie-phamie-pham
committed
handle compare query truncate
1 parent 5991060 commit 8094e16

File tree

3 files changed

+323
-14
lines changed

3 files changed

+323
-14
lines changed

‎modules/frontend/combiner/metrics_query_range.go

+97-12
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,12 @@ func NewQueryRange(req *tempopb.QueryRangeRequest, maxSeriesLimit int) (Combiner
5555
}
5656

5757
sortResponse(resp)
58-
if combiner.MaxSeriesReached() {
59-
// Truncating the final response because even if we bail as soon as len(resp.Series) >= maxSeries
60-
// it's possible that the last response pushed us over the max series limit.
61-
if len(resp.Series) > maxSeries && maxSeries > 0 {
62-
resp.Series = resp.Series[:maxSeries]
63-
}
58+
truncateResponse(resp, maxSeries, req)
59+
60+
// partial is when the max series is reached either in the querier or generators
61+
// since it might have been truncated - we need to add the warning the it may be inaccurate
62+
// max series reached is when the max series is actually reached at the query-frontend level
63+
if combiner.MaxSeriesReached() || combiner.Partial() {
6464
resp.Status = tempopb.PartialStatus_PARTIAL
6565
resp.Message = maxSeriesReachedErrorMsg
6666
}
@@ -75,19 +75,15 @@ func NewQueryRange(req *tempopb.QueryRangeRequest, maxSeriesLimit int) (Combiner
7575
}
7676

7777
sortResponse(resp)
78-
if len(resp.Series) > maxSeries && maxSeries > 0 {
79-
// Truncating the final response because even if we bail as soon as len(resp.Series) >= maxSeries
80-
// it's possible that the last response pushed us over the max series limit.
81-
resp.Series = resp.Series[:maxSeries]
82-
}
78+
truncateResponse(resp, maxSeries, req)
8379
attachExemplars(req, resp)
8480

8581
// compare with prev resp and only return diffs
8682
diff := diffResponse(prevResp, resp)
8783
// store resp for next diff
8884
prevResp = resp
8985

90-
if combiner.MaxSeriesReached() {
86+
if combiner.Partial() || combiner.MaxSeriesReached() {
9187
diff.Status = tempopb.PartialStatus_PARTIAL
9288
diff.Message = maxSeriesReachedErrorMsg
9389
}
@@ -235,3 +231,92 @@ func attachExemplars(req *tempopb.QueryRangeRequest, res *tempopb.QueryRangeResp
235231
}
236232
}
237233
}
234+
235+
func truncateResponse(resp *tempopb.QueryRangeResponse, maxSeries int, req *tempopb.QueryRangeRequest) {
236+
if maxSeries == 0 || len(resp.Series) <= maxSeries {
237+
return
238+
}
239+
rootexpr, err := traceql.Parse(req.Query)
240+
if err != nil {
241+
return
242+
}
243+
// if this query is a compare, we need make sure there is one total series per key
244+
// only include if both total series and corresponding count series exist
245+
const (
246+
baselinePrefix = "baseline"
247+
selectionPrefix = "selection"
248+
baselineTotalPrefix = "baseline_total"
249+
selectionTotalPrefix = "selection_total"
250+
metaTypePrefix = "__meta_type"
251+
)
252+
if _, ok := rootexpr.MetricsPipeline.(*traceql.MetricsCompare); ok {
253+
baselineCountMap := make(map[string][]int) // count is one single series for each key/value pair
254+
baselineTotalMap := make(map[string]int) // total is always a single series for each key
255+
selectionCountMap := make(map[string][]int)
256+
selectionTotalMap := make(map[string]int)
257+
results := make([]*tempopb.TimeSeries, maxSeries)
258+
resultsIdx := 0
259+
260+
for i, series := range resp.Series {
261+
for _, label := range series.Labels {
262+
if label.Key != metaTypePrefix {
263+
// record the corresponding index
264+
if strings.Contains(series.PromLabels, baselineTotalPrefix) {
265+
baselineTotalMap[label.Key] = i
266+
continue
267+
}
268+
// if it's not baselineTotal but has baseline it's baselineCount
269+
if strings.Contains(series.PromLabels, baselinePrefix) {
270+
baselineCountMap[label.Key] = append(baselineCountMap[label.Key], i)
271+
continue
272+
}
273+
if strings.Contains(series.PromLabels, selectionTotalPrefix) {
274+
selectionTotalMap[label.Key] = i
275+
continue
276+
}
277+
// if it's not selectionTotal but has selection it's selectionCount
278+
if strings.Contains(series.PromLabels, selectionPrefix) {
279+
selectionCountMap[label.Key] = append(selectionCountMap[label.Key], i)
280+
continue
281+
}
282+
}
283+
}
284+
}
285+
286+
// do baseline first,
287+
// the total is more important so just check total first
288+
for a, i := range baselineTotalMap {
289+
// check if we have a count for this total
290+
if _, ok := baselineCountMap[a]; ok && resultsIdx < maxSeries {
291+
results[resultsIdx] = resp.Series[i]
292+
resultsIdx++
293+
for _, series := range baselineCountMap[a] {
294+
if resultsIdx >= maxSeries {
295+
break
296+
}
297+
results[resultsIdx] = resp.Series[series]
298+
resultsIdx++
299+
}
300+
}
301+
}
302+
// then do selection
303+
for a, i := range selectionTotalMap {
304+
// check if we have a count for this total
305+
if _, ok := selectionCountMap[a]; ok && resultsIdx < maxSeries {
306+
results[resultsIdx] = resp.Series[i]
307+
resultsIdx++
308+
for _, series := range selectionCountMap[a] {
309+
if resultsIdx >= maxSeries {
310+
break
311+
}
312+
results[resultsIdx] = resp.Series[series]
313+
resultsIdx++
314+
}
315+
}
316+
}
317+
resp.Series = results[:resultsIdx]
318+
return
319+
}
320+
// otherwise just truncate
321+
resp.Series = resp.Series[:maxSeries]
322+
}

‎modules/frontend/combiner/metrics_query_range_test.go

+214
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,198 @@ func TestQueryRangemaxSeriesShouldQuit(t *testing.T) {
619619
require.True(t, queryRangeCombiner.ShouldQuit())
620620
}
621621

622+
func TestTruncateResponse(t *testing.T) {
623+
type testCase struct {
624+
name string
625+
query string
626+
series []*tempopb.TimeSeries
627+
maxSeries int
628+
expectedSeries []*tempopb.TimeSeries
629+
}
630+
631+
defaultSeries := func(series int) []*tempopb.TimeSeries {
632+
randomValues := []string{"bar", "baz", "bat", "cat", "dog", "cow", "pig", "hen", "boo", "moo"}
633+
seriesList := make([]*tempopb.TimeSeries, series)
634+
for i := 0; i < series; i++ {
635+
seriesList[i] = &tempopb.TimeSeries{
636+
PromLabels: "foo=" + randomValues[i],
637+
Labels: []v1.KeyValue{
638+
{Key: "foo", Value: &v1.AnyValue{Value: &v1.AnyValue_StringValue{StringValue: randomValues[i]}}},
639+
},
640+
}
641+
}
642+
return seriesList
643+
}
644+
645+
tcs := []testCase{
646+
{
647+
name: "Less than max series",
648+
query: "{} | rate()",
649+
series: defaultSeries(3),
650+
maxSeries: 4,
651+
expectedSeries: defaultSeries(3),
652+
},
653+
{
654+
name: "More than max series",
655+
query: "{} | rate()",
656+
series: defaultSeries(10),
657+
maxSeries: 4,
658+
expectedSeries: defaultSeries(4),
659+
},
660+
{
661+
name: "No limit",
662+
query: "{} | rate()",
663+
series: defaultSeries(10),
664+
maxSeries: 0,
665+
expectedSeries: defaultSeries(10),
666+
},
667+
{
668+
name: "Compare less than max series",
669+
query: "{} | compare({status=error})",
670+
series: []*tempopb.TimeSeries{
671+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
672+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
673+
makeCompareSeries("selection", "resource.service.name", "even", false),
674+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
675+
},
676+
maxSeries: 4,
677+
expectedSeries: []*tempopb.TimeSeries{
678+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
679+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
680+
makeCompareSeries("selection", "resource.service.name", "even", false),
681+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
682+
},
683+
},
684+
{
685+
name: "Compare more than max series with extra count series same key",
686+
query: "{} | compare({status=error})",
687+
series: []*tempopb.TimeSeries{
688+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
689+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
690+
makeCompareSeries("selection", "resource.service.name", "even", false),
691+
makeCompareSeries("selection", "resource.service.name", "odd", false), // will be removed after truncate
692+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
693+
},
694+
maxSeries: 4,
695+
expectedSeries: []*tempopb.TimeSeries{
696+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
697+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
698+
makeCompareSeries("selection", "resource.service.name", "even", false),
699+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
700+
},
701+
},
702+
{
703+
name: "Compare more than max series with extra total baseline series diff key",
704+
query: "{} | compare({status=error})",
705+
series: []*tempopb.TimeSeries{
706+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
707+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
708+
makeCompareSeries("baseline_total", "namespace", "<nil>", true), // will be removed bc no matching count series
709+
makeCompareSeries("selection", "resource.service.name", "even", false),
710+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
711+
},
712+
maxSeries: 4,
713+
expectedSeries: []*tempopb.TimeSeries{
714+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
715+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
716+
makeCompareSeries("selection", "resource.service.name", "even", false),
717+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
718+
},
719+
},
720+
{
721+
name: "Compare more than max series with extra total selection series diff key",
722+
query: "{} | compare({status=error})",
723+
series: []*tempopb.TimeSeries{
724+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
725+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
726+
makeCompareSeries("selection_total", "namespace", "<nil>", true), // will be removed bc no matching count series
727+
makeCompareSeries("selection", "resource.service.name", "even", false),
728+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
729+
},
730+
maxSeries: 4,
731+
expectedSeries: []*tempopb.TimeSeries{
732+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
733+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
734+
makeCompareSeries("selection", "resource.service.name", "even", false),
735+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
736+
},
737+
},
738+
{
739+
name: "Compare more than max series with extra count selection series diff key",
740+
query: "{} | compare({status=error})",
741+
series: []*tempopb.TimeSeries{
742+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
743+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
744+
makeCompareSeries("selection", "resource.service.name", "even", false),
745+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
746+
makeCompareSeries("selection", "namespace", "even", false), // will be removed bc no matching total series
747+
},
748+
maxSeries: 4,
749+
expectedSeries: []*tempopb.TimeSeries{
750+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
751+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
752+
makeCompareSeries("selection", "resource.service.name", "even", false),
753+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
754+
},
755+
},
756+
{
757+
name: "Compare more than max series with extra count baseline series diff key",
758+
query: "{} | compare({status=error})",
759+
series: []*tempopb.TimeSeries{
760+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
761+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
762+
makeCompareSeries("selection", "resource.service.name", "even", false),
763+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
764+
makeCompareSeries("baseline", "namespace", "even", false), // will be removed bc no matching total series
765+
},
766+
maxSeries: 4,
767+
expectedSeries: []*tempopb.TimeSeries{
768+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
769+
makeCompareSeries("baseline_total", "resource.service.name", "<nil>", true),
770+
makeCompareSeries("selection", "resource.service.name", "even", false),
771+
makeCompareSeries("selection_total", "resource.service.name", "<nil>", true),
772+
},
773+
},
774+
{
775+
name: "Compare more than max series with zero matching series",
776+
query: "{} | compare({status=error})",
777+
series: []*tempopb.TimeSeries{
778+
makeCompareSeries("baseline", "resource.service.name", "odd", false),
779+
makeCompareSeries("baseline_total", "namespace", "<nil>", true),
780+
makeCompareSeries("selection", "resource.service.name", "even", false),
781+
makeCompareSeries("selection_total", "foo", "<nil>", true),
782+
makeCompareSeries("baseline", "evenodd", "even", false),
783+
},
784+
maxSeries: 4,
785+
expectedSeries: []*tempopb.TimeSeries{},
786+
},
787+
}
788+
789+
for _, tc := range tcs {
790+
t.Run(tc.name, func(t *testing.T) {
791+
req := &tempopb.QueryRangeRequest{
792+
Query: tc.query,
793+
Start: 0,
794+
End: 1000,
795+
Step: 1,
796+
MaxSeries: uint32(tc.maxSeries),
797+
}
798+
799+
resp := &tempopb.QueryRangeResponse{
800+
Series: tc.series,
801+
}
802+
expectedResp := &tempopb.QueryRangeResponse{
803+
Series: tc.expectedSeries,
804+
}
805+
truncateResponse(resp, tc.maxSeries, req)
806+
807+
sortResponse(resp)
808+
sortResponse(expectedResp)
809+
require.Equal(t, tc.expectedSeries, resp.Series)
810+
})
811+
}
812+
}
813+
622814
func BenchmarkDiffSeriesAndMarshal(b *testing.B) {
623815
prev, curr := seriesWithTenPercentDiff()
624816

@@ -703,3 +895,25 @@ func ts(samples []tempopb.Sample, exemplars []tempopb.Exemplar, kvs ...string) *
703895

704896
return ts
705897
}
898+
899+
func makeCompareSeries(meta, key, value string, total bool) *tempopb.TimeSeries {
900+
Labels := []v1.KeyValue{
901+
tempopb.MakeKeyValueString("__meta_type", meta),
902+
tempopb.MakeKeyValueString(key, value),
903+
}
904+
905+
PromLabels := `{__meta_type="` + meta + `", "` + key + `"="` + value + `"}`
906+
907+
if total {
908+
Labels = []v1.KeyValue{
909+
tempopb.MakeKeyValueString("__meta_type", "meta"),
910+
tempopb.MakeKeyValueString(key, "nil"),
911+
}
912+
PromLabels = `{__meta_type="` + meta + `", "` + key + `"="<nil>"}`
913+
}
914+
915+
return &tempopb.TimeSeries{
916+
PromLabels: PromLabels,
917+
Labels: Labels,
918+
}
919+
}

0 commit comments

Comments
 (0)