[SecuritySolution] Remove remaining usage of `redux-observable` #175678

janmonschke · 2024-01-26T12:53:39Z

Summary

In this PR, we're removing all usages of redux-observable in favor of simple middlewares. This work is part of this tech debt ticket which outlines the motivation of this move.

A couple shortcuts had to be taken and I added comments further down to explain the motivation.

Oddities

Weirdly, the CI reports an increase in async chunks, instead of an expected decrease due to removing a library.

id	before	after	diff
`securitySolution`	11.2MB	11.4MB	+157.1KB

I'm not sure, why this is, so if anyone has any insights on how to examine the async chunks, that would be helpful.

edit: After hours of analyzing Kibana build stats at various stages of this PR, I found that the changes that are responsible for this increase in async chunk sum size is this commit: janmonschke@cde023d . It only consists of deleted files and deleted imports. Therefore it's up to webpack to figure out the best way to chunk up the app. In other words: I can't change it :(

Tests

You might notice, that I didn't add tests to the middlewares. That is because the epics didn't have tests either and their functionality is tested in acceptance tests. As a follow-up of this PR, I will add tests to all newly-introduced middlewares. My goal here is to keep the changes as small as possible.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

- remove `waitfor` - no need to actually render anything

- also removes all usage of `redux-observable`

lgestc · 2024-01-30T09:20:19Z

x-pack/plugins/security_solution/public/timelines/store/middlewares/timeline_save.ts

+            ? filters.map((myFilter: Filter) => {
+                const basicFilter = omit(['$state'], myFilter);
+                return {
+                  ...basicFilter,
+                  meta: {
+                    ...basicFilter.meta,
+                    field:
+                      (isMatchAllFilter(basicFilter) ||
+                        isPhraseFilter(basicFilter) ||
+                        isPhrasesFilter(basicFilter) ||
+                        isRangeFilter(basicFilter)) &&
+                      basicFilter.meta.field != null
+                        ? convertToString(basicFilter.meta.field)
+                        : null,
+                    value:
+                      basicFilter.meta.value != null
+                        ? convertToString(basicFilter.meta.value)
+                        : null,
+                    params:
+                      basicFilter.meta.params != null
+                        ? convertToString(basicFilter.meta.params)
+                        : null,
+                  },
+                  ...(isMatchAllFilter(basicFilter)
+                    ? {
+                        query: {
+                          match_all: convertToString(
+                            (basicFilter as MatchAllFilter).query.match_all
+                          ),
+                        },
+                      }
+                    : { match_all: null }),
+                  ...(isExistsFilter(basicFilter) && basicFilter.query.exists != null
+                    ? { query: { exists: convertToString(basicFilter.query.exists) } }
+                    : { exists: null }),
+                  ...((isQueryStringFilter(basicFilter) || get('query', basicFilter) != null) &&
+                  basicFilter.query != null
+                    ? { query: convertToString(basicFilter.query) }
+                    : { query: null }),
+                  ...(isRangeFilter(basicFilter) && basicFilter.query.range != null
+                    ? { query: { range: convertToString(basicFilter.query.range) } }
+                    : { range: null }),
+                  ...(isScriptedRangeFilter(basicFilter) &&
+                  basicFilter.query.script !=
+                    null /* TODO remove it when PR50713 is merged || esFilters.isPhraseFilter(basicFilter) */
+                    ? { query: { script: convertToString(basicFilter.query.script) } }
+                    : { script: null }),


any way we could improve this? it is very hard to read

I wish...I'm a little afraid to touch this one since I didn't write it and I'm not sure its tests cover all corner cases.

Yea, this is a nightmare. Each nested ternary should either be it's own function or at the very least have a description of what's going on. I wonder if there are better utilities for doing this now too since this code is relatively outdated... 🤔

maybe it would be a good idea to add a lot of tests THEN clean it up?

agree with that, in addition I would like to point out 2 things: 1) this code is pretty similar to the insight code that serializes markdown to filters, so much so that we should absolutely combine the two paths in someway in the future 2) there's a pretty clear bug to me here, in that we are not handling the isCombinedFilter case at all here, added to support nested filters, and so there is no way we are saving and restoring timelines with those types of filters correctly, afaik not a known issue right now.

actually it does work, since we are copying the entire $meta part of the filter over, and then just using whatever was stored in the saved object for combined filter types. not sure how much of this code is needed at all really then.

I think we could consider all of this outside of the scope of this PR. Like @janmonschke mentioned above, this code wasn't really modified, just moved around.
As much as I'd like to see this changes, it could be part of a broader effort: how to we want our timeline store to look like...

Yes agree completely it's out of scope, I just wanted to point those 2 things out, especially the not handling isCombinedFilter case.

lgestc

left one comment, thanks for the work here!

jennypavlova

Infra changes LGTM

weltenwort

infra changes LGTM too

michaelolo24 · 2024-01-30T14:17:40Z

x-pack/plugins/security_solution/public/timelines/store/middlewares/helpers.ts

+export function refreshTimelines(state: State) {
+  const allTimelineQuery = inputsSelectors.globalQueryByIdSelector()(state, ALL_TIMELINE_QUERY_ID);
+  if (allTimelineQuery.refetch != null) {
+    (allTimelineQuery.refetch as inputsModel.Refetch)();


Is this fn really stored in redux? I guess this is what @kqualters-elastic was talking about. I'm not sure if we have a ticket yet for removing non-serializable state from redux. Do you mind creating a ticket for refactoring the store to remove fn references and classes? Thanks!

sometimes it is, sometimes it's recreated from state/props in different places, it's quite a mess.

michaelolo24 · 2024-01-30T14:27:46Z

x-pack/plugins/security_solution/public/timelines/store/middlewares/timeline_favorite.ts

+          })
+        );
+      } catch (error) {
+        kibana.notifications.toasts.addDanger({


Do you think it would be better to put the notifications in the UI components instead? Then we wouldn't need to pass kibana$ at all. This may be a better idea though once we decide on how we want to refactor our queries with RTK

It depends. I like that this side-effect doesn't live in five different components. Having this non-consequential side-effect in a middleware made the most sense to me. Passing kibana isn't that much of a hassle, imo. Wdyt?

michaelolo24 · 2024-01-30T14:31:00Z

x-pack/plugins/security_solution/public/timelines/store/middlewares/timeline_save.ts

+        store.dispatch(
+          updateTimeline({
+            id: localTimelineId,
+            timeline: {


Thought, for a future reference. If we have the timeline shape stored in zod, we could just use zod.parse() on the response and spread that and not worry about missing a value or spreading something incorrectly. Thoughts?

Sounds good to me. We would just need to make sure to also update only-local timeline fields (such as isSaving)

michaelolo24

Thanks for making some awesome improvements here. Was able to test that pinning, notes, save, and save as new work as expected. Attempted to test a few iterations of the filters (kql query + filter + dataProvider ) and make sure the appropriate result was returned. Everything there worked as expected as well. Any other reviewers feel free to test any scenarios that I may have missed. Nice work @janmonschke !

janmonschke · 2024-01-31T07:08:42Z

@elasticmachine merge upstream

kibana-ci · 2024-01-31T08:22:24Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 511a05e

Failed CI Steps

FTR Configs #54

Test Failures

[job] [logs] FTR Configs #54 / console app console app with folded/unfolded lines in request body doesn't fail if a fold fails

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4957	4948	-9

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	11.2MB	11.4MB	+157.1KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	475	473	-2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	548	546	-2

History

💔 Build #190301 failed e39e887
💚 Build #189981 succeeded 477348a
💛 Build #189858 was flaky 6d99e67
💔 Build #189841 failed 543cff3
💔 Build #189835 failed d95a969

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

cc @janmonschke

## Summary After merging #175678, we have no more need for the kibana observable. It can now be replaced by an instance of kibana (aka `CoreStart`). This change mostly impacts tests that use `createStore` in `security_solution`. In a second step, this PR cleans up test-related imports between `security_solution` and the `timelines` plugin (523080c). This work is part of #175427. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

) ## Summary After merging elastic#175678, we have no more need for the kibana observable. It can now be replaced by an instance of kibana (aka `CoreStart`). This change mostly impacts tests that use `createStore` in `security_solution`. In a second step, this PR cleans up test-related imports between `security_solution` and the `timelines` plugin (elastic@523080c). This work is part of elastic#175427. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…tic#175678) ## Summary In this PR, we're removing all usages of `redux-observable` in favor of simple middlewares. This work is part of [this tech debt ticket](elastic#175427) which outlines the motivation of this move. A couple shortcuts had to be taken and I added comments further down to explain the motivation. ### Oddities Weirdly, the CI reports an increase in async chunks, instead of an expected decrease due to removing a library. | id | [before](elastic@9629e66) | [after](elastic@477348a) | diff | | --- | --- | --- | --- | | `securitySolution` | 11.2MB | 11.4MB | +157.1KB | I'm not sure, why this is, so if anyone has any insights on how to examine the async chunks, that would be helpful. _edit:_ After hours of analyzing Kibana build stats at various stages of this PR, I found that the changes that are responsible for this increase in async chunk sum size is this commit: janmonschke@cde023d . It only consists of deleted files and deleted imports. Therefore it's up to webpack to figure out the best way to chunk up the app. In other words: I can't change it :( ### Tests You might notice, that I didn't add tests to the middlewares. That is because the epics didn't have tests either and their functionality is tested in acceptance tests. As a follow-up of this PR, I will add tests to all newly-introduced middlewares. My goal here is to keep the changes as small as possible. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

) ## Summary After merging elastic#175678, we have no more need for the kibana observable. It can now be replaced by an instance of kibana (aka `CoreStart`). This change mostly impacts tests that use `createStore` in `security_solution`. In a second step, this PR cleans up test-related imports between `security_solution` and the `timelines` plugin (elastic@523080c). This work is part of elastic#175427. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…tic#175678) ## Summary In this PR, we're removing all usages of `redux-observable` in favor of simple middlewares. This work is part of [this tech debt ticket](elastic#175427) which outlines the motivation of this move. A couple shortcuts had to be taken and I added comments further down to explain the motivation. ### Oddities Weirdly, the CI reports an increase in async chunks, instead of an expected decrease due to removing a library. | id | [before](elastic@9629e66) | [after](elastic@477348a) | diff | | --- | --- | --- | --- | | `securitySolution` | 11.2MB | 11.4MB | +157.1KB | I'm not sure, why this is, so if anyone has any insights on how to examine the async chunks, that would be helpful. _edit:_ After hours of analyzing Kibana build stats at various stages of this PR, I found that the changes that are responsible for this increase in async chunk sum size is this commit: janmonschke@cde023d . It only consists of deleted files and deleted imports. Therefore it's up to webpack to figure out the best way to chunk up the app. In other words: I can't change it :( ### Tests You might notice, that I didn't add tests to the middlewares. That is because the epics didn't have tests either and their functionality is tested in acceptance tests. As a follow-up of this PR, I will add tests to all newly-introduced middlewares. My goal here is to keep the changes as small as possible. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

) ## Summary After merging elastic#175678, we have no more need for the kibana observable. It can now be replaced by an instance of kibana (aka `CoreStart`). This change mostly impacts tests that use `createStore` in `security_solution`. In a second step, this PR cleans up test-related imports between `security_solution` and the `timelines` plugin (elastic@523080c). This work is part of elastic#175427. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

janmonschke added 18 commits January 24, 2024 16:32

remove redux-observable usage in storage epics

ade7941

remove unused epic dependencies

f93d517

make sure to return the "redux" way

99fd4cc

debouncing is not necessary anymore

5d3bd7a

no need to use fake timers and waitFor anymore

b1f0dd8

no JSX in this file, so we're renaming it

4637677

simplify tests -> execute much faster now

b32aac9

- remove `waitfor` - no need to actually render anything

no JSX in this file, so we're renaming it

de6af05

Merge branch 'main' into security/remove-usage-of-redux-observables-1

95cc3db

add existence checks where necessary

38884ce

update test titles

76fa38c

replace timeline favorite epic with middleware

beb7fca

replace timeline note epic with middleware

42d0728

replace pinned event epic with middleware

1b1154f

remove unnecessary exports

4b83b2e

update comment

4ae9084

replace timeline save epic with middleware

9da4fd6

- also removes all usage of `redux-observable`

introduce refreshTimelines

2a7293c

janmonschke added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.13.0 labels Jan 26, 2024

janmonschke self-assigned this Jan 26, 2024

janmonschke added 7 commits January 26, 2024 14:53

the kibana observable has a different shape in the test env

097f961

use the correct timeline version

90ea8d5

reduce complexity

15fc538

remove double declarations

bd60d53

Merge branch 'main' into security/remove-usage-of-redux-observables-2

71d4dd2

skip notes part of the test

d95a969

comment out unneeded imports for now

543cff3

jennypavlova self-requested a review January 30, 2024 09:18

lgestc reviewed Jan 30, 2024

View reviewed changes

jennypavlova approved these changes Jan 30, 2024

View reviewed changes

weltenwort approved these changes Jan 30, 2024

View reviewed changes

janmonschke added buildkite-ci and removed buildkite-ci labels Jan 30, 2024

michaelolo24 reviewed Jan 30, 2024

View reviewed changes

michaelolo24 approved these changes Jan 30, 2024

View reviewed changes

Merge branch 'main' into security/remove-usage-of-redux-observables-2

511a05e

janmonschke enabled auto-merge (squash) January 31, 2024 07:27

janmonschke merged commit ca23dd5 into elastic:main Jan 31, 2024
37 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jan 31, 2024

This was referenced Jan 31, 2024

[SecuritySolution] Remove usage of the kibana observable #175962

Merged

[Tech Debt][Security Solution][Investigations] Replace redux-observable with redux-thunk and middlewares #175427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecuritySolution] Remove remaining usage of `redux-observable` #175678

[SecuritySolution] Remove remaining usage of `redux-observable` #175678

janmonschke commented Jan 26, 2024 •

edited

Loading

lgestc Jan 30, 2024

janmonschke Jan 30, 2024

michaelolo24 Jan 30, 2024

PhilippeOberti Jan 30, 2024

kqualters-elastic Jan 30, 2024

kqualters-elastic Jan 30, 2024

PhilippeOberti Jan 30, 2024

kqualters-elastic Jan 30, 2024

lgestc left a comment

jennypavlova left a comment

weltenwort left a comment

michaelolo24 Jan 30, 2024

kqualters-elastic Jan 30, 2024

michaelolo24 Jan 30, 2024 •

edited

Loading

janmonschke Jan 30, 2024

michaelolo24 Jan 30, 2024

janmonschke Jan 30, 2024

michaelolo24 left a comment

janmonschke commented Jan 31, 2024

kibana-ci commented Jan 31, 2024

ESLint disabled line counts

Total ESLint disabled count

[SecuritySolution] Remove remaining usage of redux-observable #175678

[SecuritySolution] Remove remaining usage of redux-observable #175678

Conversation

janmonschke commented Jan 26, 2024 • edited Loading

Summary

Oddities

Tests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgestc left a comment

Choose a reason for hiding this comment

jennypavlova left a comment

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 left a comment

Choose a reason for hiding this comment

janmonschke commented Jan 31, 2024

kibana-ci commented Jan 31, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

[SecuritySolution] Remove remaining usage of `redux-observable` #175678

[SecuritySolution] Remove remaining usage of `redux-observable` #175678

janmonschke commented Jan 26, 2024 •

edited

Loading

michaelolo24 Jan 30, 2024 •

edited

Loading