Skip to content
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

Refactoring: implement arrays for traceql.Static with reused fields #3827

Merged
merged 23 commits into from
Jul 16, 2024

Conversation

stoewer
Copy link
Collaborator

@stoewer stoewer commented Jul 2, 2024

What this PR does:
This PR adds support for the new types TypeIntArray, TypeFloatArray, TypeStringArray, and TypeBooleanArray to traceql.Static. This is a perquisite for supporting arrays in TraceQL.

The goal of this approach is to add support for array types to traceql.Static without increasing the size of the existing struct. It makes use of packed/reused struct fields that store values of multiple different types. This is accomplished through heavy use of casts and unsafe.

The new traceql.Static type is no longer comparable and can no longer be used as key in a map. This PR therefore introduces a new type traceql.StaticMapKey.

For context: a previous approach based on a traceql.Static interface with multiple implementations was explored in this PR.

Which issue(s) this PR fixes:
Is prerequisite for https://github.com/grafana/tempo-squad/issues/435

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@stoewer stoewer changed the title Refactor traceql static2 Refactoring: implement traceql.Static with reused fields Jul 2, 2024
@stoewer
Copy link
Collaborator Author

stoewer commented Jul 2, 2024

I ran a couple of more benchmarks and the results look pretty good. Here are the latest benchmarks:

Metrics:

goos: linux
goarch: amd64
pkg: github.com/grafana/tempo/tempodb/encoding/vparquet4
cpu: AMD Ryzen 7 PRO 6850U with Radeon Graphics     
                                                                             │ bench-metrics-05-static.txt │ bench-metrics-05-static-reuse-fields.txt │
                                                                             │           sec/op            │      sec/op        vs base               │
