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

GetMetrics second pass + other improvements #2501

Merged
merged 15 commits into from
Jun 2, 2023

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented May 23, 2023

What this PR does:
Contains a variety of performance improvements mainly directed at the new dynamic metrics API. It can process several million spansets per call so it suffered from memory churn significantly more than normal search. A summary of the changes:

  • Fix GetMetrics span limit to be honored with caching
  • Fix GetMetrics to close the results iter
  • Fix GetMetrics to allo span pool by using the SecondPass callback
  • Default metrics generator to vParquet2 instead of CurrentEncoding (Duration column is good to have)
  • SpansetFilter buffer reuse
  • Spanset pooling
  • Optimally allocate slice in traceCollector

Currently only made the changes in vParquet2, but can backport them to vParquet after review, and can run the standard traceql benchmarks too.

name                               old time/op    new time/op    delta
BackendBlockGetMetrics/{}/name-12     1.20s ± 2%     1.02s ±10%  -14.37%  (p=0.000 n=8+9)

name                               old alloc/op   new alloc/op   delta
BackendBlockGetMetrics/{}/name-12    1.13GB ± 0%    0.07GB ± 2%  -93.60%  (p=0.000 n=9+8)

name                               old allocs/op  new allocs/op  delta
BackendBlockGetMetrics/{}/name-12     4.65M ± 0%     1.69M ± 0%  -63.63%  (p=0.000 n=9+8)

Main benchmarks are mixed 🤷

