Skip to content

[New Term] Add persist_new_terms option to survive restarts - #1770

Open
MarcellZamboHu wants to merge 5 commits into
jertel:masterfrom
MarcellZamboHu:feat/new-term-persistence
Open

[New Term] Add persist_new_terms option to survive restarts#1770
MarcellZamboHu wants to merge 5 commits into
jertel:masterfrom
MarcellZamboHu:feat/new-term-persistence

Conversation

@MarcellZamboHu

@MarcellZamboHu MarcellZamboHu commented Jul 15, 2026

Copy link
Copy Markdown

Description

Adds an opt-in persist_new_terms option to the new_term rule type, making it restart-safe.

Problem: NewTermsRule keeps its learned terms (seen_values) only in memory. On startup the baseline is rebuilt from a terms aggregation over the last terms_window_size (default 30 days), so after a restart the rule re-alerts on terms that:

  • were last seen (and alerted on) longer ago than terms_window_size, or
  • whose source documents have since been deleted by index lifecycle/retention policies.

Solution: When persist_new_terms: true is set, every newly seen term is also written into the already-existing (and previously unused) <writeback_index>_past index:

  • one small document per term, under the non-indexed match_body field of the existing past_elastalert mapping — no new index, no mapping growth;
  • a deterministic document id (sha1(rule_name|field|value)) makes writes idempotent;
  • at startup the persisted terms are merged into the baseline built by get_all_terms();
  • composite fields round-trip correctly (tuple ↔ JSON list);
  • persistence is fail-safe by design: if the _past index 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 _past index is missing (installations created before it was added to elastalert-create-index), a warning explains how to create it.

The feature code is ~60 lines in ruletypes.py plus one schema line; the rest is tests and docs.

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • I have updated the documentation.
  • I have updated the changelog.

Testing

Besides the unit tests (option off by default, missing _past index, 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 single new_term rule with persist_new_terms: true on a field user:

  1. Index docs user=alice|bob|eve → all three are new terms, alert once each, and appear in <writeback>_past.
  2. Delete the eve documents from the source index (simulating retention/lifecycle deletion so get_all_terms can no longer rediscover it), then restart ElastAlert. Startup log: Loaded 3 persisted termseve is restored from _past even though it is gone from the source index.
  3. Index user=eve again → no alert (correctly suppressed by the persisted baseline). Index a genuinely new user=malloryalerts (proving the rule still fires). The writeback index ends with eve alerted 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 trips ElasticSearchClient.search's eql/esql format_request (they assume query.bool.filter is a dict, not a list) and raised AttributeError at 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 _past index.

🤖 Generated with Claude Code

MarcellZamboHu and others added 3 commits July 15, 2026 12:26
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>
@jertel

jertel commented Jul 15, 2026

Copy link
Copy Markdown
Owner

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.

@MarcellZamboHu

MarcellZamboHu commented Jul 15, 2026

Copy link
Copy Markdown
Author

"I have manually tested all relevant modes of the change in this PR." in progress still
my question is: the idea is correct, can you aprove?

…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>
@jertel

jertel commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Generally, the idea appears correct. The primary concern is the use of the elastalert_past index. I think the index was originally intended to be used to store a list of all previously sent alerts. That's not implemented, as far as I know. Repurposing the index to store persisted in-memory data seems reasonable. The issue is that your implementation assumes that the index will be exclusively used for NewTerms rule types. However, other rule types also could benefit from persisting in-memory caches to disk, in the event someone else wants to support a similar persistence for another rule type. So you will need a way to efficiently query just the data relating to NewTerms persisted data to avoid pulling back unrelated data in the future.

@MarcellZamboHu

Copy link
Copy Markdown
Author

" I have manually tested all relevant modes of the change in this PR." Done.

@MarcellZamboHu

Copy link
Copy Markdown
Author

Generally, the idea appears correct. The primary concern is the use of the elastalert_past index. I think the index was originally intended to be used to store a list of all previously sent alerts. That's not implemented, as far as I know. Repurposing the index to store persisted in-memory data seems reasonable. The issue is that your implementation assumes that the index will be exclusively used for NewTerms rule types. However, other rule types also could benefit from persisting in-memory caches to disk, in the event someone else wants to support a similar persistence for another rule type. So you will need a way to efficiently query just the data relating to NewTerms persisted data to avoid pulling back unrelated data in the future.

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.

PUT elastalert_past
{
  "mappings": {
    "properties": {
      "persistence_type": { "type": "keyword" },
      "rule_name":        { "type": "keyword" },
      "term_hash":        { "type": "keyword" },
      "first_seen":       { "type": "date" },
      "expires_at":       { "type": "date" }
    }
  }
}

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>
@MarcellZamboHu

MarcellZamboHu commented Jul 15, 2026

Copy link
Copy Markdown
Author

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:

  • Every persisted document now carries persistence_type: "new_terms" (keyword) alongside rule_name, and the document _id is namespaced with it: new_terms:<sha1>.
  • Loads are scoped to persistence_type + rule_name in filter context, so documents belonging to a future rule type are never pulled back.
  • The past_elastalert mapping gains the persistence_type keyword field (both the ES 7 and ES 8 variants). I extended the existing mapping rather than replacing it, so match_body/aggregate_id stay as they are and existing installs aren't affected.

Deviations from what I proposed:

  1. constant_score instead of a top-level bool filter. A {query: {bool: {filter: [...]}}} body never reaches ES: ElasticSearchClient.search runs every body through eql.format_request / esql.format_request, and both assume query.bool.filter is a dict, so a list raises AttributeError and silently disables term loading. I hit exactly this in live testing. {query: {constant_score: {filter: {bool: {must: [...]}}}}} keeps filter context (no scoring) and both formatters correctly ignore it. A regression test now pins the query shape against both formatters.

  2. The raw term value is stored, not just a hash. Restoring the baseline needs the actual value — add_data compares raw values against seen_values, and a hash can't be reversed — so match_body keeps {field, value} and the hash is used where it pays off: the _id. I also left rule_name out of the _id (it goes into the hashed input instead), because _id is capped at 512 bytes while rule names are unbounded.

  3. No expires_at / TTL. It came from the mapping I sketched in my previous comment, not from anything you asked for. It is orthogonal to the scoping concern you raised and would widen this PR, so I dropped it. Happy to follow up separately if you'd like persisted state to age out via ILM.

Live verification of your specific concern: with three new_terms documents plus one hand-injected {"persistence_type": "frequency"} document sharing the same rule_name in _past, startup logs Loaded 3 persisted terms — the unrelated document is not pulled back. Feeding that document's value in as an event still alerts, confirming it never leaked into the NewTerms baseline, and the foreign document is left untouched in the index.

@jertel

jertel commented Jul 15, 2026

Copy link
Copy Markdown
Owner

How will existing installations handle this new persistence_type field mapping? My understanding is that existing environments will not receive the mapping, even if create_index.py is executed (which is what happens on a restart of the official Docker container). Therefore, what is the behavior for those that see the release notes and opt in to try this new feature? Will they end up with an index with an ambiguously typed field? Or will ES conveniently default to make this field a keyword type? I'd like to get these details ironed out before merging this.

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 expires_at is a separate concern from the one you (presuming me, jertel, since your comment is a reply to mine) yet I never mentioned expires_at nor a TTL in this PR conversation.

@MarcellZamboHu

Copy link
Copy Markdown
Author

You're right on both counts. I measured it, and it turns out to be worse than an ambiguously typed field.

create_index.py returns early when the base index already exists (L36-L39), before any put_mapping runs, so an existing install never receives the updated past_elastalert mapping, including on a restart of the official container.

Tested against ES 8.13.4, with a _past index created from the mapping currently on master and a document written exactly as persist_term() writes it: the field comes out as text with a .keyword subfield, assigned dynamically. On a fresh install with this PR's mapping it is keyword. So ES does not default to keyword here, because past_elastalert has no dynamic: strict and no dynamic template.

The current value happens to survive that. The standard analyzer emits new_terms as a single token, so the term query still matches on a legacy index, but that is a property of the analyzer rather than something the code should rely on. A future persistence type such as New-Terms analyzes to [new, terms] and the same query returns 0 hits, with nothing in the log to tell the operator. On the legacy index I get 1 hit for new_terms and 0 for New-Terms; on a fresh index both return 1.

It also cannot be repaired after the fact. Once the field exists as text, PUT _mapping fails with mapper [persistence_type] cannot be changed from type [text] to [keyword], and the only way out is a reindex. The same PUT _mapping issued before the first persisted write succeeds and yields a proper keyword, so whichever route we take, the mapping has to be in place before anything is written.

Three ways forward, and I'd rather you pick than have me guess:

  1. Bootstrap from the rule. When persist_new_terms is enabled, get_persist_index() issues an idempotent PUT <writeback_index>_past/_mapping carrying only the persistence_type keyword property, before anything is written. If that fails, whether from insufficient permissions or from a field already mapped as text by an earlier opt-in, it logs and disables persistence for that rule, which is the fail-safe path that function already implements. No operator action needed, same behaviour on ES 7 and 8.

  2. Make create_index.py idempotent: create missing sub-indices and re-apply mappings instead of returning early. That covers every future mapping change rather than just this one, but it changes the behaviour of a tool operators run on every container restart, so I'd rather do it as its own PR than fold it into this one.

  3. A separate index for persisted state. It avoids the legacy mapping entirely, since the index would be new everywhere, but it walks back your earlier point that repurposing _past is reasonable, so I'm listing it only for completeness.

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. expires_at came from my own earlier comment, where I sketched a mapping that included it, not from anything you asked for. My wording made it read as though the TTL had been your suggestion. I've edited that comment to make the origin clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants