-
Notifications
You must be signed in to change notification settings - Fork 532
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
TraceQL: Attribute iterators collect matched array values #3867
Conversation
pkg/parquetquery/iters.go
Outdated
Key string | ||
Value pq.Value | ||
Key string | ||
Values []pq.Value |
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'm generally interested in benchmarks/perf impact. this is the first line that made me think to ask :)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 did some profiling for the TraceQL struct benchmark: the additional allocations are happening in BinaryOperation.execute()
but so far I have not figured out where exactly
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.
very strange benchmarks. i'm quite concerned about the hit on structural operators. i think we need to figure that out before merging. the increase in allocs is surprising. perhaps something in the DescendantOf function?
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.
metrics:
goarch: arm64
pkg: github.com/grafana/tempo/tempodb/encoding/vparquet4
│ metrics_main_20aug_1.txt │ metrics_branch_20aug_1.txt │
│ sec/op │ sec/op vs base │
BackendBlockQueryRange/{}_|_rate()/5-11 671.4m ± 7% 678.1m ± 8% ~ (p=0.190 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(name)/5-11 1.497 ± 9% 1.481 ± 8% ~ (p=0.393 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/5-11 1.010 ± 11% 1.026 ± 10% ~ (p=0.123 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/5-11 1.435 ± 1% 1.443 ± 2% ~ (p=0.143 n=10)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/5-11 100.3m ± 2% 101.0m ± 2% ~ (p=0.436 n=10)
BackendBlockQueryRange/{status=error}_|_rate()/5-11 117.8m ± 0% 117.2m ± 0% -0.53% (p=0.000 n=10)
geomean 508.1m 510.1m +0.39%
│ metrics_main_20aug_1.txt │ metrics_branch_20aug_1.txt │
│ MB_IO/op │ MB_IO/op vs base │
BackendBlockQueryRange/{}_|_rate()/5-11 18.67 ± 0% 18.67 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{}_|_rate()_by_(name)/5-11 24.20 ± 0% 24.20 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/5-11 18.99 ± 0% 18.99 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/5-11 21.35 ± 0% 21.35 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/5-11 18.99 ± 0% 18.99 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{status=error}_|_rate()/5-11 19.79 ± 0% 19.79 ± 0% ~ (p=1.000 n=10) ¹
geomean 20.24 20.24 +0.00%
¹ all samples are equal
│ metrics_main_20aug_1.txt │ metrics_branch_20aug_1.txt │
│ spans/op │ spans/op vs base │
BackendBlockQueryRange/{}_|_rate()/5-11 4.239M ± 0% 4.239M ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{}_|_rate()_by_(name)/5-11 4.239M ± 0% 4.239M ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/5-11 4.239M ± 0% 4.239M ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/5-11 4.239M ± 0% 4.239M ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/5-11 264.7k ± 0% 264.7k ± 0% ~ (p=1.000 n=10) ¹
BackendBlockQueryRange/{status=error}_|_rate()/5-11 92.51k ± 0% 92.51k ± 0% ~ (p=1.000 n=10) ¹
geomean 1.411M 1.411M +0.00%
¹ all samples are equal
│ metrics_main_20aug_1.txt │ metrics_branch_20aug_1.txt │
│ spans/s │ spans/s vs base │
BackendBlockQueryRange/{}_|_rate()/5-11 6.314M ± 8% 6.251M ± 8% ~ (p=0.190 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(name)/5-11 2.832M ± 10% 2.862M ± 9% ~ (p=0.393 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/5-11 4.198M ± 12% 4.131M ± 12% ~ (p=0.123 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/5-11 2.954M ± 1% 2.937M ± 2% ~ (p=0.143 n=10)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/5-11 2.638M ± 2% 2.619M ± 2% ~ (p=0.436 n=10)
BackendBlockQueryRange/{status=error}_|_rate()/5-11 785.5k ± 0% 789.6k ± 0% +0.53% (p=0.000 n=10)
geomean 2.778M 2.767M -0.38%
│ metrics_main_20aug_1.txt │ metrics_branch_20aug_1.txt │
│ B/op │ B/op vs base │
BackendBlockQueryRange/{}_|_rate()/5-11 8.133Mi ± 82% 11.184Mi ± 42% ~ (p=0.256 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(name)/5-11 15.54Mi ± 51% 17.39Mi ± 23% ~ (p=0.165 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/5-11 14.72Mi ± 61% 13.39Mi ± 41% ~ (p=0.631 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/5-11 203.2Mi ± 3% 202.0Mi ± 3% ~ (p=0.971 n=10)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/5-11 8.111Mi ± 24% 8.148Mi ± 19% ~ (p=0.971 n=10)
BackendBlockQueryRange/{status=error}_|_rate()/5-11 3.420Mi ± 22% 3.067Mi ± 13% ~ (p=0.353 n=10)
geomean 14.80Mi 15.36Mi +3.83%
│ metrics_main_20aug_1.txt │ metrics_branch_20aug_1.txt │
│ allocs/op │ allocs/op vs base │
BackendBlockQueryRange/{}_|_rate()/5-11 14.38k ± 0% 14.39k ± 0% ~ (p=0.170 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(name)/5-11 19.61k ± 0% 19.61k ± 0% ~ (p=0.578 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/5-11 15.20k ± 0% 15.20k ± 0% ~ (p=0.739 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/5-11 742.8k ± 1% 742.5k ± 2% ~ (p=0.796 n=10)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/5-11 33.74k ± 0% 33.74k ± 0% ~ (p=0.896 n=10)
BackendBlockQueryRange/{status=error}_|_rate()/5-11 21.08k ± 0% 21.08k ± 0% ~ (p=0.564 n=10)
geomean 36.24k 36.24k +0.01%
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
allocs are almost same but the queries with multiple &&
and ||
are taking around 10% hit:
{"traceOrMatch", "{ rootServiceName = `tempo-gateway` && (status = error || span.http.status_code = 500)}"},
{"traceOrNoMatch", "{ rootServiceName = `doesntexist` && (status = error || span.http.status_code = 500)}"},
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.
closing the loop on this, performance and allocations are now almost same as main:
goos: darwin
goarch: arm64
pkg: github.com/grafana/tempo/tempodb/encoding/vparquet4
│ traceql_main_22aug_2.txt │ traceql_branch_22aug_2.txt │
│ sec/op │ sec/op vs base │
BackendBlockTraceQL/spanAttValMatch 108.7m ± 0% 108.5m ± 0% ~ (p=0.247 n=10)
BackendBlockTraceQL/spanAttValNoMatch 1.171m ± 0% 1.171m ± 0% ~ (p=0.684 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch 1.207m ± 0% 1.206m ± 0% ~ (p=0.684 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch 1.207m ± 0% 1.205m ± 0% -0.12% (p=0.023 n=10)
BackendBlockTraceQL/resourceAttValMatch 565.6m ± 0% 566.9m ± 0% ~ (p=0.052 n=10)
BackendBlockTraceQL/resourceAttValNoMatch 1.108m ± 0% 1.107m ± 0% ~ (p=0.353 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch 38.57m ± 0% 39.10m ± 0% +1.38% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch#01 1.077m ± 0% 1.077m ± 0% ~ (p=0.739 n=10)
BackendBlockTraceQL/traceOrMatch 537.9m ± 5% 541.7m ± 2% ~ (p=0.280 n=10)
BackendBlockTraceQL/traceOrNoMatch 545.7m ± 1% 561.8m ± 1% +2.95% (p=0.002 n=10)
BackendBlockTraceQL/mixedValNoMatch 214.1m ± 0% 214.9m ± 0% +0.41% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd 1.326m ± 0% 1.343m ± 0% +1.30% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr 204.0m ± 1% 204.4m ± 1% ~ (p=0.052 n=10)
BackendBlockTraceQL/count 749.5m ± 1% 748.6m ± 0% ~ (p=0.853 n=10)
BackendBlockTraceQL/struct 859.4m ± 4% 848.6m ± 2% ~ (p=0.796 n=10)
BackendBlockTraceQL/|| 307.5m ± 0% 301.7m ± 1% -1.90% (p=0.000 n=10)
BackendBlockTraceQL/mixed 55.80m ± 0% 55.98m ± 0% +0.31% (p=0.000 n=10)
BackendBlockTraceQL/complex 1.682m ± 0% 1.644m ± 0% -2.26% (p=0.000 n=10)
geomean 32.37m 32.40m +0.08%
│ traceql_main_22aug_2.txt │ traceql_branch_22aug_2.txt │
│ B/s │ B/s vs base │
BackendBlockTraceQL/spanAttValMatch 291.9Mi ± 0% 292.3Mi ± 0% ~ (p=0.247 n=10)
BackendBlockTraceQL/spanAttValNoMatch 1.777Gi ± 0% 1.778Gi ± 0% ~ (p=0.670 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch 1.725Gi ± 0% 1.725Gi ± 0% ~ (p=0.684 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch 1.724Gi ± 0% 1.726Gi ± 0% +0.12% (p=0.023 n=10)
BackendBlockTraceQL/resourceAttValMatch 55.77Mi ± 0% 55.64Mi ± 0% -0.23% (p=0.041 n=10)
BackendBlockTraceQL/resourceAttValNoMatch 158.5Mi ± 0% 158.6Mi ± 0% ~ (p=0.343 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch 820.0Mi ± 0% 808.8Mi ± 0% -1.36% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch#01 230.7Mi ± 0% 230.7Mi ± 0% ~ (p=0.670 n=10)
BackendBlockTraceQL/traceOrMatch 2.813Mi ± 5% 2.789Mi ± 3% ~ (p=0.237 n=10)
BackendBlockTraceQL/traceOrNoMatch 2.770Mi ± 1% 2.689Mi ± 1% -2.93% (p=0.001 n=10)
BackendBlockTraceQL/mixedValNoMatch 16.40Mi ± 0% 16.34Mi ± 0% -0.41% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd 788.9Mi ± 0% 778.7Mi ± 0% -1.29% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr 158.3Mi ± 1% 157.9Mi ± 1% -0.20% (p=0.050 n=10)
BackendBlockTraceQL/count 42.04Mi ± 0% 42.09Mi ± 0% ~ (p=0.810 n=10)
BackendBlockTraceQL/struct 39.08Mi ± 4% 39.57Mi ± 2% ~ (p=0.810 n=10)
BackendBlockTraceQL/|| 103.2Mi ± 0% 105.2Mi ± 1% +1.94% (p=0.000 n=10)
BackendBlockTraceQL/mixed 561.3Mi ± 0% 559.6Mi ± 0% -0.31% (p=0.000 n=10)
BackendBlockTraceQL/complex 1.462Gi ± 0% 1.495Gi ± 0% +2.27% (p=0.000 n=10)
geomean 162.6Mi 162.4Mi -0.09%
│ traceql_main_22aug_2.txt │ traceql_branch_22aug_2.txt │
│ MB_io/op │ MB_io/op vs base │
BackendBlockTraceQL/spanAttValMatch 33.26 ± 0% 33.26 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttValNoMatch 2.234 ± 0% 2.234 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttIntrinsicMatch 2.234 ± 0% 2.234 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttIntrinsicNoMatch 2.234 ± 0% 2.234 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttValMatch 33.08 ± 0% 33.08 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttValNoMatch 184.2m ± 0% 184.2m ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttIntrinsicMatch 33.16 ± 0% 33.16 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttIntrinsicMatch#01 260.5m ± 0% 260.5m ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/traceOrMatch 1.585 ± 0% 1.585 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/traceOrNoMatch 1.585 ± 0% 1.585 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedValNoMatch 3.682 ± 0% 3.682 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedValMixedMatchAnd 1.097 ± 0% 1.097 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedValMixedMatchOr 33.86 ± 0% 33.86 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/count 33.04 ± 0% 33.04 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/struct 35.21 ± 0% 35.21 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/|| 33.29 ± 0% 33.29 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixed 32.85 ± 0% 32.85 ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/complex 2.641 ± 0% 2.641 ± 0% ~ (p=0.755 n=10)
geomean 5.518 5.519 +0.00%
¹ all samples are equal
│ traceql_main_22aug_2.txt │ traceql_branch_22aug_2.txt │
│ B/op │ B/op vs base │
BackendBlockTraceQL/spanAttValMatch 28.55Mi ± 0% 28.43Mi ± 1% ~ (p=0.247 n=10)
BackendBlockTraceQL/spanAttValNoMatch 913.6Ki ± 0% 913.7Ki ± 0% +0.01% (p=0.000 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch 976.2Ki ± 0% 976.3Ki ± 0% +0.00% (p=0.033 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch 976.3Ki ± 0% 976.3Ki ± 0% ~ (p=0.224 n=10)
BackendBlockTraceQL/resourceAttValMatch 606.2Mi ± 0% 606.2Mi ± 0% ~ (p=0.353 n=10)
BackendBlockTraceQL/resourceAttValNoMatch 876.0Ki ± 0% 876.0Ki ± 0% ~ (p=1.000 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch 2.183Mi ± 0% 2.182Mi ± 0% ~ (p=0.739 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch#01 884.1Ki ± 0% 884.1Ki ± 0% ~ (p=0.582 n=10)
BackendBlockTraceQL/traceOrMatch 28.94Mi ± 0% 28.44Mi ± 7% ~ (p=0.543 n=10)
BackendBlockTraceQL/traceOrNoMatch 28.94Mi ± 1% 29.15Mi ± 0% +0.72% (p=0.000 n=10)
BackendBlockTraceQL/mixedValNoMatch 949.9Ki ± 0% 950.1Ki ± 0% +0.02% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd 879.5Ki ± 0% 879.6Ki ± 0% +0.01% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr 1.330Mi ± 0% 1.330Mi ± 1% ~ (p=0.561 n=10)
BackendBlockTraceQL/count 692.4Mi ± 0% 692.4Mi ± 0% ~ (p=0.481 n=10)
BackendBlockTraceQL/struct 240.4Mi ± 8% 263.7Mi ± 5% +9.67% (p=0.007 n=10)
BackendBlockTraceQL/|| 115.1Mi ± 0% 115.5Mi ± 0% ~ (p=0.315 n=10)
BackendBlockTraceQL/mixed 3.888Mi ± 0% 3.886Mi ± 0% -0.06% (p=0.015 n=10)
BackendBlockTraceQL/complex 1010.9Ki ± 0% 1010.9Ki ± 0% ~ (p=0.796 n=10)
geomean 6.962Mi 6.994Mi +0.45%
│ traceql_main_22aug_2.txt │ traceql_branch_22aug_2.txt │
│ allocs/op │ allocs/op vs base │
BackendBlockTraceQL/spanAttValMatch 324.6k ± 0% 324.6k ± 0% ~ (p=0.928 n=10)
BackendBlockTraceQL/spanAttValNoMatch 12.82k ± 0% 12.82k ± 0% +0.01% (p=0.000 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch 12.79k ± 0% 12.79k ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttIntrinsicNoMatch 12.79k ± 0% 12.79k ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttValMatch 3.458M ± 0% 3.458M ± 0% +0.00% (p=0.043 n=10)
BackendBlockTraceQL/resourceAttValNoMatch 12.81k ± 0% 12.81k ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttIntrinsicMatch 29.42k ± 0% 29.42k ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttIntrinsicMatch#01 12.81k ± 0% 12.81k ± 0% ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/traceOrMatch 279.1k ± 0% 279.1k ± 3% ~ (p=0.780 n=10)
BackendBlockTraceQL/traceOrNoMatch 279.1k ± 0% 279.1k ± 0% +0.00% (p=0.000 n=10)
BackendBlockTraceQL/mixedValNoMatch 13.49k ± 0% 13.49k ± 0% +0.01% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd 12.85k ± 0% 12.85k ± 0% +0.01% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr 16.02k ± 0% 16.02k ± 0% ~ (p=0.153 n=10)
BackendBlockTraceQL/count 4.224M ± 0% 4.224M ± 0% ~ (p=0.725 n=10)
BackendBlockTraceQL/struct 1.939M ± 5% 2.053M ± 3% +5.86% (p=0.011 n=10)
BackendBlockTraceQL/|| 672.8k ± 0% 672.8k ± 0% ~ (p=0.101 n=10)
BackendBlockTraceQL/mixed 46.04k ± 0% 46.04k ± 0% ~ (p=0.474 n=10)
BackendBlockTraceQL/complex 13.19k ± 0% 13.19k ± 0% ~ (p=1.000 n=10)
geomean 76.59k 76.84k +0.32%
¹ all samples are equal
418bfee
to
db49dee
Compare
b4adb74
to
1d8258d
Compare
1d59746
to
f08b5a9
Compare
62324a4
to
3ea2d31
Compare
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.
Nice work on the latest round of changes. Everything I had in mind is addressed, lgtm, approving.
What this PR does:
With vParquet4 enabled TraceQL can match elements in arrays: If an attribute value is an array, operators like
=
and=~
are applied to the individual elements of the array. If one of the elements matches the condition, the whole array is considered a matched value.So far the returned spanset will only contain an attribute with the matched element of the array and not the whole array:
However, the correct behavior is to return the whole attribute value / array an not only the matched element:
What is missing from this PR:
This PR focuses on the capability of the. With this change the attribute iterators still don't evaluate theSyncIterator
to return arraysIsArray
column for the attribute (this is intentional in order to keep scope of the PR small). As a result arrays containing single values are returned as scalar values instead of arrays.we collect arrays in AttributeCollector and return arrays, we walked back the change in SyncIterator due to the perf. impact across to board.
Which issue(s) this PR fixes:
Contributes to: https://github.com/grafana/tempo-squad/issues/435
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]