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

[TraceQL] add support for event attribute query #3748

Merged
merged 22 commits into from
Jun 28, 2024
Merged

Conversation

ie-pham
Copy link
Collaborator

@ie-pham ie-pham commented Jun 4, 2024

What this PR does: Add support for querying event attributes. In order to make this work, row numbers have been extended from having a length of 6 to a length of 8.

TLDR: 8 levels is faster than 7 levels
Final Benchmark

                                                    │ 6levels.txt │          8levels-fast.txt          │
                                                    │   sec/op    │   sec/op     vs base               │
BackendBlockTraceQL/spanAttValMatch-16                474.9m ± 2%   490.7m ± 2%  +3.32% (p=0.004 n=10)
BackendBlockTraceQL/spanAttValNoMatch-16              38.30m ± 2%   37.85m ± 1%  -1.18% (p=0.005 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch-16          250.6m ± 1%   254.6m ± 1%  +1.57% (p=0.000 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-16        5.882m ± 0%   5.872m ± 1%       ~ (p=0.971 n=10)
BackendBlockTraceQL/resourceAttValMatch-16             1.546 ± 1%    1.594 ± 1%  +3.07% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttValNoMatch-16          5.614m ± 3%   5.653m ± 1%       ~ (p=0.353 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-16      284.4m ± 6%   289.0m ± 2%       ~ (p=0.247 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch#01-16   5.647m ± 1%   5.668m ± 2%       ~ (p=0.481 n=10)
BackendBlockTraceQL/mixedValNoMatch-16                497.9m ± 1%   525.6m ± 2%  +5.56% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16          5.755m ± 2%   5.671m ± 2%  -1.46% (p=0.029 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16           415.8m ± 2%   437.6m ± 1%  +5.24% (p=0.000 n=10)
BackendBlockTraceQL/count-16                           1.330 ± 1%    1.322 ± 2%       ~ (p=0.089 n=10)
BackendBlockTraceQL/struct-16                          1.485 ± 3%    1.549 ± 3%  +4.30% (p=0.015 n=10)
BackendBlockTraceQL/||-16                             432.4m ± 1%   452.8m ± 2%  +4.72% (p=0.000 n=10)
BackendBlockTraceQL/mixed-16                          156.3m ± 1%   158.5m ± 4%  +1.45% (p=0.003 n=10)
BackendBlockTraceQL/complex-16                        6.285m ± 1%   6.297m ± 6%       ~ (p=0.393 n=10)
geomean                                               108.6m        110.5m       +1.77%

Screenshot 2024-06-27 at 4 47 39 PM

--- story time if you're interested in the investigation ---

When adding just one extra level (7 levels), the performance decreased dramatically and I originally thought it was just due to the memory allocation of the RowNumber going from length of 6 to length of 7.

                                             │ 6block.txt  │             7block.txt              │
                                             │   sec/op    │   sec/op     vs base                │
BackendBlockTraceQL/mixedValNoMatch-16         505.5m ± 1%   587.5m ± 2%  +16.22% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16   5.804m ± 1%   5.748m ± 2%        ~ (p=0.315 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16    419.5m ± 1%   494.2m ± 3%  +17.81% (p=0.000 n=10)
geomean                                        107.2m        118.6m       +10.68%

But then, curiosity got me thinking... What if we drop to 5 levels? Would that increase our performance? (it didn't)

                                             │ 6block.txt  │             5block.txt              │
                                             │   sec/op    │   sec/op     vs base                │
BackendBlockTraceQL/mixedValNoMatch-16         505.5m ± 1%   571.8m ± 1%  +13.12% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16   5.804m ± 1%   5.785m ± 1%        ~ (p=0.853 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16    419.5m ± 1%   484.9m ± 1%  +15.59% (p=0.000 n=10)
geomean                                        107.2m        117.1m        +9.23%

Then @stoewer suggested trying 8 levels...

This is a bit far fetched: It could be an alignment problem

                                             │ 6block.txt  │             8block.txt             │
                                             │   sec/op    │   sec/op     vs base               │
BackendBlockTraceQL/mixedValNoMatch-16         505.5m ± 1%   534.2m ± 2%  +5.69% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16   5.804m ± 1%   5.748m ± 2%       ~ (p=0.190 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16    419.5m ± 1%   435.8m ± 2%  +3.89% (p=0.001 n=10)
geomean                                        107.2m        110.2m       +2.83%

Welp.


I also modified the truncate function to make it slightly more efficient

                                             │ slowTruncate.txt │          fastTruncate.txt          │
                                             │      sec/op      │   sec/op     vs base               │
BackendBlockTraceQL/mixedValNoMatch-16              590.1m ± 1%   580.6m ± 2%  -1.61% (p=0.035 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16        5.772m ± 3%   5.737m ± 3%       ~ (p=0.190 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16         497.4m ± 4%   485.2m ± 2%  -2.46% (p=0.019 n=10)
geomean                                             119.2m        117.4m       -1.56%

And the full comparison (8levels-fast = new truncate function)

                                                    │ 6levels.txt │            8levels.txt             │          8levels-fast.txt          │
                                                    │   sec/op    │   sec/op     vs base               │   sec/op     vs base               │
BackendBlockTraceQL/spanAttValMatch-16                474.9m ± 2%   490.0m ± 2%  +3.18% (p=0.002 n=10)   490.7m ± 2%  +3.32% (p=0.004 n=10)
BackendBlockTraceQL/spanAttValNoMatch-16              38.30m ± 2%   37.99m ± 1%       ~ (p=0.075 n=10)   37.85m ± 1%  -1.18% (p=0.005 n=10)
BackendBlockTraceQL/spanAttIntrinsicMatch-16          250.6m ± 1%   257.7m ± 2%  +2.83% (p=0.000 n=10)   254.6m ± 1%  +1.57% (p=0.000 n=10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-16        5.882m ± 0%   5.888m ± 3%       ~ (p=0.684 n=10)   5.872m ± 1%       ~ (p=0.971 n=10)
BackendBlockTraceQL/resourceAttValMatch-16             1.546 ± 1%    1.603 ± 5%  +3.62% (p=0.000 n=10)    1.594 ± 1%  +3.07% (p=0.000 n=10)
BackendBlockTraceQL/resourceAttValNoMatch-16          5.614m ± 3%   5.649m ± 1%       ~ (p=1.000 n=10)   5.653m ± 1%       ~ (p=0.353 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-16      284.4m ± 6%   288.7m ± 9%       ~ (p=0.123 n=10)   289.0m ± 2%       ~ (p=0.247 n=10)
BackendBlockTraceQL/resourceAttIntrinsicMatch#01-16   5.647m ± 1%   5.622m ± 2%       ~ (p=0.684 n=10)   5.668m ± 2%       ~ (p=0.481 n=10)
BackendBlockTraceQL/mixedValNoMatch-16                497.9m ± 1%   535.7m ± 6%  +7.59% (p=0.000 n=10)   525.6m ± 2%  +5.56% (p=0.000 n=10)
BackendBlockTraceQL/mixedValMixedMatchAnd-16          5.755m ± 2%   5.676m ± 2%       ~ (p=0.089 n=10)   5.671m ± 2%  -1.46% (p=0.029 n=10)
BackendBlockTraceQL/mixedValMixedMatchOr-16           415.8m ± 2%   446.8m ± 1%  +7.44% (p=0.000 n=10)   437.6m ± 1%  +5.24% (p=0.000 n=10)
BackendBlockTraceQL/count-16                           1.330 ± 1%    1.325 ± 1%       ~ (p=0.247 n=10)    1.322 ± 2%       ~ (p=0.089 n=10)
BackendBlockTraceQL/struct-16                          1.485 ± 3%    1.571 ± 8%  +5.81% (p=0.009 n=10)    1.549 ± 3%  +4.30% (p=0.015 n=10)
BackendBlockTraceQL/||-16                             432.4m ± 1%   466.2m ± 3%  +7.82% (p=0.000 n=10)   452.8m ± 2%  +4.72% (p=0.000 n=10)
BackendBlockTraceQL/mixed-16                          156.3m ± 1%   157.8m ± 2%  +1.01% (p=0.002 n=10)   158.5m ± 4%  +1.45% (p=0.003 n=10)
BackendBlockTraceQL/complex-16                        6.285m ± 1%   6.304m ± 2%       ~ (p=0.143 n=10)   6.297m ± 6%       ~ (p=0.393 n=10)
geomean                                               108.6m        111.2m       +2.38%                  110.5m       +1.77%

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@ie-pham ie-pham marked this pull request as ready for review June 25, 2024 04:04
@ie-pham ie-pham changed the title [wip]]TraceQL] add support for event attribute query [TraceQL] add support for event attribute query Jun 25, 2024
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Thank you for updating the doc! Approving the doc changes.

Copy link
Collaborator

@stoewer stoewer left a comment

Choose a reason for hiding this comment

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

Great PR! I tested the event attributes locally and everything worked as expected.

I left a couple of comments for improvements

pkg/parquetquery/iters.go Outdated Show resolved Hide resolved
pkg/parquetquery/iters_test.go Outdated Show resolved Hide resolved
pkg/parquetquery/iters_test.go Outdated Show resolved Hide resolved
pkg/traceql/expr.y Outdated Show resolved Hide resolved
tempodb/encoding/vparquet4/block_traceql.go Outdated Show resolved Hide resolved
docs/sources/tempo/traceql/_index.md Outdated Show resolved Hide resolved
docs/sources/tempo/traceql/_index.md Outdated Show resolved Hide resolved
tempodb/encoding/vparquet4/block_traceql.go Outdated Show resolved Hide resolved
pkg/parquetquery/iters.go Outdated Show resolved Hide resolved
pkg/parquetquery/iters.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.

fantastic work here. i think it's close, but i want to make an honest effort at this perf regression before merging.

I would like to see tests added to ./tempodb/tempodb_search_test.go:TestSearchCompleteBlock

And I would expect local manual testing using the local docker-compose to confirm things work as expected.

docs/sources/tempo/traceql/_index.md Outdated Show resolved Hide resolved
pkg/parquetquery/iters.go Show resolved Hide resolved
pkg/parquetquery/iters.go Show resolved Hide resolved
pkg/parquetquery/iters.go Outdated Show resolved Hide resolved
pkg/parquetquery/iters.go Show resolved Hide resolved
pkg/parquetquery/iters.go Outdated Show resolved Hide resolved
tempodb/encoding/vparquet4/block_traceql.go 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.

o7

@ie-pham ie-pham merged commit 78ec5d4 into grafana:main Jun 28, 2024
15 checks passed
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