hanahmily commented on code in PR #1213:
URL:
https://github.com/apache/skywalking-banyandb/pull/1213#discussion_r3558703338
##########
banyand/liaison/grpc/bydbql.go:
##########
@@ -143,6 +166,79 @@ func (b *bydbQLService) Query(ctx context.Context, req
*bydbqlv1.QueryRequest) (
return resp, nil
}
+// topKDumper tracks the top cache-miss and slow queries and, on a supervised
+// goroutine, periodically logs the cumulative top-K. All methods are
nil-safe, so the
+// call sites need no guards when the top-K log is disabled (the dumper is
nil).
+type topKDumper struct {
+ miss *topK
+ slow *topK
+ l *logger.Logger
+ cancel context.CancelFunc
+}
+
+// newTopKDumper starts the trackers and the dump goroutine; a non-positive
interval
+// disables the feature and returns nil.
+func newTopKDumper(interval time.Duration, l *logger.Logger) *topKDumper {
+ if interval <= 0 {
+ return nil
+ }
+ ctx, cancel := context.WithCancel(context.Background())
+ d := &topKDumper{miss: newTopK(bydbqlTopKSize), slow:
newTopK(bydbqlTopKSize), l: l, cancel: cancel}
+ run.Go(ctx, "liaison.grpc.bydbql.topk-dump", l, func(ctx
context.Context) {
+ ticker := time.NewTicker(interval)
+ defer ticker.Stop()
+ for {
+ select {
+ case <-ctx.Done():
+ return
+ case <-ticker.C:
+ d.dump()
+ }
+ }
+ })
+ return d
+}
+
+func (d *topKDumper) observeMiss(query string) {
+ if d != nil {
+ d.miss.observe(query, 0)
+ }
+}
+
+func (d *topKDumper) observeSlow(query string, dur time.Duration) {
+ if d != nil {
+ d.slow.observe(query, dur)
+ }
+}
+
+func (d *topKDumper) close() {
+ if d != nil && d.cancel != nil {
+ d.cancel()
+ }
+}
+
+func (d *topKDumper) dump() {
+ d.logTopK(d.miss, "top bydbql cache-miss queries", func(s topKSlot)
string {
Review Comment:
**Requested change — the cumulative miss top-K conflates cold-start with
thrashing.**
This list tracks misses cumulatively since process start. But every
parameterized template necessarily misses exactly once on its first lookup
(cold cache), so a perfectly healthy, always-hitting template still appears
here at `count=1` and stays for the life of the process. That makes *presence*
in the log non-actionable — only an elevated/growing miss count (a template
repeatedly evicted and re-parsed → thrashing) is a real signal.
An operator reading a log titled "cache-miss queries" is likely to misread
benign `count=1` cold-starts as misconfigured/uncacheable queries.
Suggest surfacing only *problematic* misses — see the tracker note on
`topk.go`; per-interval windowing fixes this too.
The slow-query top-K and the `bydbql_prepared_cache_hit_ratio` gauge don't
have this issue — this is specifically the cache-miss list's cumulative
semantics.
##########
banyand/liaison/grpc/topk.go:
##########
@@ -0,0 +1,94 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package grpc
+
+import (
+ "math"
+ "sort"
+ "sync"
+ "time"
+)
+
+// bydbqlTopKSize is the number of hot entries each top-K tracker keeps.
+const bydbqlTopKSize = 10
+
+// topKSlot is one tracked query and its accumulated statistics.
+type topKSlot struct {
+ key string
+ count uint64
+ maxDur time.Duration
+}
+
+// topK is a bounded approximate heavy-hitters tracker (Space-Saving): it
keeps at
+// most k entries and, when full, evicts the least-frequent one, letting the
new key
+// inherit that entry's count + 1 so a fresh key gets a fair chance instead of
being
+// evicted again immediately. With k small (10) the min scan and the snapshot
sort are
+// effectively O(1), and observing an existing key needs no reordering at all.
+type topK struct {
+ slots map[string]*topKSlot
+ k int
+ mu sync.Mutex
+}
+
+func newTopK(k int) *topK {
+ if k < 1 {
+ k = 1
+ }
+ return &topK{slots: make(map[string]*topKSlot, k), k: k}
+}
+
+// observe records one occurrence of key; dur is the query latency (0 when
latency is
+// not tracked, e.g. the cache-miss queue). maxDur keeps the largest latency
seen.
+func (t *topK) observe(key string, dur time.Duration) {
+ t.mu.Lock()
+ defer t.mu.Unlock()
+ if s, ok := t.slots[key]; ok {
+ s.count++
+ if dur > s.maxDur {
+ s.maxDur = dur
+ }
+ return
+ }
+ if len(t.slots) < t.k {
+ t.slots[key] = &topKSlot{key: key, count: 1, maxDur: dur}
+ return
+ }
+ // Full: evict the least-frequent entry and let the new key inherit its
count.
Review Comment:
**Requested change — the Space-Saving tracker is correct/thread-safe, but
the reported numbers aren't actionable in the scenario this feature targets.**
Ranked:
1. **(primary) Add windowing.** Both trackers are cumulative for the whole
process lifetime. Reset — or exponentially decay — the maps at each
dump/`snapshot`. This one change fixes items 2 and 3 below and largely
neutralizes 4, and makes the numbers track the *current* workload instead of
ancient history.
2. **`count=` is a Space-Saving over-estimate, not a true count.** The
newcomer inherits `minCount+1`, so its printed count includes frequency donated
by other evicted keys, and no per-entry error term (ε) is tracked to bound it.
`count=812` does not mean the query missed 812 times. If you keep it
cumulative, track ε so `count` can be reported as a bounded range; windowing
makes this mostly moot.
3. **Inflation is worst exactly during thrashing** — the case the miss log
exists to diagnose. In a high-cardinality stream (mostly-unique templates)
nearly every `observe` hits this eviction branch; the min-count floor rises ~1
every k inserts, so after N observations all k slots converge to ≈ N/k with
near-identical counts. Result: the top-10 becomes 10 arbitrary queries with
indistinguishable counts, and the real repeat-offenders don't stand out.
4. **`k = 10` is a weak guarantee** — Space-Saving with k=10 only reliably
surfaces items exceeding ~10% of the stream, which a many-template workload
rarely hits. Bump to ~100–200; the O(k) min-scan and map stay trivially cheap.
5. **`maxDur` is silently lost on eviction.** A slow query that is evicted
and later re-observed re-enters as a fresh slot with `maxDur = dur`, discarding
its earlier peak → `max_latency` under-reports for churning keys. Windowing
moots this within a window.
6. **Slow queries are ranked by frequency only.** `snapshot` sorts by
`count` desc, so a query that was catastrophically slow once (huge
`max_latency`, `count=1`) gets buried/evicted under frequently-mildly-slow
ones. Rank or additionally emit the slow tracker by `max_latency`.
7. **(polish) Non-deterministic tie-break.** Equal-count entries order by
map iteration + unstable sort, so output order flaps between dumps. Sort by
`(count desc, maxDur desc, key)`.
Recommendation: do 1 + 4 (and 6 for the slow tracker) — small,
high-leverage, and they resolve the rest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
