[New Term] Add persist_new_terms option to survive restarts - #1770
[New Term] Add persist_new_terms option to survive restarts#1770MarcellZamboHu wants to merge 5 commits into
Conversation
Newly seen terms are written into the existing <writeback_index>_past index with deterministic ids and merged back into the baseline at startup, so terms alerted on longer ago than terms_window_size (or whose source documents were deleted) do not re-alert after a restart. Opt-in; persistence failures never interrupt matching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the contribution. What is the reason for not performing live testing against an actual cluster? All PRs must have this completed. Unit tests can validate a lot but without actually running this code in a live environment you don't have full confidence that it will work properly in both the disabled and enabled (initial + restart) scenarios. |
|
"I have manually tested all relevant modes of the change in this PR." in progress still |
…wrapper The ElasticSearchClient.search wrapper runs every query body through eql.format_request/esql.format_request, which assume query.bool.filter is a dict. A list-valued filter raised AttributeError at startup, disabling term loading. Use a plain term query instead. Found via live ES testing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Generally, the idea appears correct. The primary concern is the use of the |
|
" I have manually tested all relevant modes of the change in this PR." Done. |
Good point — I'll add a persistence_type keyword field (e.g. "new_terms_cache") plus rule_name to every document, and scope all NewTerms reads/writes with a filtered term query on both fields. Document _id will follow {persistence_type}:{rule_name}:{term_hash} so future rule types can reuse the index without collisions, and lookups stay cheap either via exact GET or a filter-context bool query. Happy to add an index mapping/template as part of this PR to enforce the schema going forward. |
The _past index may later hold persisted state for other rule types, so
tag every document with persistence_type ('new_terms'), namespace the
document id with it, and scope loads to persistence_type + rule_name.
The scoped query uses constant_score rather than a top-level bool: the
eql/esql request formatters in the ES client assume query.bool.filter is
a dict, so a bool there raises AttributeError. A regression test pins the
query shape against both formatters.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Implemented in the latest commits. Summary of what landed, plus three deliberate deviations from the schema I sketched above — each for a reason I verified rather than assumed. What's in:
Deviations from what I proposed:
Live verification of your specific concern: with three |
|
How will existing installations handle this new Separately, it appears you are pasting conversation dumps from Claude, which are only partially relevant to this PR's conversation. Please be mindful that doing so will cause confusion for other community members of this project. Ex: #3 mentions |
|
You're right on both counts. I measured it, and it turns out to be worse than an ambiguously typed field.
Tested against ES 8.13.4, with a The current value happens to survive that. The standard analyzer emits It also cannot be repaired after the fact. Once the field exists as Three ways forward, and I'd rather you pick than have me guess:
I'd go with the first option here, and the second separately if you want the general fix. On the Claude dumps: fair, and I've stopped. What follows is written by me. One correction on the example you cited. |
Description
Adds an opt-in
persist_new_termsoption to thenew_termrule type, making it restart-safe.Problem:
NewTermsRulekeeps its learned terms (seen_values) only in memory. On startup the baseline is rebuilt from a terms aggregation over the lastterms_window_size(default 30 days), so after a restart the rule re-alerts on terms that:terms_window_size, orSolution: When
persist_new_terms: trueis set, every newly seen term is also written into the already-existing (and previously unused)<writeback_index>_pastindex:match_bodyfield of the existingpast_elastalertmapping — no new index, no mapping growth;sha1(rule_name|field|value)) makes writes idempotent;get_all_terms();_pastindex does not exist or any ES call fails, a warning is logged and the rule keeps working from its in-memory baseline. Matching is never interrupted.No behavior changes without the new option. If the
_pastindex is missing (installations created before it was added toelastalert-create-index), a warning explains how to create it.The feature code is ~60 lines in
ruletypes.pyplus one schema line; the rest is tests and docs.Checklist
make test-dockerwith my changes.Testing
Besides the unit tests (option off by default, missing
_pastindex, persisting new terms with deterministic ids, restoring/merging at startup, composite-field round-trip, ES failure tolerance), I ran an end-to-end test against a live Elasticsearch 8 in Docker, using this repo's Dockerfile image, a singlenew_termrule withpersist_new_terms: trueon a fielduser:user=alice|bob|eve→ all three are new terms, alert once each, and appear in<writeback>_past.evedocuments from the source index (simulating retention/lifecycle deletion soget_all_termscan no longer rediscover it), then restart ElastAlert. Startup log:Loaded 3 persisted terms—eveis restored from_pasteven though it is gone from the source index.user=eveagain → no alert (correctly suppressed by the persisted baseline). Index a genuinely newuser=mallory→ alerts (proving the rule still fires). The writeback index ends withevealerted exactly once, i.e. no post-restart duplicate.Live testing surfaced one real bug that the mocked unit tests could not: the original persisted-terms query used a
{bool: {filter: [...]}}shape, which tripsElasticSearchClient.search'seql/esqlformat_request(they assumequery.bool.filteris a dict, not a list) and raisedAttributeErrorat startup, silently disabling term loading. Fixed by using a plain{term: {...}}query (last commit).Comments
Happy to adjust anything — naming, defaults, or moving the storage elsewhere if you'd prefer not to reuse the
_pastindex.🤖 Generated with Claude Code