Skip to content
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

Change .alerts to use datastream #154266

Closed
kobelb opened this issue Apr 3, 2023 · 9 comments · Fixed by #160572
Closed

Change .alerts to use datastream #154266

kobelb opened this issue Apr 3, 2023 · 9 comments · Fixed by #160572
Assignees
Labels
Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Project:Serverless MVP Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@kobelb
Copy link
Contributor

kobelb commented Apr 3, 2023

When initially implementing the alerts-as-data indices, we decided to not use a datastream out of fear that datastreams were not the right option for mutable data. As a result, we ended up implementing something that was very datastream-esque, but not exactly datastreams. With Stateful Elasticsearch, this approach doesn't have any issues.

However, Serverless Elasticsearch will be replacing index lifecycle management with datastream lifecycle management, and we will no longer be allowed to create the .alerts-ilm-policy which targets the not a real datastream alerts-as-data indices.

As a result, we will need to evaluate the feasibility of switching alerts to using a datastream. There is migrate to data stream API that we can use to do so. If we come across any unexpected complexities, we should discuss with the @elastic/es-data-management team.

The one unknown that I currently have is how we'll approach rollbacks with this approach. I don't know whether we won't be able to rollback to an old version of Kibana once we start creating a datastream.

@kobelb kobelb added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework labels Apr 3, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@mikecote
Copy link
Contributor

mikecote commented Apr 3, 2023

@pmuellr probably the same for event log?

@pmuellr
Copy link
Member

pmuellr commented Apr 3, 2023

probably the same for event log?

Yessir! I thought for sure there was an issue open to do this for the event log - IIRC, datastreams were brand new when event log was created, and we were a little scared to use it :-). But was always in the back of my mind to do this. Didn't find an issue though!

@pmuellr pmuellr self-assigned this Apr 4, 2023
@mikecote
Copy link
Contributor

mikecote commented Apr 4, 2023

I thought for sure there was an issue open to do this for the event log

Found it! #62677. Heh, references 7.8.0 while we are now 8.8.0..

@simianhacker
Copy link
Member

How are you going to deal with "updates" since data-streams are "append-only"? Right now the Rule Registry (AAD) updates the alerts after each evaluation.

@dakrone
Copy link
Member

dakrone commented May 31, 2023

How are you going to deal with "updates" since data-streams are "append-only"? Right now the Rule Registry (AAD) updates the alerts after each evaluation.

You can still perform updates directly to a backing index for a data stream (the concrete index is returned in both the indexing and search operation), the data stream itself doesn't prohibit the operation. I assume this is the way that updates are done right now with the alias-based approach?

pmuellr added a commit to pmuellr/kibana that referenced this issue Jun 5, 2023
@pmuellr
Copy link
Member

pmuellr commented Jun 12, 2023

You can still perform updates directly to a backing index for a data stream (the concrete index is returned in both the indexing and search operation), the data stream itself doesn't prohibit the operation. I assume this is the way that updates are done right now with the alias-based approach?

Correct, that's what we're doing today.

@oatkiller
Copy link
Contributor

@kobelb Question: will alerts-as-data be switched to data streams? And will that change happen on both serverless and ESS? Thank you!

@kobelb
Copy link
Contributor Author

kobelb commented Jul 20, 2023

@kobelb Question: will alerts-as-data be switched to data streams?

Yes

And will that change happen on both serverless and ESS? Thank you!

We will only be making this change for Serverless, as making these changes elsewhere would break rollbacks.

ymao1 added a commit that referenced this issue Aug 10, 2023
Reverting #163414 and
#163487

## Summary

@pmuellr uncovered a bug in ES with `ignore_malformed` and datastreams
while working on #154266