BackendBlockQueryRange/{}_|_rate()/7-16                                                         2.496 ± 1%         2.465 ±  7%  -1.22% (p=0.033 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(name)/7-16                                               4.705 ± 8%         4.639 ± 12%       ~ (p=0.443 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/7-16                              3.510 ± 5%         3.646 ±  7%  +3.89% (p=0.028 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/7-16                                      4.377 ± 5%         4.512 ±  5%       ~ (p=0.198 n=12)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/7-16                   236.2m ± 3%        236.1m ±  3%       ~ (p=0.755 n=12)
BackendBlockQueryRange/{status=error}_|_rate()/7-16                                            285.2m ± 1%        287.0m ±  1%  +0.63% (p=0.000 n=12)
geomean                                                                                         1.516              1.528        +0.81%

                                                                             │ bench-metrics-05-static.txt │ bench-metrics-05-static-reuse-fields.txt │
                                                                             │          MB_IO/op           │    MB_IO/op      vs base                 │
BackendBlockQueryRange/{}_|_rate()/7-16                                                         34.39 ± 0%        34.39 ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{}_|_rate()_by_(name)/7-16                                               44.62 ± 0%        44.62 ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/7-16                              34.87 ± 0%        34.87 ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/7-16                                      38.73 ± 0%        38.73 ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/7-16                    34.87 ± 0%        34.87 ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{status=error}_|_rate()/7-16                                             36.07 ± 0%        36.07 ± 0%       ~ (p=1.000 n=12) ¹
geomean                                                                                         37.10             37.10       +0.00%
¹ all samples are equal

                                                                             │ bench-metrics-05-static.txt │ bench-metrics-05-static-reuse-fields.txt │
                                                                             │          spans/op           │    spans/op      vs base                 │
BackendBlockQueryRange/{}_|_rate()/7-16                                                        8.537M ± 0%       8.537M ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{}_|_rate()_by_(name)/7-16                                              8.537M ± 0%       8.537M ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/7-16                             8.537M ± 0%       8.537M ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/7-16                                     8.537M ± 0%       8.537M ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/7-16                   382.0k ± 0%       382.0k ± 0%       ~ (p=1.000 n=12) ¹
BackendBlockQueryRange/{status=error}_|_rate()/7-16                                            94.10k ± 0%       94.10k ± 0%       ~ (p=1.000 n=12) ¹
geomean                                                                                        2.400M            2.400M       +0.00%
¹ all samples are equal

                                                                             │ bench-metrics-05-static.txt │ bench-metrics-05-static-reuse-fields.txt │
                                                                             │           spans/s           │      spans/s       vs base               │
BackendBlockQueryRange/{}_|_rate()/7-16                                                        3.421M ± 1%        3.463M ±  8%  +1.23% (p=0.033 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(name)/7-16                                              1.815M ± 9%        1.841M ± 13%       ~ (p=0.443 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/7-16                             2.432M ± 6%        2.341M ±  7%  -3.74% (p=0.028 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/7-16                                     1.951M ± 5%        1.892M ±  5%       ~ (p=0.198 n=12)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/7-16                   1.617M ± 3%        1.618M ±  3%       ~ (p=0.755 n=12)
BackendBlockQueryRange/{status=error}_|_rate()/7-16                                            330.0k ± 1%        327.9k ±  1%  -0.63% (p=0.000 n=12)
geomean                                                                                        1.583M             1.570M        -0.80%

                                                                             │ bench-metrics-05-static.txt │ bench-metrics-05-static-reuse-fields.txt │
                                                                             │            B/op             │       B/op         vs base               │
BackendBlockQueryRange/{}_|_rate()/7-16                                                      39.78Mi ± 37%       44.24Mi ± 17%       ~ (p=0.443 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(name)/7-16                                            52.27Mi ± 32%       52.01Mi ± 16%       ~ (p=0.590 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/7-16                           50.48Mi ± 32%       50.37Mi ±  0%       ~ (p=0.977 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/7-16                                   395.0Mi ±  5%       392.8Mi ±  3%       ~ (p=0.630 n=12)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/7-16                 13.38Mi ± 10%       13.95Mi ± 10%       ~ (p=0.799 n=12)
BackendBlockQueryRange/{status=error}_|_rate()/7-16                                          5.613Mi ±  8%       5.580Mi ± 11%       ~ (p=0.887 n=12)
geomean                                                                                      38.21Mi             39.05Mi        +2.18%

                                                                             │ bench-metrics-05-static.txt │ bench-metrics-05-static-reuse-fields.txt │
                                                                             │          allocs/op          │     allocs/op      vs base               │
BackendBlockQueryRange/{}_|_rate()/7-16                                                        522.6k ± 0%         522.6k ± 0%       ~ (p=0.681 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(name)/7-16                                              535.8k ± 0%         535.8k ± 0%       ~ (p=0.401 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)/7-16                             523.7k ± 0%         523.7k ± 0%       ~ (p=0.854 n=12)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)/7-16                                     1.856M ± 8%         1.826M ± 5%       ~ (p=0.410 n=12)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()/7-16                   117.8k ± 0%         117.8k ± 0%       ~ (p=0.945 n=12)
BackendBlockQueryRange/{status=error}_|_rate()/7-16                                            76.53k ± 0%         76.53k ± 0%       ~ (p=1.000 n=12)
geomean                                                                                        367.3k              366.3k       -0.27%
  

TraceQL

goos: linux
goarch: amd64
pkg: github.com/grafana/tempo/tempodb/encoding/vparquet4
cpu: AMD Ryzen 7 PRO 6850U with Radeon Graphics     
                                                   │ bench-traceql-07-static.txt │ bench-traceql-07-static-reuse-fields.txt │
                                                   │           sec/op            │      sec/op        vs base               │
BackendBlockTraceQL/spanAttValMatch-16                               553.7m ± 1%         553.4m ± 0%       ~ (p=0.315 n=10)
BackendBlockTraceQL/spanAttValNoMatch-16                             76.10m ± 1%         75.27m ± 1%  -1.10% (p=0.000 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch-16                         166.8m ± 1%         167.1m ± 1%       ~ (p=0.739 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-16                       4.183m ± 0%         4.096m ± 2%  -2.09% (p=0.007 n=10)
BackendBlockTraceQL/resourceAttValMatch-16                            1.848 ± 1%          1.765 ± 1%  -4.50% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttValNoMatch-16                         3.813m ± 1%         3.768m ± 0%  -1.19% (p=0.035 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-16                     341.0m ± 1%         339.8m ± 1%       ~ (p=0.123 n=10)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-16                   3.829m ± 0%         3.865m ± 0%  +0.95% (p=0.000 n=10)
BackendBlockTraceQL/mixedValNoMatch-16                               651.6m ± 1%         662.9m ± 1%  +1.74% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16                         52.71m ± 1%         53.12m ± 0%  +0.79% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16                          544.3m ± 1%         550.7m ± 2%       ~ (p=0.052 n=10)
BackendBlockTraceQL/count-16                                          1.449 ± 1%          1.502 ± 1%  +3.68% (p=0.000 n=10)
BackendBlockTraceQL/struct-16                                         2.399 ± 5%          2.316 ± 4%  -3.44% (p=0.000 n=10)
BackendBlockTraceQL/||-16                                            563.7m ± 1%         574.7m ± 1%  +1.95% (p=0.000 n=10)
BackendBlockTraceQL/mixed-16                                         211.0m ± 0%         211.6m ± 0%       ~ (p=0.075 n=10)
BackendBlockTraceQL/complex-16                                       4.375m ± 0%         4.399m ± 0%  +0.54% (p=0.001 n=10)
geomean                                                              132.6m              132.5m       -0.11%

                                                   │ bench-traceql-07-static.txt │ bench-traceql-07-static-reuse-fields.txt │
                                                   │             B/s             │        B/s         vs base               │
BackendBlockTraceQL/spanAttValMatch-16                              107.4Mi ± 1%        107.5Mi ± 0%       ~ (p=0.315 n=10)
BackendBlockTraceQL/spanAttValNoMatch-16                            233.0Mi ± 1%        235.5Mi ± 1%  +1.11% (p=0.000 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch-16                        372.8Mi ± 1%        372.3Mi ± 1%       ~ (p=0.739 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-16                      651.5Mi ± 0%        665.4Mi ± 2%  +2.13% (p=0.007 n=10)
BackendBlockTraceQL/resourceAttValMatch-16                          31.55Mi ± 1%        33.04Mi ± 1%  +4.72% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttValNoMatch-16                        196.4Mi ± 1%        198.8Mi ± 0%  +1.21% (p=0.034 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-16                    171.4Mi ± 1%        172.0Mi ± 1%       ~ (p=0.109 n=10)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-16                  231.7Mi ± 0%        229.5Mi ± 0%  -0.94% (p=0.000 n=10)
BackendBlockTraceQL/mixedValNoMatch-16                              32.07Mi ± 1%        31.53Mi ± 1%  -1.69% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16                        485.5Mi ± 1%        481.7Mi ± 0%  -0.78% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16                         49.22Mi ± 1%        48.65Mi ± 2%       ~ (p=0.052 n=10)
BackendBlockTraceQL/count-16                                        40.24Mi ± 0%        38.81Mi ± 1%  -3.54% (p=0.000 n=10)
BackendBlockTraceQL/struct-16                                       5.679Mi ± 5%        5.879Mi ± 4%  +3.53% (p=0.000 n=10)
BackendBlockTraceQL/||-16                                           104.0Mi ± 1%        102.0Mi ± 1%  -1.92% (p=0.000 n=10)
BackendBlockTraceQL/mixed-16                                        274.5Mi ± 0%        273.8Mi ± 0%       ~ (p=0.075 n=10)
BackendBlockTraceQL/complex-16                                      729.4Mi ± 0%        725.6Mi ± 0%  -0.52% (p=0.004 n=10)
geomean                                                             128.4Mi             128.5Mi       +0.11%

                                                   │ bench-traceql-07-static.txt │ bench-traceql-07-static-reuse-fields.txt │
                                                   │          MB_io/op           │    MB_io/op      vs base                 │
BackendBlockTraceQL/spanAttValMatch-16                                62.38 ± 0%        62.38 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttValNoMatch-16                              18.59 ± 0%        18.59 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttIntrinsicMatch-16                          65.22 ± 0%        65.22 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttIntrinsicNoMatch-16                        2.858 ± 0%        2.858 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttValMatch-16                            61.14 ± 0%        61.14 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttValNoMatch-16                         785.4m ± 0%       785.4m ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttIntrinsicMatch-16                      61.29 ± 0%        61.29 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-16                   930.2m ± 0%       930.2m ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedValNoMatch-16                                21.91 ± 0%        21.91 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedValMixedMatchAnd-16                          26.83 ± 0%        26.83 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedValMixedMatchOr-16                           28.09 ± 0%        28.09 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/count-16                                          61.12 ± 0%        61.12 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/struct-16                                         14.29 ± 0%        14.29 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/||-16                                             61.46 ± 0%        61.46 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixed-16                                          60.73 ± 0%        60.73 ± 0%       ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/complex-16                                        3.353 ± 1%        3.343 ± 0%       ~ (p=0.362 n=10)
geomean                                                               17.86             17.85       -0.02%
¹ all samples are equal

                                                   │ bench-traceql-07-static.txt │ bench-traceql-07-static-reuse-fields.txt │
                                                   │            B/op             │       B/op         vs base               │
BackendBlockTraceQL/spanAttValMatch-16                             250.3Mi ±  1%       237.4Mi ±  1%  -5.14% (p=0.000 n=10)
BackendBlockTraceQL/spanAttValNoMatch-16                           29.35Mi ±  1%       29.51Mi ±  1%       ~ (p=0.529 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch-16                       29.40Mi ±  2%       29.25Mi ±  4%       ~ (p=0.684 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-16                     2.726Mi ±  0%       2.729Mi ±  0%       ~ (p=0.529 n=10)
BackendBlockTraceQL/resourceAttValMatch-16                         1.546Gi ±  0%       1.536Gi ±  0%  -0.64% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttValNoMatch-16                       2.672Mi ±  0%       2.679Mi ±  0%  +0.29% (p=0.035 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-16                   145.2Mi ±  1%       138.6Mi ±  2%  -4.55% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-16                 2.677Mi ±  0%       2.672Mi ±  0%       ~ (p=0.315 n=10)
BackendBlockTraceQL/mixedValNoMatch-16                             31.02Mi ±  5%       32.19Mi ±  7%       ~ (p=0.063 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16                       3.484Mi ±  4%       3.619Mi ±  8%       ~ (p=0.280 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16                        4.058Mi ± 17%       4.320Mi ± 19%       ~ (p=0.796 n=10)
BackendBlockTraceQL/count-16                                       1.130Gi ±  0%       1.118Gi ±  0%  -1.07% (p=0.000 n=10)
BackendBlockTraceQL/struct-16                                      540.0Mi ±  1%       539.2Mi ±  1%       ~ (p=1.000 n=10)
BackendBlockTraceQL/||-16                                          19.63Mi ±  6%       18.22Mi ±  3%  -7.15% (p=0.001 n=10)
BackendBlockTraceQL/mixed-16                                       22.94Mi ±  4%       21.92Mi ±  4%  -4.45% (p=0.003 n=10)
BackendBlockTraceQL/complex-16                                     2.780Mi ±  1%       2.790Mi ±  1%       ~ (p=0.280 n=10)
geomean                                                            29.52Mi             29.34Mi        -0.59%

                                                   │ bench-traceql-07-static.txt │ bench-traceql-07-static-reuse-fields.txt │
                                                   │          allocs/op          │     allocs/op      vs base               │
BackendBlockTraceQL/spanAttValMatch-16                               2.174M ± 0%         2.174M ± 0%       ~ (p=0.148 n=10)
BackendBlockTraceQL/spanAttValNoMatch-16                             42.03k ± 0%         42.03k ± 0%       ~ (p=0.142 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch-16                         207.8k ± 0%         207.8k ± 0%       ~ (p=0.724 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-16                       41.69k ± 0%         41.69k ± 0%       ~ (p=1.000 n=10)
BackendBlockTraceQL/resourceAttValMatch-16                           10.04M ± 0%         10.04M ± 0%       ~ (p=0.782 n=10)
BackendBlockTraceQL/resourceAttValNoMatch-16                         41.72k ± 0%         41.72k ± 0%       ~ (p=0.070 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-16                     1.360M ± 0%         1.360M ± 0%       ~ (p=0.269 n=10)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-16                   41.72k ± 0%         41.72k ± 0%       ~ (p=0.370 n=10)
BackendBlockTraceQL/mixedValNoMatch-16                               46.68k ± 0%         46.69k ± 0%       ~ (p=0.488 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16                         43.72k ± 0%         43.72k ± 0%       ~ (p=0.158 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16                          44.70k ± 0%         44.71k ± 0%       ~ (p=0.725 n=10)
BackendBlockTraceQL/count-16                                         5.982M ± 0%         5.982M ± 0%       ~ (p=0.255 n=10)
BackendBlockTraceQL/struct-16                                        4.393M ± 0%         4.393M ± 0%       ~ (p=1.000 n=10)
BackendBlockTraceQL/||-16                                            207.1k ± 0%         207.1k ± 0%       ~ (p=0.066 n=10)
BackendBlockTraceQL/mixed-16                                         214.2k ± 0%         214.2k ± 0%       ~ (p=0.381 n=10)
BackendBlockTraceQL/complex-16                                       42.11k ± 0%         42.11k ± 0%       ~ (p=0.233 n=10)
geomean                                                              234.6k              234.6k       -0.00%

@stoewer stoewer changed the title Refactoring: implement traceql.Static with reused fields Refactoring: implement arrays for traceql.Static with reused fields Jul 4, 2024
@stoewer stoewer marked this pull request as ready for review July 4, 2024 06:13
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, awesome work

pkg/traceql/ast.go Show resolved Hide resolved
pkg/traceql/engine.go Show resolved Hide resolved
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of qs, otherwise it LGTM

pkg/traceql/ast.go Outdated Show resolved Hide resolved
pkg/traceql/ast.go Show resolved Hide resolved
pkg/traceqlmetrics/metrics.go Outdated Show resolved Hide resolved
pkg/traceql/ast.go Outdated Show resolved Hide resolved
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a great direction to take this. two small comments, but overall much prefer this to the interface approach

@stoewer stoewer self-assigned this Jul 15, 2024
@stoewer stoewer merged commit 6a6a7d1 into grafana:main Jul 16, 2024
14 checks passed
@stoewer stoewer deleted the refactor-traceql-static2 branch July 16, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants