-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution] Fix timeline dynamic batching #204034
Conversation
/ci |
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
76f5dd6
to
c60fc77
Compare
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. 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 |
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. |
There was a problem hiding this 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!
There was a problem hiding this 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
Starting backport for target branches: 8.16, 8.17, 8.x |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
|
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
## 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
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
## 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
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
## 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]>
## 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]>
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. |
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. |
Summary
Handles :
Issue with Batches
iff a new column has been added
.Note
To fully understand this use case, please look at this test case.
new_column_new_batch_timeline.mp4
fix_pagination_new_columns.mp4
Issue with Elastic search limit
>10K
. 10001 for example.Removal of obsolete code
Below files related to old Timeline code are removed as well:
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.