Update alert documents when the write index changes #110788

mgiota · 2021-09-01T12:15:52Z

📝 Summary

The proposed solution in the above ticket to remove require_alias: true didn't work, because the mappings wouldn't be installed correctly. This is because of the current logic where resources are installed only when the bulk operation fails https://github.com/elastic/kibana/blob/master/x-pack/plugins/rule_registry/server/rule_data_client/rule_data_client.ts#L139. If the flag is missing and we would like to index some new data, resources wouldn't be installed and alerts table wouldn't render any data.

So the fix was to:

keep the require_alias: true
and disable it only when updating a document

await ruleDataClient.getWriter().bulk({
      body: allEventsToIndex.flatMap(({ event, indexName }) => [
        indexName
          ? { index: { _id: event[ALERT_UUID]!, _index: indexName, require_alias: false } }
          : { index: { _id: event[ALERT_UUID]! } },
        event,
      ]),
    });

How to test

For both scenarios here's the command you can use in Dev tools to get the alerts that are being indexed

GET .alerts-observability*/_search
{
  "fields": [
    "*"
  ],
  "_source": false,
  "sort": [
    {
      "kibana.alert.start": {
        "order": "desc" 
      }
    }
  ]
}

Scenario 1 (an ongoing alert gets written in old index after rollover)

Create a new rule and generate some data that should trigger an alert
Verify one new alert is written in the correct index .internal.alerts-observability.logs.alerts-default-000001
Wait for the next trigger of the alert and verify that alert is updated and no new alert is created
In Devtools do a rollover POST .alerts-observability.logs.alerts-default/_rollover
Verify you can see two indices GET .alerts-observability.logs.alerts-default
Wait for the alert to trigger again
Verify the new alert is still written in old new index .internal.alerts-observability.logs.alerts-default-000001

Scenario 2 (an ongoing alert SHOULD be written in the old index after rollover and after a new rule type was created)

Create a new rule and generate some data that should trigger an alert
Verify one new alert is written in the correct index .internal.alerts-observability.logs.alerts-default-000001
Wait for the next trigger of the alert and verify that alert is updated and no new alert is created
In Devtools do a rollover POST .alerts-observability.logs.alerts-default/_rollover
Verify you can see two indices GET .alerts-observability.logs.alerts-default
Create a new rule and wait for the new alert to trigger
Verify the new alert is written in the new index .internal.alerts-observability.logs.alerts-default-000002
Verify the old alert is written in the old index .internal.alerts-observability.logs.alerts-default-000001 => ~~We just spotted a bug 🐞 🐛 , where ILM policy deleted the old indices after rollover and the old alert got indexed in .internal.alerts-observability.logs.alerts-default-000002 instead~~ [RAC] Alert ILM policy shouldn't delete old indices after rollover #111029

x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts

kibanamachine · 2021-09-02T10:50:06Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 626cdf4
Storybooks not built yet
This comment will update when the build is complete

History

💔 Build #150357 failed e10635e8d22cad4606c3c0b67825964cc2b2457b
💔 Build #150348 failed 195e63c57b8d1d61b24bede406463633fd5b4799
💔 Build #150339 failed 8263d96ab0429e3840d5982095de48bff979ceb8
💔 Build #150161 failed 09af843

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-09-02T10:50:07Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 626cdf4
Storybooks not built yet
This comment will update when the build is complete

History

💔 Build #150357 failed e10635e8d22cad4606c3c0b67825964cc2b2457b
💔 Build #150348 failed 195e63c57b8d1d61b24bede406463633fd5b4799
💔 Build #150339 failed 8263d96ab0429e3840d5982095de48bff979ceb8
💔 Build #150161 failed 09af843

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-09-02T10:50:08Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 626cdf4
Storybooks not built yet
This comment will update when the build is complete

History

💔 Build #150357 failed e10635e8d22cad4606c3c0b67825964cc2b2457b
💔 Build #150348 failed 195e63c57b8d1d61b24bede406463633fd5b4799
💔 Build #150339 failed 8263d96ab0429e3840d5982095de48bff979ceb8
💔 Build #150161 failed 09af843

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-09-02T10:50:09Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 626cdf4
Storybooks not built yet
This comment will update when the build is complete

History

💔 Build #150357 failed e10635e8d22cad4606c3c0b67825964cc2b2457b
💔 Build #150348 failed 195e63c57b8d1d61b24bede406463633fd5b4799
💔 Build #150339 failed 8263d96ab0429e3840d5982095de48bff979ceb8
💔 Build #150161 failed 09af843

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-09-02T10:50:09Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 626cdf4
Storybooks not built yet
This comment will update when the build is complete

History

💔 Build #150357 failed e10635e8d22cad4606c3c0b67825964cc2b2457b
💔 Build #150348 failed 195e63c57b8d1d61b24bede406463633fd5b4799
💔 Build #150339 failed 8263d96ab0429e3840d5982095de48bff979ceb8
💔 Build #150161 failed 09af843

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

elasticmachine · 2021-09-02T12:07:33Z

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

kibanamachine · 2021-09-02T13:37:05Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: b42dd5f
Storybooks Preview
Flaky suites:
- xpack-kibana-ciGroup12

Metrics [docs]

✅ unchanged

History

💚 Build #150498 succeeded e9bdb37
💔 Build #150491 failed 626cdf4
💔 Build #150357 failed e10635e8d22cad4606c3c0b67825964cc2b2457b
💔 Build #150348 failed 195e63c57b8d1d61b24bede406463633fd5b4799
💔 Build #150339 failed 8263d96ab0429e3840d5982095de48bff979ceb8

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mgiota

mgiota · 2021-09-02T14:05:55Z

I tested above two scenarios and here are the findings:

Scenario 1 ✅
I took a video and as you can see after rollover the existing alert gets written in the old index as expected.
https://user-images.githubusercontent.com/2852703/131854460-e4aa01fb-0aa2-4e19-879c-ffdca4ae677b.mov.

Scenario 2 🐞
When I created a 2nd rule the 2nd alert was written in the correct index .internal.alerts-observability.logs.alerts-default-000002

But the old alert got written in the new index as well .internal.alerts-observability.logs.alerts-default-000002, where it shouldn't.

This is a new bug that we spotted and it is because ILM policy deleted the old indices after rollover.

UPDATE
Above scenario is not always reproducible. It turns out the ILM policy deletes old indices, but ES might not evaluate the policy immediately, which can make testing a bit harder

weltenwort · 2021-09-02T17:16:16Z

Isn't the whole point of this bugfix to ensure that it's being written to the old index again? Otherwise the duplication I described in the issue would occur, which we are trying to avoid. Maybe I'm misunderstanding. The video and the screenshots. Which is happening when?

mgiota · 2021-09-02T17:34:39Z

@weltenwort I updated my comment above as per our discussion. I hope it is more clear now for other reviewers. And yes the expected behavior is for old alerts to keep being written in the old index again after a rollover.

mgiota · 2021-09-02T17:41:08Z

A bug I spotted is the reason field (same for threshold and actual value) are blank when the alert recovers

weltenwort · 2021-09-02T17:47:40Z

Hm, seems like we have some edge cases to smooth out. I'll take a look asap.

weltenwort · 2021-09-02T19:14:38Z

So after some collaborative investigation we realized this fix uncovered a different problem related to the ILM policy associated with the alerting indices by default. We'll track and resolve that separately. 😌

mgiota · 2021-09-02T19:45:13Z

Here's the new ticket #111029. It should also solve the issue with the empty reason field for recovered alerts I pasted above

Kerry350 · 2021-09-03T12:07:14Z

Functionality looks good 👍

However, just to throw a spanner in the works, I didn't see this behaviour:

We just spotted a bug 🐞 🐛 , where ILM policy deleted the old indices after rollover

In both scenario 1 and 2 I didn't have my old index deleted.

Checking the code now.

Test results

Scenario 1

Create a new rule and generate some data that should trigger an alert

Verify one new alert is written in the correct index .internal.alerts-observability.logs.alerts-default-000001

Wait for the next trigger of the alert and verify that alert is updated and no new alert is created

In Devtools do a rollover POST .alerts-observability.logs.alerts-default/_rollover
Verify you can see two indices GET .alerts-observability.logs.alerts-default (I used GET _cat/indices/.alerts-observability.logs.alerts-default but same idea)

Wait for the alert to trigger again
Verify the new alert is still written in old new index .internal.alerts-observability.logs.alerts-default-000001

Scenario 2

Create a new rule and generate some data that should trigger an alert

Verify one new alert is written in the correct index .internal.alerts-observability.logs.alerts-default-000001

Wait for the next trigger of the alert and verify that alert is updated and no new alert is created

In Devtools do a rollover POST .alerts-observability.logs.alerts-default/_rollover
Verify you can see two indices GET .alerts-observability.logs.alerts-default

Create a new rule and wait for the new alert to trigger

Verify the new alert is written in the new index .internal.alerts-observability.logs.alerts-default-000002
Verify the old alert is written in the old index .internal.alerts-observability.logs.alerts-default-000001

(Timestamps show the updates continuing after rollover without deleting the old index)

mgiota · 2021-09-03T12:13:02Z

@Kerry350 I was also not able to reproduce the bug in Scenario 2. It looks like unconditionally ILM deleted the indices for me when I was testing. I will update the description.

Thanks for checking it out so thoroughly

mgiota · 2021-09-03T12:19:52Z

@Kerry350 I will enable automerge, since I will be off for a couple of hours.

Kerry350 · 2021-09-03T12:21:46Z

@mgiota

I will enable automerge, since I will be off for a couple of hours.

👍

I'll keep an eye too in case anything goes wrong.

Kerry350

LGTM 👍

* first draft(work in progress) * add back missing await * disable require_alias flag only when we update * cleanup

Kerry350 · 2021-09-03T12:45:29Z

I approved before noticing the auto-backport label was missing. I've created the backports manually.

* first draft(work in progress) * add back missing await * disable require_alias flag only when we update * cleanup

weltenwort · 2021-09-03T12:53:18Z

Thanks for the review! I think auto-backport also works retroactively.

banderror

LGTM 👍

banderror · 2021-09-03T12:55:22Z

x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts

+        indexName
+          ? { index: { _id: event[ALERT_UUID]!, _index: indexName, require_alias: false } }
+          : { index: { _id: event[ALERT_UUID]! } },


This looks good to me, I'd just like to ask if you thought about using create + update instead of index and decided to keep index for both operations. It looks like this executor collects the full set of document fields, so it's safer to use index (create or replace the whole doc).

Yes, we considered update but figured using index gives tighter control over the full document content (e.g. allows for removal of fields). We might refactor to not fetch the full content and use update in the future, but it looked like too much of a change for a late 7.15.0 bug-fix.

* first draft(work in progress) * add back missing await * disable require_alias flag only when we update * cleanup Co-authored-by: mgiota <giota85@gmail.com>

kibanamachine · 2021-09-07T12:41:51Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

* first draft(work in progress) * add back missing await * disable require_alias flag only when we update * cleanup Co-authored-by: mgiota <giota85@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

first draft(work in progress)

796373a

mgiota marked this pull request as draft September 1, 2021 12:15

add back missing await

09af843

mgiota commented Sep 1, 2021

View reviewed changes

x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts Show resolved Hide resolved

mgiota commented Sep 1, 2021

View reviewed changes

x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts Outdated Show resolved Hide resolved

disable require_alias flag only when we update

626cdf4

mgiota force-pushed the 110519_update_documents branch from e10635e to 626cdf4 Compare September 2, 2021 10:36

cleanup

e9bdb37

resolve conflicts with master

b42dd5f

mgiota marked this pull request as ready for review September 2, 2021 12:07

mgiota self-assigned this Sep 2, 2021

mgiota mentioned this pull request Sep 2, 2021

[RAC] Alert ILM policy shouldn't delete old indices after rollover #111029

Closed

mgiota mentioned this pull request Sep 2, 2021

[RAC][Meta] Consolidate the two indexing implementations in rule_registry plugin #101016

Open

41 tasks

Kerry350 self-requested a review September 3, 2021 09:12

mgiota enabled auto-merge (squash) September 3, 2021 12:20

mgiota merged commit e2ee263 into elastic:master Sep 3, 2021

Kerry350 approved these changes Sep 3, 2021

View reviewed changes

Kerry350 pushed a commit to Kerry350/kibana that referenced this pull request Sep 3, 2021

Update alert documents when the write index changes (elastic#110788)

ef1b6a4

* first draft(work in progress) * add back missing await * disable require_alias flag only when we update * cleanup

Kerry350 mentioned this pull request Sep 3, 2021

[7.x] Update alert documents when the write index changes (#110788) #111122

Merged

Kerry350 pushed a commit to Kerry350/kibana that referenced this pull request Sep 3, 2021

Update alert documents when the write index changes (elastic#110788)

6710456

* first draft(work in progress) * add back missing await * disable require_alias flag only when we update * cleanup

Kerry350 mentioned this pull request Sep 3, 2021

[7.15] Update alert documents when the write index changes (#110788) #111123

Merged

banderror reviewed Sep 3, 2021

View reviewed changes

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 7, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 7, 2021

mgiota deleted the 110519_update_documents branch January 4, 2022 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update alert documents when the write index changes #110788

Update alert documents when the write index changes #110788

mgiota commented Sep 1, 2021 •

edited

Loading

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

elasticmachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

mgiota commented Sep 2, 2021 •

edited

Loading

weltenwort commented Sep 2, 2021 •

edited

Loading

mgiota commented Sep 2, 2021 •

edited

Loading

mgiota commented Sep 2, 2021

weltenwort commented Sep 2, 2021

weltenwort commented Sep 2, 2021

mgiota commented Sep 2, 2021

Kerry350 commented Sep 3, 2021 •

edited

Loading

mgiota commented Sep 3, 2021 •

edited

Loading

mgiota commented Sep 3, 2021

Kerry350 commented Sep 3, 2021

Kerry350 left a comment

Kerry350 commented Sep 3, 2021

weltenwort commented Sep 3, 2021 •

edited

Loading

banderror left a comment

banderror Sep 3, 2021

weltenwort Sep 3, 2021

kibanamachine commented Sep 7, 2021

Update alert documents when the write index changes #110788

Update alert documents when the write index changes #110788

Conversation

mgiota commented Sep 1, 2021 • edited Loading

📝 Summary

How to test

kibanamachine commented Sep 2, 2021

⏳ Build in-progress, with failures

History

kibanamachine commented Sep 2, 2021

⏳ Build in-progress, with failures

History

kibanamachine commented Sep 2, 2021

⏳ Build in-progress, with failures

History

kibanamachine commented Sep 2, 2021

⏳ Build in-progress, with failures

History

kibanamachine commented Sep 2, 2021

⏳ Build in-progress, with failures

History

elasticmachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

💛 Build succeeded, but was flaky

Metrics [docs]

History

mgiota commented Sep 2, 2021 • edited Loading

weltenwort commented Sep 2, 2021 • edited Loading

mgiota commented Sep 2, 2021 • edited Loading

mgiota commented Sep 2, 2021

weltenwort commented Sep 2, 2021

weltenwort commented Sep 2, 2021

mgiota commented Sep 2, 2021

Kerry350 commented Sep 3, 2021 • edited Loading

Test results

mgiota commented Sep 3, 2021 • edited Loading

mgiota commented Sep 3, 2021

Kerry350 commented Sep 3, 2021

Kerry350 left a comment

Choose a reason for hiding this comment

Kerry350 commented Sep 3, 2021

weltenwort commented Sep 3, 2021 • edited Loading

banderror left a comment

Choose a reason for hiding this comment

banderror Sep 3, 2021

Choose a reason for hiding this comment

weltenwort Sep 3, 2021

Choose a reason for hiding this comment

kibanamachine commented Sep 7, 2021

mgiota commented Sep 1, 2021 •

edited

Loading

mgiota commented Sep 2, 2021 •

edited

Loading

weltenwort commented Sep 2, 2021 •

edited

Loading

mgiota commented Sep 2, 2021 •

edited

Loading

Kerry350 commented Sep 3, 2021 •

edited

Loading

mgiota commented Sep 3, 2021 •

edited

Loading

weltenwort commented Sep 3, 2021 •

edited

Loading