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

ui: reduce frequent Metrics page re-rendering #88707

Merged

Conversation

koorosh
Copy link
Collaborator

@koorosh koorosh commented Sep 26, 2022

Before, many components on Metrics page relied on
nodesSummarySelector selector that in turn relies on NodeStatus that change constantly with every request (and we request it periodically every 10 seconds). NodeStatus include lots of unnecessary data for Metrics page (ie. node's and stores last metrics that aren't used on charts) but these changes forces react components to be re-rendered.

This patch refactors selectors that are used by metrics page in a way to provide only relevant subset of NodeStatus's info
to Graph components and reduce propagation of props passing from parent to child components. Instead, use selectors
in child components if necessary.

Release note: None

Resolves #65030

Video that shows how often components were re-rendered before this fix - regardless of timewindow, it always
updates every 10 seconds:

metrics_page_refreshes_before_fix.mov

and here's after fix. Components re-render every 10 second for 10 minutes interval and it is not re-rendered
for timewindows like 2 weeks or 1 month:

metrics_page_refreshes_after_fix.mov

@koorosh koorosh requested review from a team September 26, 2022 12:55
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator Author

@koorosh koorosh left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ui/workspaces/db-console/src/redux/nodes.ts line 403 at r1 (raw file):

 * connect to components on child pages.
 */
export const nodesSummarySelector = createSelector(

nodesSummarySelector selector caused most of "re-rendering" issues on Metrics page as far as it selects lots of data that changes periodically. Now, components on Metrics page doesn't rely on it at all, instead, components use specific selectors to fetch minimal subset
of data.

@koorosh koorosh force-pushed the ui-reduce-metrics-page-rerendering branch from 65f2895 to f79323b Compare September 26, 2022 13:18
Copy link
Contributor

@Santamaura Santamaura left a comment

Choose a reason for hiding this comment

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

I think in general :lgtm: aside from a couple of comments.

Reviewed 17 of 18 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @koorosh and @Santamaura)


pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 259 at r2 (raw file):

    // node is already selected, these per-node graphs should only display data
    // only for the selected node.
    const nodeIDs = nodeSources ? nodeSources : this.props.nodeIds;

nit: can destructure nodeIds at the top of the render


pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 411 at r2 (raw file):

 * collection.
 */
const nodeDropdownOptionsSelector = createSelector(

I'm assuming this selector is specific to this file. Does it make sense to move this to a selectors only file?

Copy link
Collaborator Author

@koorosh koorosh left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Santamaura)


pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 259 at r2 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

nit: can destructure nodeIds at the top of the render

Done.


pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 411 at r2 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

I'm assuming this selector is specific to this file. Does it make sense to move this to a selectors only file?

It's not reused anywhere so I prefer to keep it close to component.

@koorosh koorosh force-pushed the ui-reduce-metrics-page-rerendering branch from f79323b to 2d8e394 Compare September 29, 2022 12:15
@koorosh
Copy link
Collaborator Author

koorosh commented Sep 29, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 29, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 29, 2022

Build failed:

Before, many components on Metrics page relied on
`nodesSummarySelector` selector that in turn relies on `NodeStatus`
that change constantly with every request (and we request it periodically
every 10 seconds). `NodeStatus` include lots of unnecessary data for
Metrics page (ie. node's and stores last metrics that aren't used on charts)
but these changes forces react components to be re-rendered.

This patch refactors selectors that are used by metrics page in a way to
provide only relevant subset of NodeStatus's info to Graph components
and reduce propagation of props passing from parent to child components.
Instead, use selectors in child components if necessary.

Release note: None
@koorosh koorosh force-pushed the ui-reduce-metrics-page-rerendering branch from 2d8e394 to e333650 Compare September 30, 2022 13:41
@koorosh
Copy link
Collaborator Author

koorosh commented Sep 30, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 30, 2022

Build succeeded:

@craig craig bot merged commit aaca5ce into cockroachdb:master Sep 30, 2022
@blathers-crl
Copy link

blathers-crl bot commented Sep 30, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from e333650 to blathers/backport-release-22.1-88707: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: metrics refresh intermittently causing visual flash on large clusters
3 participants