Skip to content

Do not cache empty query results in QueryCache - #735

Open
olowosulu wants to merge 3 commits into
aws:mainfrom
olowosulu:fix/querycache-no-cache-empty
Open

Do not cache empty query results in QueryCache#735
olowosulu wants to merge 3 commits into
aws:mainfrom
olowosulu:fix/querycache-no-cache-empty

Conversation

@olowosulu

Copy link
Copy Markdown

Problem

QueryCache caches empty successful results, which poisons every later lookup for the
same metric within a test binary run.

In Get() and GetUnfiltered() the cache write executed unconditionally:

qc.filtered[metricName] = entry

When a query legitimately returns zero results with no error, that empty cacheEntry
is stored for the remainder of the run. Every subsequent call for that metric then hits
the cache and receives the empty result instead of re-querying, even though the metric
may have become available in the meantime.

Blast radius

The cache is package-level state created once per test package in each setup_test.go.
The impact is therefore largest in test/otel/standard/, where more than ten test files
share a single queryCache. A single early transient miss on a metric can turn into
failures across every later test in that package that queries the same metric.

The effect is confined to one test binary. Each test/otel/* package builds its own
cache, so this does not propagate between packages.

Fix

Cache only when there is something worth caching:

if len(entry.results) > 0 || entry.err != nil {
    qc.filtered[metricName] = entry
}

Errors are still cached, matching the previous behaviour. Only empty successful results
are left uncached.

Behavioural notes for reviewers

Two consequences worth being explicit about.

First, declining to cache an empty result means each subsequent call re-queries the
backend for that metric. This is the intended behaviour, but it does increase query
volume for a metric that is genuinely absent, where previously exactly one query was
made. In practice a genuinely absent metric fails the assertion on the first call, so
the additional volume is bounded in the common case.

Second, the concurrent waiter path in Get() now has a newly reachable state. Waiters
that block on the inflight channel read the cache afterwards without an existence check:

<-ch
qc.mu.RLock()
entry := qc.filtered[metricName]
qc.mu.RUnlock()
return entry.results, entry.err

Previously an entry was always present. Now the key can be absent, in which case Go
returns the zero cacheEntry and the waiter returns (nil, nil). That is exactly what
the fetching goroutine returns for the same query, so behaviour is consistent and
correct. It is worth noting that this relies on the zero value of cacheEntry being
semantically equivalent to an empty result, so a future field whose zero value carries
meaning would need this path revisited. I left the diff minimal rather than adding
explicit , ok checks at the three read sites, but I am happy to add them if you would
prefer the invariant made explicit.

The locking and inflight dedup design is otherwise unchanged.

Verification

Verified with Go 1.25.8, matching the ~1.23.0 used in CI rather than the go 1.20 in
go.mod, which is stale relative to the code (util/otelmetrics/query_cache.go already
imports log/slog, which requires Go 1.21+).

  • go build ./util/otelmetrics/ passes
  • go vet ./util/otelmetrics/ passes
  • go test -c -tags integration compiles for ./test/otel/standard/, ./test/otel/neuron/
    and ./test/otel/multi_efa/
  • One file changed, +11/-3

These are integration tests requiring live AWS infrastructure, so I was able to verify
compilation but not execution.

Scope

This change is deliberately limited to the caching bug so it can be reviewed on its own
merits. It is independent of any test-flakiness question and is provable from the code
alone.

QueryCache.Get() and QueryCache.GetUnfiltered() unconditionally write
the fetched entry into their respective maps, even when the result set
is empty and no error occurred. Because the cache is package-scoped and
lives for the entire test binary run, a single transient empty response
poisons every subsequent lookup for that metric. Later tests that call
Get() for the same metric receive the cached empty slice instead of
re-querying the backend.

Guard both cache writes with:
  if len(entry.results) > 0 || entry.err != nil

This ensures that errors are still cached (preserving existing
behaviour) while empty successful results are not stored.

Behavioural note: without a retry loop, declining to cache an empty
result means that each subsequent call for a genuinely absent metric
will re-query the backend rather than returning instantly from the
cache. This increases query volume for metrics that truly do not exist,
but it is the correct trade-off because transient absence is
indistinguishable from permanent absence at query time, and poisoning
the cache is the more severe failure mode.
@olowosulu
olowosulu requested a review from a team as a code owner August 4, 2026 07:37
Comment thread util/otelmetrics/query_cache.go
Comment thread util/otelmetrics/query_cache.go
- Add comma-ok checks at waiter map read sites so absent entries
  (empty result not cached) return nil, nil for retry
- Add query_cache_test.go with 7 unit tests covering the cache
  predicate: empty-not-stored, non-empty-stored, error-stored,
  waiter-gets-nil-on-empty, unfiltered variants, and promqlMetricSelector
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