name                                                 old time/op    new time/op    delta
BackendBlockTraceQL/spanAttNameNoMatch-12              3.45ms ± 3%    3.38ms ± 2%   -1.96%  (p=0.043 n=10+8)
BackendBlockTraceQL/spanAttValNoMatch-12               3.43ms ± 2%    3.82ms ±28%     ~     (p=0.203 n=8+10)
BackendBlockTraceQL/spanAttValMatch-12                 3.41ms ± 5%    3.47ms ± 7%     ~     (p=0.720 n=10+9)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-12         2.55ms ± 1%    2.59ms ± 3%     ~     (p=0.055 n=8+10)
BackendBlockTraceQL/spanAttIntrinsicMatch-12           33.9ms ± 3%    34.3ms ± 3%     ~     (p=0.549 n=10+9)
BackendBlockTraceQL/spanAttIntrinsicRegexNoMatch-12    2.68ms ± 4%    2.64ms ± 4%     ~     (p=0.190 n=10+10)
BackendBlockTraceQL/spanAttIntrinsicRegexMatch-12      47.5ms ± 5%    48.7ms ± 1%     ~     (p=0.050 n=9+9)
BackendBlockTraceQL/resourceAttNameNoMatch-12          2.46ms ± 1%    2.36ms ± 4%   -4.30%  (p=0.000 n=9+9)
BackendBlockTraceQL/resourceAttValNoMatch-12           7.43ms ± 3%    6.72ms ± 6%   -9.54%  (p=0.000 n=9+9)
BackendBlockTraceQL/resourceAttValMatch-12              207ms ± 5%     203ms ± 3%     ~     (p=0.200 n=9+8)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-12     1.96ms ±12%    2.15ms ±15%  +10.02%  (p=0.006 n=9+10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-12       1.95ms ± 5%    2.17ms ±13%  +11.32%  (p=0.000 n=10+10)
BackendBlockTraceQL/mixedNameNoMatch-12                 398ms ± 1%     410ms ± 2%   +3.00%  (p=0.000 n=9+9)
BackendBlockTraceQL/mixedValNoMatch-12                  404ms ± 2%     422ms ± 4%   +4.45%  (p=0.002 n=8+10)
BackendBlockTraceQL/mixedValMixedMatchAnd-12           2.52ms ± 6%    2.64ms ±11%   +4.99%  (p=0.043 n=10+10)
BackendBlockTraceQL/mixedValMixedMatchOr-12             259ms ± 1%     272ms ± 5%   +5.08%  (p=0.000 n=9+10)
BackendBlockTraceQL/mixedValBothMatch-12               2.09ms ±30%    1.96ms ± 8%     ~     (p=0.436 n=10+10)

name                                                 old speed      new speed      delta
BackendBlockTraceQL/spanAttNameNoMatch-12             487MB/s ± 2%   492MB/s ± 8%     ~     (p=0.156 n=10+9)
BackendBlockTraceQL/spanAttValNoMatch-12              490MB/s ± 2%   446MB/s ±23%     ~     (p=0.203 n=8+10)
BackendBlockTraceQL/spanAttValMatch-12                493MB/s ± 5%   484MB/s ± 7%     ~     (p=0.720 n=10+9)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-12        379MB/s ± 1%   374MB/s ± 3%     ~     (p=0.055 n=8+10)
BackendBlockTraceQL/spanAttIntrinsicMatch-12          395MB/s ± 3%   392MB/s ± 3%     ~     (p=0.549 n=10+9)
BackendBlockTraceQL/spanAttIntrinsicRegexNoMatch-12   361MB/s ± 4%   366MB/s ± 3%     ~     (p=0.190 n=10+10)
BackendBlockTraceQL/spanAttIntrinsicRegexMatch-12     283MB/s ± 5%   276MB/s ± 1%     ~     (p=0.050 n=9+9)
BackendBlockTraceQL/resourceAttNameNoMatch-12         346MB/s ± 1%   362MB/s ± 4%   +4.52%  (p=0.000 n=9+9)
BackendBlockTraceQL/resourceAttValNoMatch-12          448MB/s ± 3%   489MB/s ±13%   +9.02%  (p=0.002 n=9+10)
BackendBlockTraceQL/resourceAttValMatch-12           78.5MB/s ± 5%  80.2MB/s ± 3%     ~     (p=0.190 n=9+8)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-12   81.7MB/s ±11%  74.5MB/s ±14%   -8.86%  (p=0.006 n=9+10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-12     81.8MB/s ± 5%  73.7MB/s ±12%   -9.93%  (p=0.000 n=10+10)
BackendBlockTraceQL/mixedNameNoMatch-12              26.9MB/s ± 3%  26.2MB/s ± 2%   -2.58%  (p=0.001 n=10+9)
BackendBlockTraceQL/mixedValNoMatch-12               26.6MB/s ± 2%  25.5MB/s ± 4%   -4.17%  (p=0.002 n=8+10)
BackendBlockTraceQL/mixedValMixedMatchAnd-12          339MB/s ± 5%   324MB/s ±10%   -4.56%  (p=0.043 n=10+10)
BackendBlockTraceQL/mixedValMixedMatchOr-12          66.0MB/s ± 1%  62.8MB/s ± 5%   -4.76%  (p=0.000 n=9+10)
BackendBlockTraceQL/mixedValBothMatch-12             77.7MB/s ±25%  81.8MB/s ± 7%     ~     (p=0.424 n=10+10)

name                                                 old MB_io/op   new MB_io/op   delta
BackendBlockTraceQL/spanAttNameNoMatch-12                1.68 ± 0%      1.68 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttValNoMatch-12                 1.68 ± 0%      1.68 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttValMatch-12                   1.68 ± 0%      1.68 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-12           0.97 ± 0%      0.97 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttIntrinsicMatch-12             13.4 ± 0%      13.4 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttIntrinsicRegexNoMatch-12      0.97 ± 0%      0.97 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttIntrinsicRegexMatch-12        13.4 ± 0%      13.4 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttNameNoMatch-12            0.85 ± 0%      0.85 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttValNoMatch-12             3.33 ± 0%      3.33 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttValMatch-12               16.2 ± 0%      16.2 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-12       0.16 ± 0%      0.16 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttIntrinsicMatch-12         0.16 ± 0%      0.16 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedNameNoMatch-12                  10.8 ± 0%      10.8 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedValNoMatch-12                   10.8 ± 0%      10.8 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedValMixedMatchAnd-12             0.85 ± 0%      0.85 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedValMixedMatchOr-12              17.1 ± 0%      17.1 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedValBothMatch-12                 0.16 ± 0%      0.16 ± 0%     ~     (all equal)

name                                                 old alloc/op   new alloc/op   delta
BackendBlockTraceQL/spanAttNameNoMatch-12              1.06MB ± 6%    1.05MB ± 4%     ~     (p=0.739 n=10+10)
BackendBlockTraceQL/spanAttValNoMatch-12               1.04MB ± 4%    1.08MB ± 9%     ~     (p=0.095 n=9+10)
BackendBlockTraceQL/spanAttValMatch-12                 1.04MB ± 4%    1.07MB ± 2%   +2.95%  (p=0.013 n=10+9)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-12         1.08MB ± 1%    1.11MB ± 3%   +3.10%  (p=0.001 n=7+10)
BackendBlockTraceQL/spanAttIntrinsicMatch-12           4.56MB ± 4%    4.61MB ± 7%     ~     (p=0.218 n=10+10)
BackendBlockTraceQL/spanAttIntrinsicRegexNoMatch-12    1.13MB ± 1%    1.13MB ± 3%     ~     (p=0.815 n=9+8)
BackendBlockTraceQL/spanAttIntrinsicRegexMatch-12      4.82MB ± 8%    4.87MB ± 2%     ~     (p=0.211 n=10+9)
BackendBlockTraceQL/resourceAttNameNoMatch-12           981kB ± 3%     970kB ± 6%     ~     (p=0.315 n=9+10)
BackendBlockTraceQL/resourceAttValNoMatch-12           7.48MB ± 2%    7.45MB ± 2%     ~     (p=0.684 n=10+10)
BackendBlockTraceQL/resourceAttValMatch-12              145MB ± 2%     129MB ± 1%  -10.84%  (p=0.000 n=10+10)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-12      968kB ± 6%     979kB ± 5%     ~     (p=0.243 n=9+10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-12        959kB ± 3%     948kB ± 3%     ~     (p=0.278 n=10+9)
BackendBlockTraceQL/mixedNameNoMatch-12                88.5MB ± 1%    89.2MB ± 9%     ~     (p=0.875 n=6+10)
BackendBlockTraceQL/mixedValNoMatch-12                 88.3MB ±10%    90.0MB ± 9%     ~     (p=0.842 n=10+9)
BackendBlockTraceQL/mixedValMixedMatchAnd-12            979kB ± 3%    1006kB ± 4%   +2.70%  (p=0.035 n=10+10)
BackendBlockTraceQL/mixedValMixedMatchOr-12            10.7MB ± 9%    10.9MB ±12%     ~     (p=0.684 n=10+10)
BackendBlockTraceQL/mixedValBothMatch-12                980kB ± 5%     982kB ± 2%     ~     (p=0.604 n=10+9)

name                                                 old allocs/op  new allocs/op  delta
BackendBlockTraceQL/spanAttNameNoMatch-12               12.3k ± 0%     12.3k ± 0%     ~     (p=1.000 n=10+10)
BackendBlockTraceQL/spanAttValNoMatch-12                12.3k ± 0%     12.3k ± 0%   +0.01%  (p=0.023 n=8+10)
BackendBlockTraceQL/spanAttValMatch-12                  12.3k ± 0%     12.3k ± 0%   +0.01%  (p=0.008 n=8+10)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-12          12.3k ± 0%     12.3k ± 0%     ~     (p=0.176 n=7+10)
BackendBlockTraceQL/spanAttIntrinsicMatch-12             190k ± 0%      190k ± 0%   -0.03%  (p=0.000 n=10+10)
BackendBlockTraceQL/spanAttIntrinsicRegexNoMatch-12     12.9k ± 0%     12.9k ± 0%     ~     (p=0.871 n=10+10)
BackendBlockTraceQL/spanAttIntrinsicRegexMatch-12        191k ± 0%      191k ± 0%   -0.03%  (p=0.000 n=10+10)
BackendBlockTraceQL/resourceAttNameNoMatch-12           12.3k ± 0%     12.3k ± 0%     ~     (p=0.650 n=10+10)
BackendBlockTraceQL/resourceAttValNoMatch-12            12.5k ± 0%     12.5k ± 0%     ~     (p=1.000 n=10+10)
BackendBlockTraceQL/resourceAttValMatch-12              1.35M ± 0%     1.23M ± 0%   -8.46%  (p=0.000 n=10+10)
BackendBlockTraceQL/resourceAttIntrinsicNoMatch-12      12.2k ± 0%     12.2k ± 0%     ~     (p=0.294 n=8+10)
BackendBlockTraceQL/resourceAttIntrinsicMatch-12        12.2k ± 0%     12.2k ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedNameNoMatch-12                 14.4k ± 0%     14.4k ± 0%     ~     (p=0.382 n=10+10)
BackendBlockTraceQL/mixedValNoMatch-12                  14.4k ± 0%     14.4k ± 0%     ~     (p=0.507 n=10+9)
BackendBlockTraceQL/mixedValMixedMatchAnd-12            12.3k ± 0%     12.3k ± 0%     ~     (p=0.176 n=10+7)
BackendBlockTraceQL/mixedValMixedMatchOr-12              191k ± 0%      191k ± 0%   -0.02%  (p=0.001 n=9+10)
BackendBlockTraceQL/mixedValBothMatch-12                12.2k ± 0%     12.2k ± 0%     ~     (all equal)

Which issue(s) this PR fixes:
Fixes n/a

Checklist

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

@knylander-grafana
Copy link
Contributor

@mdisibio Will we need any doc updates? Should we can add something to the metrics-generator doc about updating to vParquet2?

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.

lot o thoughts. i'll give some space to @zalegrala but this lgtm. i'm ready to approve if desired

modules/generator/config.go Outdated Show resolved Hide resolved
modules/generator/processor/localblocks/config.go Outdated Show resolved Hide resolved
pkg/traceql/ast.go Show resolved Hide resolved
pkg/traceql/ast.go Outdated Show resolved Hide resolved
pkg/traceql/ast.go Outdated Show resolved Hide resolved
}

if len(matchingSpans) == 0 {
if len(f.matchingSpansBuffer) == len(ss.Spans) {
Copy link
Member

Choose a reason for hiding this comment

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

so, i think this is safe, but i need to mention the bug where manipulating the input slice of spans caused the double add the sync pool and panic'ed everything.

we do have tests this cover this and working through this in my head makes me believe there are no issues.

Copy link
Contributor

@zalegrala zalegrala left a comment

Choose a reason for hiding this comment

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

Nice work, this looks good to me.

@mdisibio
Copy link
Contributor Author

mdisibio commented Jun 2, 2023

@mdisibio Will we need any doc updates? Should we can add something to the metrics-generator doc about updating to vParquet2?

@knylander-grafana The default change was just for the new experimental feature that isn't documented yet, however it was reverted anyway.

@mdisibio mdisibio merged commit d091f8c into grafana:main Jun 2, 2023
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.

5 participants