[TraceQL] add support for event attribute query by ie-pham · Pull Request #3748 · grafana/tempo

ie-pham · 2024-06-04T19:08:26Z

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%

--- 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]

knylander-grafana

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

stoewer

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

I left a couple of comments for improvements

joe-elliott

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.

joe-elliott

o7

* lexer first * block stuff * adding one more definition level * added tests back * lint * lets see if the hack passes tests * the real change * 8 levels * update doc * rebase and changelog * meow * lint smh * oops docs * addressing comments * oopsie * panic! at the dis-row..number * lint * thank you, next() * update test * fix test and docs * lint * i love linting

joe-elliott mentioned this pull request Jun 5, 2024

Allow searching by event attribute in TraceQL #2313

Closed

ie-pham force-pushed the eventattr branch 2 times, most recently from 13c9df6 to e761187 Compare June 13, 2024 17:23

ie-pham added 10 commits June 24, 2024 22:34

lexer first

9edf0ed

block stuff

5eabea8

adding one more definition level

93ce3f6

added tests back

e7587d6

lint

f1e14b4

lets see if the hack passes tests

b656992

the real change

484b571

8 levels

463890a

update doc

c4d98e9

rebase and changelog

2d03712

ie-pham force-pushed the eventattr branch from bc03875 to 2d03712 Compare June 25, 2024 04:04

ie-pham marked this pull request as ready for review June 25, 2024 04:04

ie-pham requested review from annanay25, electron0zero, joe-elliott, knylander-grafana, mapno, mdisibio, stoewer, yvrhdn and zalegrala as code owners June 25, 2024 04:04

ie-pham changed the title ~~[wip]]TraceQL] add support for event attribute query~~ Jun 25, 2024

ie-pham added 2 commits June 24, 2024 23:09

meow

b54275d

lint smh

3114356

ie-pham force-pushed the eventattr branch from bee2d39 to 3114356 Compare June 25, 2024 13:48

knylander-grafana reviewed Jun 25, 2024

View reviewed changes

oops docs

27a7502

stoewer reviewed Jun 26, 2024

View reviewed changes

ie-pham added 2 commits June 26, 2024 09:53

addressing comments

470f198

oopsie

78b6871

ie-pham mentioned this pull request Jun 26, 2024

[TraceQL] Add support for querying link attributes #3814

Merged

3 tasks

panic! at the dis-row..number

30c0640

ie-pham force-pushed the eventattr branch from 34288ea to 30c0640 Compare June 27, 2024 00:59

lint

7158af9

stoewer reviewed Jun 27, 2024

View reviewed changes

Comment thread pkg/parquetquery/iters.go Outdated

stoewer reviewed Jun 27, 2024

View reviewed changes

Comment thread pkg/parquetquery/iters.go Outdated

stoewer reviewed Jun 27, 2024

View reviewed changes

Comment thread pkg/parquetquery/iters_test.go Outdated

ie-pham force-pushed the eventattr branch from 4c2887b to 73dcf1c Compare June 27, 2024 01:58

thank you, next()

f28f4fe

ie-pham force-pushed the eventattr branch from 73dcf1c to f28f4fe Compare June 27, 2024 02:00

stoewer approved these changes Jun 27, 2024

View reviewed changes

joe-elliott reviewed Jun 27, 2024

View reviewed changes

ie-pham added 4 commits June 27, 2024 15:47

update test

f2f8fa0

fix test and docs

5726599

lint

da8405b

i love linting

5b8572b

joe-elliott approved these changes Jun 28, 2024

View reviewed changes

ie-pham merged commit 78ec5d4 into grafana:main Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TraceQL] add support for event attribute query - #3748

[TraceQL] add support for event attribute query#3748
ie-pham merged 22 commits into
grafana:mainfrom
ie-pham:eventattr

ie-pham commented Jun 4, 2024 •

edited

Loading

knylander-grafana left a comment

stoewer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joe-elliott left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joe-elliott left a comment

Labels

4 participants

Uh oh!

Conversation

ie-pham commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

knylander-grafana left a comment

Choose a reason for hiding this comment

stoewer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joe-elliott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joe-elliott left a comment

Choose a reason for hiding this comment

Labels

4 participants

ie-pham commented Jun 4, 2024 •

edited

Loading