> Found what I hope is an ES bug yesterday w/data streams (DS). It
doesn’t like ignore_malformed on the @timestamp field
:slightly_smiling_face:. I think this is a bug since [the doc says
(https://www.elastic.co/guide/en/elasticsearch/reference/current/ignore-malformed.html#ignore-malformed-setting)
Mapping types that don’t support the setting will ignore it if set on
the index level.
I think it’s understandable - the @timestamp field is a key field for DS
(can be overridden) - so you’d not be surprised it’s treated specially.
But … why not just ignore it in that case, like the other mapping types
that are ignored.
I tried overriding ignore_malformed for just that field, and it
complained that I couldn’t use that option on that field! hahahahah
So, we’d be left having to add ignore_malformed to every mapped field in
our mappings, except for @timestamp.
For the time being, I’ve removed all the ignore_malformed stuff in my
AaD DS PR, when using DS, but left it when using alias/index.
Unless someone knows more about this special ignored_malformed /
@timestamp field / data-stream relationship, I’ll boil down a simple
test case and open an issue for ES.

In order to avoid having even more divergent code between serverless &
serverful, we will revert this change until we can confirm a bug with ES
and hopefully get a fix in.
pmuellr added a commit to pmuellr/kibana that referenced this issue Aug 16, 2023
resolves elastic#154266

Changes the way the alerts-as-data (AAD) indices are created and
written to, to allow them to be built as they have been in the past
(alias and backing indices created manually) OR as an ES Data Stream.

Serverless will use Data Streams, other environments will use the
existing alias and backing indices.  The determination is made by
optionally including the `serverless` plugin, and determining if it's
available.
@mikecote mikecote moved this from In Progress to In Review in AppEx: ResponseOps - Execution & Connectors Aug 29, 2023
pmuellr added a commit that referenced this issue Aug 30, 2023
resolves #154266

Changes the way the alerts-as-data (AAD) indices are created and written
to, to allow them to be built as they have been in the past (alias and
backing indices created manually) OR as an ES Data Stream.

Serverless will use Data Streams, other environments will use the
existing alias and backing indices. The determination is made by
optionally including the `serverless` plugin, and determining if it's
available.

The implementation is organized around a `DataStreamAdapter` object,
which is instantiated with a "data stream" or "alias" flavor, and then
it handles the specialized behavior. Currently, a lot of the smaller
implementation bits, like setting property values in ES calls, is done
via in-line boolean checks of that object, as to whether data streams or
aliases are being used. This could probably be cleaned up some.

Existing non-serverless function tests are largely unchanged, as they
can't test the new data stream path. Some tests have been added to the
serverless function tests, to test basic reading / writing via updated
alert documents.

## DEFER

- more serverless AaD tests

- #158403 - this issue is more
noticeable now that we HAVE to do OCC with data streams, so we get
errors instead of simply overwriting documents (which is also bad)

Co-authored-by: Patryk Kopycinski <[email protected]>
eokoneyo pushed a commit to eokoneyo/kibana that referenced this issue Aug 31, 2023
…#160572)

resolves elastic#154266

Changes the way the alerts-as-data (AAD) indices are created and written
to, to allow them to be built as they have been in the past (alias and
backing indices created manually) OR as an ES Data Stream.

Serverless will use Data Streams, other environments will use the
existing alias and backing indices. The determination is made by
optionally including the `serverless` plugin, and determining if it's
available.

The implementation is organized around a `DataStreamAdapter` object,
which is instantiated with a "data stream" or "alias" flavor, and then
it handles the specialized behavior. Currently, a lot of the smaller
implementation bits, like setting property values in ES calls, is done
via in-line boolean checks of that object, as to whether data streams or
aliases are being used. This could probably be cleaned up some.

Existing non-serverless function tests are largely unchanged, as they
can't test the new data stream path. Some tests have been added to the
serverless function tests, to test basic reading / writing via updated
alert documents.

## DEFER

- more serverless AaD tests

- elastic#158403 - this issue is more
noticeable now that we HAVE to do OCC with data streams, so we get
errors instead of simply overwriting documents (which is also bad)

Co-authored-by: Patryk Kopycinski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Project:Serverless MVP Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
No open projects
8 participants