Skip to content

GeoIpCache and EnrichCache improvements #132922

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

szybia
Copy link
Contributor

@szybia szybia commented Aug 14, 2025

  • AtomicLong -> LongAdder
    • we largely write stats and read very infrequently, so let's avoid any lock-contention possibilities
    • adding hits_misses_in_millis into DLS cache, and think LongAdder is the better class to use, so changing this to be consistent
  • call System::nanoTime twice to measure cache miss times, rather than four times

@szybia szybia added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Aug 14, 2025
@szybia szybia requested a review from joegallo August 14, 2025 13:14
@szybia szybia marked this pull request as ready for review August 14, 2025 13:14
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Aug 14, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine
Copy link
Collaborator

Hi @szybia, I've created a changelog YAML for you.

@joegallo
Copy link
Contributor

There's more prior art (not a ton, but some) for using a CounterMetric for tracking times in the Elasticsearch codebase than for using a LongAdder directly.

joegallo@simulacron:~/Code/elastic/elasticsearch $ git grep 'new LongAdder' | grep -i time | grep -i -E '(nanos|millis)' | grep -iv test/
x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/input/DirectBlobContainerIndexInput.java:            private final LongAdder timeNanos = new LongAdder();
joegallo@simulacron:~/Code/elastic/elasticsearch $ git grep 'new CounterMetric()' | grep -i time | grep -i -E '(nanos|millis)' | grep -iv test/
server/src/main/java/org/elasticsearch/index/engine/Engine.java:        private final CounterMetric throttleTimeMillisMetric = new CounterMetric();
server/src/main/java/org/elasticsearch/ingest/IngestMetric.java:    private final CounterMetric ingestTimeInNanos = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric snapshotRateLimitingTimeInNanos = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric restoreRateLimitingTimeInNanos = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric uploadTimeInMillis = new CounterMetric();
server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreSnapshotMetrics.java:    private final CounterMetric uploadReadTimeInNanos = new CounterMetric();
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsStatsAction.java:        CounterMetric ingestTimeInMillis = new CounterMetric();

Is there any overriding reason to be unique here, or does it make more sense to just do what we usually do?

@joegallo
Copy link
Contributor

I'd be fine with +1ing the type changes by themselves, but in order to change the logic on the time math, I'm going to have to start by understanding the reason this fence was here in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants