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

[Security Solution] Fix timeline dynamic batching #204034

Merged
merged 25 commits into from
Jan 7, 2025

Conversation

logeekal
Copy link
Contributor

@logeekal logeekal commented Dec 12, 2024

Summary

Handles :

Issue with Batches

  • [Security Solution] Timeline - adding new column does not fetch its values after next batch has been fetched #201405
    • Timeline had a bug where if users fetched multiple batches and then if user adds a new column, the value of this new columns will only be fetched for the latest batch and not old batches.
    • This PR fixes that ✅ by cumulatively fetching the data for old batches till current batch iff a new column has been added.
    • For example, if user has already fetched the 3rd batch, data for 1st,2nd and 3rd will be fetched together when a column has been added, otherwise, data will be fetched incrementally.

Note

To fully understand this use case, please look at this test case.

Before After
new_column_new_batch_timeline.mp4
fix_pagination_new_columns.mp4

Issue with Elastic search limit

  • Elastic search has a limit of 10K hits at max but we throw error at 10K which should be allowed.
    • Error should be thrown at anything >10K. 10001 for example.
    • ✅ This PR fixes that just for timeline by allowing 10K hits.
Before After
image image

Removal of obsolete code

Below files related to old Timeline code are removed as well:

  • x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.tsx

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

@logeekal
Copy link
Contributor Author

/ci

@logeekal logeekal marked this pull request as ready for review December 16, 2024 11:20
@logeekal logeekal requested review from a team as code owners December 16, 2024 11:20
@logeekal logeekal marked this pull request as draft December 16, 2024 11:20
@logeekal logeekal added Team:Threat Hunting:Investigations Security Solution Investigations Team backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.16.3 labels Dec 16, 2024
@logeekal logeekal marked this pull request as ready for review December 16, 2024 11:21
@elasticmachine
Copy link
Contributor

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

@logeekal logeekal force-pushed the fix/timeline_dynamic_batching branch from 76f5dd6 to c60fc77 Compare December 17, 2024 08:34
@logeekal
Copy link
Contributor Author

logeekal commented Dec 20, 2024

Though as you can see in the video below, it seems that we fetch the last batch when removing a column, I'm not sure why we would do that? This is not happening in main

this is strange. 100% a bug and should not happen. but it took me time to replicate your scenario. If we remove column generally, the fetch does not happen but it happens if and only if when you have just added a column and you immediately remove a column as a next step )

We compare previous and current request to decide whether to file a request or not, if we remove a field it does not actually change the current request when compared previous one so it does not fire the request... mostly.
Only problem is that we have now switched the batching strategy from fetching all the batches (when a column was added. ) to fetching incremental batch ( when a new column is being removed ). Check this section.

This difference in pagination results in system firing a new request. This is one of those case which might be tricky to handle. I. thought i will fix it now but nothing good is coming to my mind so i will leave it to you for now.

Screen.Recording.2024-12-20.at.02.57.21.mov

@PhilippeOberti
Copy link
Contributor

this is strange. 100% a bug and should not happen. but it took me time to replicate your scenario. If we remove column generally, the fetch does not happen but it happens if and only if when you have just added a column and you immediately remove a column as a next step )

We compare previous and current request to decide whether to file a request or not, if we remove a field it does not actually change the current request when compared previous one so it does not fire the request... mostly. Only problem is that we have now switched the batching strategy from fetching all the batches (when a column was added. ) to fetching incremental batch ( when a new column is being removed ). Check this section.

This difference in pagination results in system firing a new request. This is one of those case which might be tricky to handle. I. thought i will fix it now but nothing good is coming to my mind so i will leave it to you for now.

Ok I dug through the code a bit and I understand what's happening. I agree with you, it's a little bit tricky and I also do not see an easy way to get around this at this time.
I think because it's such an edge case (it only happens if you remove a column (and the newPagination value goes back to 500) right after having added one (where the newPagination value was changed to a higher number like 1000, 1500...`)) I think this solution is ok.
It is definitely better than what we have now.

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the solution isn't 100% ideal (see comment), it is a lot better than what we have now.

Getting things to work perfectly would probably require a heavier refactor, or at least some more complex logic, and I'm not convinced that the higher complexity would outweigh the small downside of this implementation.

LGTM, great job @logeekal!

@PhilippeOberti PhilippeOberti enabled auto-merge (squash) January 6, 2025 20:02
@elastic-vault-github-plugin-prod elastic-vault-github-plugin-prod bot requested a review from a team as a code owner January 6, 2025 20:16
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages/kbn-babel-preset/styled_components_files.js LGTM

@PhilippeOberti PhilippeOberti merged commit 088169f into elastic:main Jan 7, 2025
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.17, 8.x

https://github.com/elastic/kibana/actions/runs/12646300422

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #21 / Stateful Observability - Deployment-agnostic API integration tests SyntheticsAPITests getSyntheticsMonitors get many monitors without params

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 22.1MB 22.1MB +742.0B

History

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.16 Backport failed because of merge conflicts
8.17 Backport failed because of merge conflicts
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 204034

Questions ?

Please refer to the Backport tool documentation

logeekal added a commit to logeekal/kibana that referenced this pull request Jan 7, 2025
## Summary

Handles :

