-
Notifications
You must be signed in to change notification settings - Fork 10
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
Check cached postings TTL before returning from cache + metrics #822
base: main
Are you sure you want to change the base?
Check cached postings TTL before returning from cache + metrics #822
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach LGTM.
It'd be interesting to benchmark this and compare the cost of checking the done
channel with the approach prior to #734.
tsdb/postings_for_matchers_cache.go
Outdated
case <-oldPromise.done: | ||
if c.timeNow().Sub(oldPromise.evaluationCompletedAt) >= c.ttl { | ||
// The cached promise already expired, but it has not been evicted. | ||
// TODO trace + metric |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not something for this PR) Metrics for the PFMC in general would be handy - would be nice to see the rate of cache hits and misses, as well as the rate of cache evictions and their reason (TTL vs cache growing too large).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well as the rate of cache evictions and their reason (TTL vs cache growing too large)
Will do in a separate PR.
Signed-off-by: Marco Pracucci <[email protected]>
b826f27
to
69f78e0
Compare
…otherGoroutineIsEvictingTheCache Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
select { | ||
case <-firstCallReceived: | ||
case <-time.After(5 * time.Second): | ||
// Do not block forever. The test will anyway in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a word missing in this comment - "the test will XXX (fail?) anyway in this case"
@@ -211,13 +254,15 @@ func (c *PostingsForMatchersCache) postingsForMatchersPromise(ctx context.Contex | |||
} | |||
} | |||
|
|||
c.metrics.hits.Inc() | |||
span.AddEvent("using cached postingsForMatchers promise", trace.WithAttributes( | |||
attribute.String("cache_key", key), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to include evaluationCompletedAt
here as well, to replace what was reverted from #820.
In this PR I propose two changes to PostingsForMatchers cache:
expire()
and another one skipping theexpire()
execution because it's already in progress.The following benchmark shows the difference betweeen:
01bb37aae
: the commit before Reduce PostingsForMatchersCache.expire() pressure on mutex #7346c2603082
: the commit at Reduce PostingsForMatchersCache.expire() pressure on mutex #734main