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

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

Closed
4 tasks done
Tracked by #124138
janmonschke opened this issue Jan 24, 2024 · 2 comments
Closed
4 tasks done
Tracked by #124138
Assignees
Labels
Team:Threat Hunting:Investigations Security Solution Investigations Team technical debt Improvement of the software architecture and operational architecture
Milestone

Comments

@janmonschke
Copy link
Contributor

janmonschke commented Jan 24, 2024

The timeline redux store is the only redux store in Kibana that uses redux-observable. In this issue we are laying out the reasoning for removing the package in favor of redux-thunk and redux middlewares.

redux-observable is obsolete

The logic in timeline's epic can be implemented with redux-thunk and regular redux middlewares. There is no need for observables to be part of the epic(s). Observables are currently used to extract slices of the timeline state, which in the background is using reselect. The selectors can be used as-is in middlewares.

Further, the use of observables, lead to the use of global state (myEpicTimelineId) which reduces confidence in making changes to the epic. Changes in that global state have an impact on how the persistence queue works. A queue that is also not necessary anymore, now that auto-save in timeline has been removed.

Debugging experience

Observables are hard to debug and they are not integrated into the redux dev tools. Thunks and middlewares are easier to debug as they don't have "hidden" internal state. As they are just regular functions, they can be interrupted and inspected like any pice of JS code.

Redux toolkit

redux-observable is not compatible with @reduxjs/toolkit which is the de-facto standard way of using redux these days. Most applications in Kibana are already using the redux toolkit. Removing redux-observable will bring us one step closer to adding redux toolkit to the timeline store.

On top of this, some of the redux methods we're using are deprecated and have been replaced by methods in redux toolkit (e.g. create_store vs configureStore)

Package size

Package Size (minified)
redux-observable 24kb
redux-thunk 0.35kb

The comparison is a little lacking, since it includes the sizes for redux-observable's dependencies (rxjs and tslib). Given, we're using both dependencies in other places in Kibana, the actual savings in terms of package size might be marginal (<1kB).

Tasks

  1. Team:Threat Hunting:Investigations backport:skip release_note:skip technical debt v8.13.0
    janmonschke
  2. Team:Threat Hunting:Investigations backport:skip refactoring release_note:skip technical debt v8.13.0
    janmonschke
  3. Team:Threat Hunting:Investigations backport:skip release_note:skip technical debt v8.13.0
    janmonschke
  4. Team:Threat Hunting:Investigations backport:prev-minor refactoring release_note:skip technical debt v8.13.0 v8.14.0
    janmonschke
@janmonschke janmonschke added technical debt Improvement of the software architecture and operational architecture Team:Threat Hunting:Investigations Security Solution Investigations Team labels Jan 24, 2024
@janmonschke janmonschke self-assigned this Jan 24, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

@janmonschke janmonschke changed the title [Tech Debt][Security Solution][Investigations] Replace redux-observable with redux-thunk [Tech Debt][Security Solution][Investigations] Replace redux-observable with redux-thunk and middlewares Jan 24, 2024
@PhilippeOberti
Copy link
Contributor

PhilippeOberti commented Jan 24, 2024

thanks for the great description! This is going to be an awesome and welcome cleanup! ❤️

janmonschke added a commit that referenced this issue Jan 29, 2024
…orage and change epics (#175450)

## Summary

As a first step in removing `redux-observable` (see
#175427), this PR transforms all
localstorage epics and the timeline change epic into regular redux
middlewares.

### Checklist

Delete any items that are not applicable to this PR.

- [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 a commit that referenced this issue Jan 31, 2024
)

## 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](#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](9629e66)
|
[after](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>
janmonschke added a commit that referenced this issue Feb 2, 2024
## 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
fkanout pushed a commit to fkanout/kibana that referenced this issue Feb 7, 2024
)

## 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
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
…orage and change epics (elastic#175450)

## Summary

As a first step in removing `redux-observable` (see
elastic#175427), this PR transforms all
localstorage epics and the timeline change epic into regular redux
middlewares.

### Checklist

Delete any items that are not applicable to this PR.

- [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
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
…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>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
)

## 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
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
…orage and change epics (elastic#175450)

## Summary

As a first step in removing `redux-observable` (see
elastic#175427), this PR transforms all
localstorage epics and the timeline change epic into regular redux
middlewares.

### Checklist

Delete any items that are not applicable to this PR.

- [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
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
…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>
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
)

## 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 a commit that referenced this issue Mar 6, 2024
## Summary

In the previous work for
#175427, we replaced
redux-observable with plain redux middlewares. The code that was based
on redux-observable wasn't tested, so as part of the refactoring we're
now adding tests to all timeline middlewares in this PR.


### 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
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Mar 6, 2024
## Summary

In the previous work for
elastic#175427, we replaced
redux-observable with plain redux middlewares. The code that was based
on redux-observable wasn't tested, so as part of the refactoring we're
now adding tests to all timeline middlewares in this PR.

### 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

(cherry picked from commit 8f39000)
kibanamachine added a commit that referenced this issue Mar 6, 2024
…78118)

# Backport

This will backport the following commits from `main` to `8.13`:
- [[SecuritySolution] Add timeline middleware tests
(#178009)](#178009)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Jan
Monschke","email":"jan.monschke@elastic.co"},"sourceCommit":{"committedDate":"2024-03-06T13:55:26Z","message":"[SecuritySolution]
Add timeline middleware tests (#178009)\n\n## Summary\r\n\r\nIn the
previous work for\r\nhttps://github.com//issues/175427, we
replaced\r\nredux-observable with plain redux middlewares. The code that
was based\r\non redux-observable wasn't tested, so as part of the
refactoring we're\r\nnow adding tests to all timeline middlewares in
this PR.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"8f390002702accb40539d3a97ea0f0d43fdedcd5","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["refactoring","technical
debt","release_note:skip","Team:Threat
Hunting:Investigations","backport:prev-minor","v8.14.0"],"title":"[SecuritySolution]
Add timeline middleware
tests","number":178009,"url":"https://github.com/elastic/kibana/pull/178009","mergeCommit":{"message":"[SecuritySolution]
Add timeline middleware tests (#178009)\n\n## Summary\r\n\r\nIn the
previous work for\r\nhttps://github.com//issues/175427, we
replaced\r\nredux-observable with plain redux middlewares. The code that
was based\r\non redux-observable wasn't tested, so as part of the
refactoring we're\r\nnow adding tests to all timeline middlewares in
this PR.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"8f390002702accb40539d3a97ea0f0d43fdedcd5"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/178009","number":178009,"mergeCommit":{"message":"[SecuritySolution]
Add timeline middleware tests (#178009)\n\n## Summary\r\n\r\nIn the
previous work for\r\nhttps://github.com//issues/175427, we
replaced\r\nredux-observable with plain redux middlewares. The code that
was based\r\non redux-observable wasn't tested, so as part of the
refactoring we're\r\nnow adding tests to all timeline middlewares in
this PR.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"8f390002702accb40539d3a97ea0f0d43fdedcd5"}}]}]
BACKPORT-->

Co-authored-by: Jan Monschke <jan.monschke@elastic.co>
@michaelolo24 michaelolo24 added this to the 8.14 milestone Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Threat Hunting:Investigations Security Solution Investigations Team technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

4 participants