### Issue with Batches
- elastic#201405
- Timeline had a bug where if users fetched multiple batches and then if
user adds a new column, the value of this new columns will only be
fetched for the latest batch and not old batches.
- This PR fixes that ✅ by cumulatively fetching the data for old batches
till current batch `iff a new column has been added`.
- For example, if user has already fetched the 3rd batch, data for
1st,2nd and 3rd will be fetched together when a column has been added,
otherwise, data will be fetched incrementally.

### Issue with Elastic search limit

- Elastic search has a limit of 10K hits at max but we throw error at
10K which should be allowed.
    - Error should be thrown at anything `>10K`. 10001 for example.
    - ✅  This PR fixes that just for timeline by allowing 10K hits.

### Removal of obsolete code

Below files related to old Timeline code are removed as well:
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.test.tsx
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.tsx

---------

Co-authored-by: Philippe Oberti <[email protected]>
(cherry picked from commit 088169f)

# Conflicts:
#	packages/kbn-babel-preset/styled_components_files.js
#	x-pack/plugins/security_solution/public/common/mock/mock_timeline_search_service.ts
#	x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.test.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/query/index.tsx
#	x-pack/solutions/security/plugins/security_solution/public/timelines/containers/index.test.tsx
@logeekal
Copy link
Contributor Author

logeekal commented Jan 7, 2025

💚 All backports created successfully

Status Branch Result
8.16

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@logeekal
Copy link
Contributor Author

logeekal commented Jan 7, 2025

💚 All backports created successfully

Status Branch Result
8.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

logeekal added a commit to logeekal/kibana that referenced this pull request Jan 7, 2025
## Summary

Handles :

### Issue with Batches
- elastic#201405
- Timeline had a bug where if users fetched multiple batches and then if
user adds a new column, the value of this new columns will only be
fetched for the latest batch and not old batches.
- This PR fixes that ✅ by cumulatively fetching the data for old batches
till current batch `iff a new column has been added`.
- For example, if user has already fetched the 3rd batch, data for
1st,2nd and 3rd will be fetched together when a column has been added,
otherwise, data will be fetched incrementally.

### Issue with Elastic search limit

- Elastic search has a limit of 10K hits at max but we throw error at
10K which should be allowed.
    - Error should be thrown at anything `>10K`. 10001 for example.
    - ✅  This PR fixes that just for timeline by allowing 10K hits.

### Removal of obsolete code

Below files related to old Timeline code are removed as well:
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.test.tsx
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.tsx

---------

Co-authored-by: Philippe Oberti <[email protected]>
(cherry picked from commit 088169f)

# Conflicts:
#	packages/kbn-babel-preset/styled_components_files.js
#	x-pack/plugins/security_solution/public/common/mock/mock_timeline_search_service.ts
#	x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.test.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/query/index.tsx
#	x-pack/solutions/security/plugins/security_solution/public/timelines/containers/index.test.tsx
@logeekal
Copy link
Contributor Author

logeekal commented Jan 7, 2025

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

logeekal added a commit to logeekal/kibana that referenced this pull request Jan 7, 2025
## Summary

Handles :

### Issue with Batches
- elastic#201405
- Timeline had a bug where if users fetched multiple batches and then if
user adds a new column, the value of this new columns will only be
fetched for the latest batch and not old batches.
- This PR fixes that ✅ by cumulatively fetching the data for old batches
till current batch `iff a new column has been added`.
- For example, if user has already fetched the 3rd batch, data for
1st,2nd and 3rd will be fetched together when a column has been added,
otherwise, data will be fetched incrementally.

### Issue with Elastic search limit

- Elastic search has a limit of 10K hits at max but we throw error at
10K which should be allowed.
    - Error should be thrown at anything `>10K`. 10001 for example.
    - ✅  This PR fixes that just for timeline by allowing 10K hits.

### Removal of obsolete code

Below files related to old Timeline code are removed as well:
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.test.tsx
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.tsx

---------

Co-authored-by: Philippe Oberti <[email protected]>
kowalczyk-krzysztof pushed a commit to kowalczyk-krzysztof/kibana that referenced this pull request Jan 7, 2025
## Summary

Handles :


### Issue with Batches
- elastic#201405
- Timeline had a bug where if users fetched multiple batches and then if
user adds a new column, the value of this new columns will only be
fetched for the latest batch and not old batches.
- This PR fixes that ✅ by cumulatively fetching the data for old batches
till current batch `iff a new column has been added`.
- For example, if user has already fetched the 3rd batch, data for
1st,2nd and 3rd will be fetched together when a column has been added,
otherwise, data will be fetched incrementally.

### Issue with Elastic search limit

- Elastic search has a limit of 10K hits at max but we throw error at
10K which should be allowed.
    - Error should be thrown at anything `>10K`. 10001 for example.
    - ✅  This PR fixes that just for timeline by allowing 10K hits.

### Removal of obsolete code

Below files related to old Timeline code are removed as well:
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.test.tsx
-
x-pack/plugins/security_solution/public/timelines/components/timeline/footer/index.tsx

---------

Co-authored-by: Philippe Oberti <[email protected]>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 8, 2025
@kibanamachine
Copy link
Contributor

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.

@kibanamachine
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport missing Added to PRs automatically when the are determined to be missing a backport. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:Threat Hunting:Investigations Security Solution Investigations Team v8.16.3 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants