refactor: [M3-5181] - React Query for Events by bnussman-akamai · Pull Request #9949 · linode/manager

bnussman-akamai · 2023-12-01T05:14:49Z

Description 📝

React Query-ifys our events / events polling system
This PR's polling system should match the behavior of the existing polling system. The main changes revolve around the fact that we are now storing and retrieving data from the React Query cache.
This is take two of refactor: [M3-5181] - RQ-ify Events #9416 because we had to revert it fix: Revert React Query for events (#9416) #9444
- The big "bug" with that PR was that useEventsInfiniteQuery was mounted many times, which also mounted the cache updating code many times, which caused many renders to happen when event were polled. This PR fixed that by only mounting a polling hook that update the cache once.
- This PR takes some inspiration from refactor: [M3-5181] - RQ-ify Events #9416, but is different in some ways

Main Additions

`useEventsPoller` ⏲️

A hook mounted at the root of the app, which is responsible for polling the events endpoint for new events
This hook is responsible for
- Updating the cache based on the events it polls
- Showing toast notifications for global toasts (like volume_delete for example)
- Invoking our React Query event handlers (volumeEventsHandler for example)

`useInProgressEvents` 📇

A hook that returns only active / in-progress events
Used in LinodeRow for example

`useEventsInfiniteQuery` 📃

Used to render lists of events
This is used for EventsLanding, LinodeActivity, and the Event Menu

Other mentions

Removes rxjs dependency 🎉
Removes events completely from Redux 🗑️

How to test 🧪

Verify that all "real-time" feature of Cloud Manager work
- For example, rebooting a Linode, shutting down a Linode, resizing a Linode, ...
Test polling behavior compared to production
- Verify that this PR does not cause a significant change in the frequency in which we poll the events endpoint
Test the http://localhost:3000/events page
- Verify the infinite scrolling works as expected
- Verify the events are in the correct order
- Verify new events show up in realtime
Test the Linode Details events tab
- Verify the infinite scrolling works as expected
- Verify the events are in the correct order
- Verify new events show up in realtime
Test on a large account
Check for bad performance
Check the functionality of the Events / Notification Menu in the top navigation
Check logic and patterns in packages/manager/src/queries/events.ts and look for any issues, mistakes, or 🚩

As an Author I have considered 🤔

github-actions · 2023-12-14T15:17:23Z

Coverage Report: ✅
Base Coverage: 80.33%
Current Coverage: 81.15%

packages/manager/src/store/index.ts

hkhalil-akamai · 2024-01-11T19:20:47Z

packages/manager/src/queries/events/events.ts

+      : intervalMultiplier * INTERVAL,
+    resetEventsPolling: () => queryClient.setQueryData<number>(queryKey, 1),
+  };
+};


The tl;dr of this hook is that it globally keeps track of a single number (the polling interval) and exposes operations like resetting and incrementing.

It's a little bit convoluted (hacky even) using the RQ store to store this value globally (another alternative would be to use Context, or the package you mentioned). The reason I chose to do it this way was to keep the polling hook and the polling interval hook tightly coupled (e.g., resetting one would reset the other).

In the original incarnation of RQ Events this hook was necessary since multiple polling hooks were mounted and they all needed to be synchronized. Now that we only have one instance of the polling hook, it may no longer be necessary. It may be possible to have the single polling hook maintain its interval using useState.

hkhalil-akamai

Left some minor comments and suggestions, but overall this PR is looking really good. Tested with all the steps listed in the two predecessor PRs (Part 1 & Part 2) and looks good.

We should also test in a large account to make sure the performance issues from the previous incarnation are resolved.

hkhalil-akamai · 2024-01-12T00:12:44Z

packages/manager/package.json

    "redux": "^4.0.4",
    "redux-thunk": "^2.3.0",
    "reselect": "^4.0.0",
-    "rxjs": "^5.5.6",


Glad we can still remove this 🎉

Me too 😮‍💨

hkhalil-akamai · 2024-01-12T00:16:01Z

packages/manager/src/GoTo.tsx

Changes here aren't directly related to the ticket; should we move them to another PR?

Yeah, I probably should have done that. I chose to move some state around to reduce the number of renders caused by state changes in App.tsx. I think the change is pretty safe so I'm okay with leaving it if everyone else is

hkhalil-akamai · 2024-01-12T00:27:25Z

packages/manager/src/features/Events/EventsLanding.tsx

hkhalil-akamai · 2024-01-12T00:56:40Z

packages/manager/src/queries/linodes/events.ts

+}: EventHandlerData) => {
  const linodeId = event.entity?.id;

  // Early return to cut down the number of invalidations.


The early return on lines 26-28 cuts down on a lot of repeated invalidations but has the negative consequence that Linodes may not reflect their current status accurately. For example, if a Linode is rebooted from another tab, it will still show as "Running".

It's not a major issue but might be worth discussing/capturing in separate ticket.

Yeah, good point. Probably out of scope because it's Linode specific, but we definitely could improve the Linode event handlers in general. I'm making myself a note to revisit this later down the road.

hkhalil-akamai · 2024-01-12T01:03:40Z

packages/manager/src/features/Events/EventRow.tsx

 import { StyledGravatar } from './EventRow.styles';

-interface ExtendedEvent extends Event {
-  _deleted?: string;


Was this property unused?

Yes, it was unused. I'm not 100% sure but I think it was used way way in the past to determine if the event's entity was deleted for purposes of removing links that would lead to 404s in event messages. It would be good UX to bring this back somehow, but I can't think of a very efficient way to do it.

hkhalil-akamai · 2024-01-12T01:08:20Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeActivity/LinodeActivity.tsx

      data-testid="linode-events-table"
      emptyMessage="No recent activity for this Linode."
      entityId={id}
-      errorMessage="There was an error retrieving activity for this Linode."


Do we want to drop this error message?

I think I removed it because I chose to surface the API error directly for more transparency. Do you think we should stick with this more friendly error? I'm sure it's a rare occurrence

hkhalil-akamai · 2024-01-12T21:33:11Z

packages/manager/src/features/NotificationCenter/NotificationData/useFormattedNotifications.tsx

 ): NotificationItem => ({
  body: <RenderNotification notification={notification} onClose={onClose} />,
  countInTotal: shouldIncludeInCount,
+  eventId: -1,


Not sure if I fully understand why this was not needed before

hkhalil-akamai · 2024-01-12T21:54:18Z

packages/manager/src/hooks/useEventHandlers.ts

Not directly related to this PR, but I noticed we're missing event handlers for Kubernetes and now, VPC

Good call. I can make a ticket for LKE.

@dwiley-akamai Should we make one for VPC?

I'll create a ticket to investigate whether a handler is needed. My initial thought is due to the nature of the VPC events it probably isn't, but it warrants a closer look.

edit: created M3-7649

hkhalil-akamai

Re-approving since first approval didn't count

jaalah-akamai

Great work @bnussman 🎉

jdamore-linode

This is a huge achievement, thanks @bnussman-akamai! Especially appreciate the thorough docs and comments, as well as the new test coverage

packages/manager/src/queries/events/event.helpers.test.ts

Co-authored-by: jdamore-linode <97627410+jdamore-linode@users.noreply.github.com>

packages/manager/src/queries/events/events.ts

bnussman added 10 commits November 30, 2023 13:25

events landing

eeb1b48

add linode events support with filter

66ab52f

clean up

5a36fcd

add poller

1ea1623

add event handlers and cache updater

9406162

hook up toasts

f84beff

fix bug by removing early return and start refactoring

3b0b1b8

refactor event consumers

37b94b5

finish general refactor

f32ecad

optimize

802e6c3

bnussman-akamai added Work in Progress React Query Relating to the transition to use React Query labels Dec 1, 2023

bnussman-akamai self-assigned this Dec 1, 2023

bnussman-akamai requested a review from a team as a code owner December 1, 2023 05:14

bnussman-akamai requested review from abailly-akamai and hana-akamai and removed request for a team December 1, 2023 05:14

bnussman-akamai changed the title ~~refactor: React Query for Events~~ refactor: [M3-5181] - React Query for Events Dec 1, 2023

bnussman added 2 commits December 1, 2023 00:20

Merge branch 'develop' into react-query-events-poc

4c033dc

fixes and clean up

9a9637a

abailly-akamai added the Do Not Merge label Dec 1, 2023

bnussman added 7 commits December 1, 2023 11:35

fix cache updater bug

5dec126

Merge branch 'develop' into react-query-events-poc

69b9293

make the query updater update all queries

0a15104

poc: use cursor based pagination

9b4ed1b

Merge branch 'develop' into react-query-events-poc

d20da52

Merge branch 'develop' into react-query-events-poc

e2513db

remove old code

eaeba01

use id for cusour based pagination and clean up

10fb540

Merge branch 'develop' into react-query-events-poc

9ae89a6

hkhalil-akamai reviewed Jan 11, 2024

View reviewed changes

remove outdated comment

c003474

hkhalil-akamai approved these changes Jan 12, 2024

View reviewed changes

bnussman-akamai added the Needs extra approvals Use for PRs that have high-impact changes where > 2 approvals are needed label Jan 16, 2024

hkhalil-akamai approved these changes Jan 16, 2024

View reviewed changes

Merge branch 'develop' into react-query-events-poc

8562e49

jaalah-akamai approved these changes Jan 17, 2024

View reviewed changes

bnussman added 15 commits January 17, 2024 11:32

don't pass an empty object if we don't have a pageParam

7675ec5

improve interval defaults

7bf1c84

disable query that is for reading data only

e4a9efe

Merge branch 'develop' into react-query-events-poc

8306304

make polling backoff match existing backoff

ff3b051

clean up interval hooks

3197846

simplify polling interval

8d7436e

fixing duplicate pull due to rules of hooks

3e99b0f

Merge branch 'develop' into react-query-events-poc

ad58673

clean up new polling interval

c0e0c34

add changesets

527ab37

Merge branch 'develop' into react-query-events-poc

f4ad829

clean up mount timestamp

7a9fdbf

clean up query key for poller

9086b93

rename resetEventsPolling to checkForNewEvents

126f7b8

bnussman-akamai requested a review from jdamore-linode January 23, 2024 18:38

jdamore-linode approved these changes Jan 26, 2024

View reviewed changes

packages/manager/src/queries/events/event.helpers.test.ts Outdated Show resolved Hide resolved

packages/manager/src/queries/events/event.helpers.test.ts Outdated Show resolved Hide resolved

packages/manager/src/queries/events/event.helpers.test.ts Outdated Show resolved Hide resolved

Apply suggestions from @jdamore-linode

3c8886a

Co-authored-by: jdamore-linode <97627410+jdamore-linode@users.noreply.github.com>

bnussman-akamai commented Jan 26, 2024

View reviewed changes

packages/manager/src/queries/events/events.ts Outdated Show resolved Hide resolved

Update packages/manager/src/queries/events/events.ts

97b7455

bnussman-akamai merged commit e4911df into linode:develop Jan 26, 2024

cpathipa mentioned this pull request Feb 1, 2024

Release v1.111.0 - release → staging #10136

Merged

12 tasks

Conversation

bnussman-akamai commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Main Additions

useEventsPoller ⏲️

useInProgressEvents 📇

useEventsInfiniteQuery 📃

Other mentions

How to test 🧪

As an Author I have considered 🤔

Uh oh!

github-actions bot commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hkhalil-akamai Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwiley-akamai Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

jaalah-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

jdamore-linode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bnussman-akamai commented Dec 1, 2023 •

edited

Loading

`useEventsPoller` ⏲️

`useInProgressEvents` 📇

`useEventsInfiniteQuery` 📃

github-actions bot commented Dec 14, 2023 •

edited

Loading

hkhalil-akamai Jan 11, 2024 •

edited

Loading

dwiley-akamai Jan 17, 2024 •

edited

Loading