-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Add hits and misses timing stats to DLS cache #133314
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
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.
Pull Request Overview
This PR enhances the Document Level Security (DLS) cache to track timing statistics for cache hits and misses. This follows the same pattern used in other cache implementations like GeoIpCache or EnrichCache.
- Add timing measurement capabilities to track how long cache hits and misses take
- Expose
hits_time_in_millis
andmisses_time_in_millis
in cache usage statistics - Refactor constructor to accept a configurable time provider for better testability
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
DocumentSubsetBitsetCache.java | Add timing measurement fields, constructor overload with time provider, and logic to track hit/miss timing in getBitSet() method |
DocumentSubsetBitsetCacheTests.java | Update tests to use mock time provider and verify timing statistics in cache usage stats |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ava/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java
Outdated
Show resolved
Hide resolved
...ava/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java
Outdated
Show resolved
Hide resolved
Hi @szybia, I've created a changelog YAML for you. |
Pinging @elastic/es-security (Team:Security) |
final BitsetCacheKey cacheKey = new BitsetCacheKey(indexKey, query); | ||
|
||
try (ReleasableLock ignored = cacheModificationLock.acquire()) { | ||
final AtomicBoolean cacheKeyWasPresent = new AtomicBoolean(true); |
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.
Let's not introduce an AtomicBoolean
for this. You're single threaded here anyway (right?), so this could just be an ordinary old mutable boolean
rather than a final
. Beyond that, though, I suspect I'd prefer if this were done by threading the logic in where it needs to be rather than separating it (and needing a variable at all). edit: disregard that, the other places where we don't do this are patterned differently. It's interesting to me that these three caches have such different little details in some places.
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.
Ah, the problem is the callback and the need for it to be a final variable -- there's a pattern of using final 1-length arrays as final-but-mutable box for this.
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.
yah seen the cacheKeyWasPresent[0] = false;
approach before
no strong opinions so fine with it, but was my first time seeing it and was a bit confused initially
so went with this since more readable imo
i'd be curious to know whether there were any previous issues around AtomicBoolean, from ignorance and intuition given zero contention and this just being a object allocation, can't imagine many problems here
anyway 🤷
so that all three of these caches have relativeNanoTimeProvider, then hitsTimeInNanos, then missesTimeInNanos.
This comment was marked as resolved.
This comment was marked as resolved.
I added a couple of nitpick commits as a form of review -- it seemed faster and easier to just do the bits rather than talking about them. Great work, and LGTM! |
- Add `hits_time_in_millis` and `misses_time_in_millis` to DLS cache stats - Approach is the same as GeoIpCache or EnrichCache
hits_time_in_millis
andmisses_time_in_millis
to DLS cache stats