Optimize some per-document hot paths in the geoip processor #120824
Conversation
This saves us the expensive time to retrieve and set the nanoseconds part of the Instant.
joegallo added >enhancement :Data Management/Ingest Node 
| Pinging @elastic/es-data-management (Team:Data Management) |
| Hi @joegallo, I've created a changelog YAML for you. |
| | ||
| public boolean isCloseToExpiration() { | ||
| return Instant.ofEpochMilli(lastCheck).isBefore(Instant.now().minus(25, ChronoUnit.DAYS)); | ||
| final Instant now = Instant.ofEpochMilli(System.currentTimeMillis()); // millisecond precision is sufficient (and faster) |
There was a problem hiding this comment.
I think we could pass in a ThreadPool, and use ThreadPool::absoluteTimeInMillis, which would be significantly faster since it's cached. Same goes in the other place below.
There was a problem hiding this comment.
That makes sense. I'll see if I can make that happen.
There was a problem hiding this comment.
I can't make it pass tests! I'm going to open a new ticket to track it.
| // to show up in a flame graph. | ||
| final long valid; | ||
| if (settings.hasValue("ingest.geoip.database_validity")) { | ||
| valid = settings.getAsTime("ingest.geoip.database_validity", THIRTY_DAYS).millis(); |
There was a problem hiding this comment.
Is this slow only when there is no value (b/c we generate an exception or something)? Or is it always slow? If the latter, should we calculate this outside of this method, and have a settings listener listening for updates to the setting?
There was a problem hiding this comment.
I added another comment here in the code -- the setting itself is kindof a hack to get around the situation where we want to be able to test this code but we can't make it possible to actually change the value of the setting in 'the real world'.
I'm not at all opposed to continuing to grind on this, but I'd love to do that in a different PR.
There was a problem hiding this comment.
If the latter, should we calculate this outside of this method, and have a settings listener listening for updates to the setting?
I took a closer look at the idea of the settings listener, and I think the reason we're not doing any of that right now is specifically because the setting in question isn't a 'real setting' -- AFAICT all the settings update listener type methods require a proper Setting object to hang off of, which we don't have in this case. 😬
Somebody could probably get pretty far by extracting our current code and manually watching for settings changes like this:
+ clusterService.addListener(event -> { + if (event.metadataChanged() && event.state().metadata().settings() != event.previousState().metadata().settings()) { + logger.info("metadata changed and settings changed"); + [... process the settings update here, check if the database validity has changed, etc ...] + } + }); 
| I need to snag a profile, but a6a4135 is a very nice change here. (edit: but I reverted it -- I'd rather we write a better cluster state listener than hack that change in the way I did it in that commit) |
Make it a little more obvious in the test failure message that GeoLite2-ASN.mmdb isn't particularly special.
This reverts commit a6a4135.
| I kicked some changes out of this PR, and I'm punting some of what I would still like to see done onto #121208. We can come back to this in the future and do even better. |
💚 Backport successful
|
3 participants
Each commit is a different optimization, and they all matter (just a little). Here's the flamegraph before (with areas highlighted):
And after: