Skip to content

enhancement: batch span-only metrics evaluation - #7711

Open
perfloop-agent wants to merge 2 commits into
grafana:mainfrom
perfloop:perfloop-pr-open-5avwaczr2g
Open

enhancement: batch span-only metrics evaluation#7711
perfloop-agent wants to merge 2 commits into
grafana:mainfrom
perfloop:perfloop-pr-open-5avwaczr2g

Conversation

@perfloop-agent

Copy link
Copy Markdown

What this PR does:

Batches uncapped span-only TraceQL metrics evaluation in groups of up to 64 spans so concurrent WAL-block workers acquire the shared evaluator mutex once per batch instead of once per span.

Storage iteration and SecondPass callbacks remain outside the aggregation lock. Batch-owned span state is released after aggregation, and reusable array-valued attributes are snapshot-cloned before that lock is acquired. Queries with MaxSeries intentionally retain per-span iterator advancement so they stop storage at the exact cutoff rather than prefetching a batch past it.

This reduces lock handoff pressure for concurrent WAL-backed range queries while bounding retained iterator state.

Which issue(s) this PR fixes:

None.

Testing:

Added coverage for batch behavior, result equivalence, watchers, exemplars, array attributes, cancellation, max-series handling, iterator fallbacks, and WAL batch propagation. The validation scope also includes focused TraceQL and vParquet5 race tests, livestore and querier tests, the contention benchmark under the race detector, vet, and changelog validation.

Workload: uncapped span-only QueryRange across 10 concurrent WAL blocks

Metric Before After Change
ns/op 10387057 7125950 31.4% lower

Workload: uncapped span-only QueryRange CPU and evaluator contention profiles across 10 WAL blocks

Metric Before After Change
evaluator-mutex-delay-ns/op 24971800 4585525 81.6% lower
evaluator-block-delay-ns/op 27577050 5174720 81.2% lower
All 7 declared correctness checks passed.

AI assistance:

An AI coding assistant was used to inspect the existing batching path, prepare the implementation, and run focused validation.

Checklist

  • Tests updated
  • Documentation added (not needed; this preserves query behavior)
  • Changelog entry added under .chloggen/

Authored and verified by Perfloop: every claim above was co-measured on both trees and independently re-verified before submission — the full record is public: case_wqf5vvek76. Replies from this account are human-approved, and a human operator is accountable for this contribution.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Signed commits report

All 2 commits between main and perfloop-pr-open-5avwaczr2g have verified signatures. ✅

@cla-assistant

cla-assistant Bot commented Aug 9, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@cla-assistant

cla-assistant Bot commented Aug 9, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@tsenart
tsenart force-pushed the perfloop-pr-open-5avwaczr2g branch from a6a0595 to 2659e81 Compare August 9, 2026 12:02
@mdisibio mdisibio self-assigned this Aug 11, 2026
@mdisibio

mdisibio commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Hi thanks for trying this out. It's interesting that this was picked up, I was very sad having to add that mutex because it was known to be causing ~10% performance hit but also necessary for the live stores since they try to operate on everything concurrently.

The benchmarks above look nice, but I'm hesitant to go with this approach for a few reasons:

  1. Avoiding pooling was one of the primary reasons for the new fetch layer. It was adding complexity and also something that has been on mind about buffer reuse risk in a multi-tenant setup. I would heavily prefer not to readd pooling. Long-term I actually hope to delete the old fetch layer and the pool with it.

  2. Also seeing some regression on backend block querying, which is done by the querier against object storage. The gist is that we are paying for the batching to support concurrency even in this case where we don't need it.

                                                                                                       │ main.cpu4.txt │             pr.cpu4.txt              │
                                                                                                       │    sec/op     │    sec/op     vs base                │
BackendBlockQueryRange/{}_|_rate()-4                                                                       904.9m ± 0%   1014.2m ± 0%  +12.08% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_rate()_with(spanonly_fetch=false)-4                                             2.096 ± 2%     2.097 ± 1%        ~ (p=0.739 n=10)
BackendBlockQueryRange/{}_|_rate()_with(sample=true)-4                                                     349.8m ± 0%    318.4m ± 1%   -8.98% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.status_code)-4                                             7.664 ± 2%     7.917 ± 0%   +3.31% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(resource.service.name)-4                                             1.971 ± 0%     2.118 ± 0%   +7.48% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_rate()_by_(span.http.url)-4                                                    6.844m ± 0%    6.898m ± 0%   +0.79% (p=0.000 n=10)
BackendBlockQueryRange/{resource.service.name=`loki-ingester`}_|_rate()-4                                  203.6m ± 0%    205.8m ± 0%   +1.08% (p=0.000 n=10)
BackendBlockQueryRange/{span.http.host_!=_``_&&_span.http.flavor=`2`}_|_rate()_by_(span.http.flavor)-4      2.604 ± 1%     2.609 ± 0%        ~ (p=0.089 n=10)
BackendBlockQueryRange/{status=error}_|_rate()-4                                                           555.4m ± 0%    559.6m ± 0%   +0.76% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_quantile_over_time(duration,_.99,_.9,_.5)-4                                     2.687 ± 0%     2.848 ± 0%   +5.99% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_quantile_over_time(duration,_.99)_by_(span.http.status_code)-4                  9.539 ± 0%     9.718 ± 1%   +1.88% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_histogram_over_time(duration)-4                                                 2.690 ± 1%     2.870 ± 4%   +6.66% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_avg_over_time(duration)_by_(span.http.status_code)-4                            8.555 ± 0%     8.857 ± 1%   +3.53% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_max_over_time(duration)_by_(span.http.status_code)-4                            8.614 ± 0%     8.903 ± 1%   +3.36% (p=0.000 n=10)
BackendBlockQueryRange/{}_|_min_over_time(duration)_by_(span.http.status_code)-4                            8.616 ± 0%     8.807 ± 0%   +2.21% (p=0.000 n=10)
BackendBlockQueryRange/{_name_!=_nil_}_|_compare({status=error})-4                                         57.18m ± 1%    57.40m ± 0%        ~ (p=0.190 n=10)
BackendBlockQueryRange/{}_>_{}_|_rate()_by_(name)-4                                                        15.87m ± 6%    15.88m ± 6%        ~ (p=0.393 n=10)
BackendBlockQueryRange/({}_|_rate())_+_({}_|_rate())-4                                                     915.8m ± 0%   1035.0m ± 0%  +13.02% (p=0.000 n=10)
BackendBlockQueryRange/({}_|_rate())_-_({}_|_rate())-4                                                     916.6m ± 0%   1035.1m ± 0%  +12.93% (p=0.000 n=10)
BackendBlockQueryRange/({}_|_rate())_*_({}_|_rate())-4                                                     915.2m ± 0%   1034.4m ± 0%  +13.02% (p=0.000 n=10)
BackendBlockQueryRange/({}_|_rate())_/_({}_|_count_over_time())-4                                           1.834 ± 0%     2.075 ± 1%  +13.11% (p=0.000 n=10)
geomean                                                                                                     1.006          1.049        +4.27%

Other ideas

This PR makes me realize that we don't need the mutex for backend querying at all, since it's always a single block per job in separate evaluators. Thanks, this would make a very nice separate PR, to add it as an option to the compile function like WithNoLock().

Regarding live store contention, I'm guessing from the PR that you're seeing a lot of contention issues with WAL blocks? The default flush settings generally minimize this, i.e. only the last 30/60s is in the form of a WAL block. Have you customized any settings or have anything unique to your environment?

I've been thinking the issue is that we are trying to run every span from every block through an evaluator. The batching is a way to share it faster, but if we take a step back the real issue is that it actually doesn't need to be the same evaluator. For local on-disk caching we already evaluate each block separately, and maybe there is a way to do the WAL block flushes the same way.

It's possible that there is a way we can still do batching without pooling. What if we allocated a slice of []span directly instead of pooling pointers? I think for the interface appeasement it would need to also have a mirror slice of []traceql.Span

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.

2 participants