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

[ResponseOps][Alerts] Migrate alerts fetching to TanStack Query #186978

Merged

Conversation

umbopepato
Copy link
Member

@umbopepato umbopepato commented Jun 26, 2024

Summary

Implements a new useSearchAlertsQuery hook based on TanStack Query to replace the useFetchAlerts hook, following this organizational logic.

This PR focuses mainly on the fetching logic itself, leaving the surrounding API surface mostly unchanged since it will be likely addressed in subsequent PRs.

To verify

  1. Create rules that fire alerts in different solutions
  2. Check that the alerts table usages work correctly ({O11y, Security, Stack} alerts and rule details pages, ...)
    1. Check that the alerts displayed in the table are coherent with the solution, KQL query, time filter, pagination
    2. Check that pagination changes are reflected in the table
    3. Check that changing the query when in pages > 0 resets the pagination to the first page

Closes point 1 of #186448
Should fix #171738

Checklist

@umbopepato
Copy link
Member Author

/ci

1 similar comment
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato changed the title [ResponseOps][Alerts] Migrate alerts fetching to tanstack query [ResponseOps][Alerts] Migrate alerts fetching to TanStack Query Jul 5, 2024
@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from 40a6699 to 6b5cd3e Compare July 5, 2024 17:35
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from 6b5cd3e to 4941851 Compare July 8, 2024 13:17
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from 4941851 to bfb0008 Compare July 10, 2024 08:05
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from bfb0008 to d3637e8 Compare July 10, 2024 10:43
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from d3637e8 to a407466 Compare July 10, 2024 10:44
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from a407466 to 785ac43 Compare July 10, 2024 13:57
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 labels Jul 10, 2024
@umbopepato umbopepato marked this pull request as ready for review July 10, 2024 14:04
@umbopepato umbopepato requested review from a team as code owners July 10, 2024 14:04
@elasticmachine
Copy link
Contributor

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

@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from 785ac43 to 1ca881e Compare July 10, 2024 14:46
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Jul 10, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@umbopepato
Copy link
Member Author

Thanks for fixing the loader!

When I click on refresh, if the data is the same as before, the time Updated message does not change, is it the same for you? image

Regarding pagination, I still see the issue, I will check to see if I can find a way to reproduce it.

Ok so it looks like the AlertsSearchBar only updates the query state on refresh. That triggers an actual refetch only if at least one of the parameters changes, which happens with sliding window time filters such as Last 24H. Ranges such as Today have fixed start and end dates (i.e. 59:59.999Z), which never cause the fetch alerts query key to change and thus a new request to start. This seems to be happening on main as well.

As for the Updated ... ago indicator, that's currently only connected to the bulk actions state so it doesn't reflect the timing of the alerts fetching (completing the migration to react-query is useful to show a better indication here based on all the queries used in the table).

The good news is this PR is useful to improve the "force refetch" functionality since we'll have tools like query's refetch and queryClient.invalidateQueries() so once this is merged we can work on exposing a more solid refresh API (perhaps through a new AlertsContext or an imperative handle on the AlertsTable).

cc @cnasikas

Copy link
Contributor

@JiaweiWu JiaweiWu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for addressing my feedback.

@maryam-saeidi
Copy link
Member

maryam-saeidi commented Jul 23, 2024

Ok so it looks like the AlertsSearchBar only updates the query state on refresh. That triggers an actual refetch only if at least one of the parameters changes, which happens with sliding window time filters such as Last 24H. Ranges such as Today have fixed start and end dates (i.e. 59:59.999Z), which never cause the fetch alerts query key to change and thus a new request to start. This seems to be happening on main as well.

I checked in edge-oblt, and you are right; when we have absolute time, the refresh button does not do anything, not sure if it is expected or not 🤔 (I will check how it works in other places and will create an issue if this needs to be fixed.)

As for the Updated ... ago indicator, that's currently only connected to the bulk actions state so it doesn't reflect the timing of the alerts fetching (completing the migration to react-query is useful to show a better indication here based on all the queries used in the table).

Is my understanding correct that this will be fixed when the bulk action is migrated to react-query? Does it make sense to hide it if it does not show the correct information, or will it be fixed before the v8.16 release?

The good news is this PR is useful to improve the "force refetch" functionality since we'll have tools like query's refetch and queryClient.invalidateQueries() so once this is merged we can work on exposing a more solid refresh API (perhaps through a new AlertsContext or an imperative handle on the AlertsTable).

That would be awesome!

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Update: Umbo and I checked the pagination issue, and it might be a feature :D I will double-check it with the Eui team.

Regarding Updated ... ago, Umbo has it in his TODO list, and it sometimes does not work if the number of alerts does not change.

So overall, it LGTM! Thanks Umbo for doing this 🙌🏻

Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

Look great on behalf on @elastic/security-threat-hunting-investigations. I have desk tested below scenarios both on Alerts and Rule Details page. I have also asked a small question below regarding the failed test.

- Row Action
	- Update Alert Status
	- Add to Timeline.
- Bulk Actions
	- Update Alert Status
- Field Browser
	- Add Columns
	- Remove Columns
- Refresh Button
- Filter/Query
	- Adding a filter
	- Adding a KQL Query
	- Adding a Lucene Query
- Building block Filters.
- Inspect 
- Run Time fields

Warning

I encountered and error once on the Rule Details Page but my mistake I navigated away and did not take the screenshot. Additionally I cannot reproduce it again but I will keep an eye.

@umbopepato umbopepato force-pushed the migrate-alerts-fetching-to-tanstack-query branch from 4362489 to 3d6857c Compare July 25, 2024 12:01
@umbopepato umbopepato enabled auto-merge (squash) July 25, 2024 12:08
@kibana-ci
Copy link
Collaborator

kibana-ci commented Jul 25, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 223 228 +5
observability 1059 1064 +5
securitySolution 5611 5616 +5
triggersActionsUi 802 806 +4
total +19

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/alerting-types 191 199 +8
@kbn/alerts-ui-shared 284 293 +9
ruleRegistry 249 250 +1
total +18

Async chunks

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

id before after diff
alerting 91.7KB 91.7KB +11.0B
infra 1.5MB 1.5MB +14.0B
observability 420.6KB 420.6KB +65.0B
securitySolution 20.4MB 20.4MB +11.0B
slo 878.4KB 878.4KB +14.0B
triggersActionsUi 1.7MB 1.6MB -3.4KB
total -3.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/alerts-ui-shared 8 9 +1
ruleRegistry 14 11 -3
triggersActionsUi 52 51 -1
total -3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerting 25.5KB 25.5KB -2.0B
observability 101.8KB 101.9KB +58.0B
triggersActionsUi 120.3KB 122.3KB +2.0KB
total +2.1KB
Unknown metric groups

API count

id before after diff
@kbn/alerting-types 194 202 +8
@kbn/alerts-ui-shared 300 310 +10
ruleRegistry 286 287 +1
total +19

History

  • 💛 Build #223157 was flaky 4362489b0e3092ac4fbfe85b4e6a8e2d2b43fbeb
  • 💔 Build #217956 failed 40a6699c5438aeec2438fdb107821bba976820b5
  • 💔 Build #217894 failed d1f9f8e9c518c6f0f5dd316eef769c1a1d10a689

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

@umbopepato umbopepato merged commit bd3032b into elastic:main Jul 25, 2024
43 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Response Ops] Alerts table issues