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

[Logs UI] Move page template to avoid re-renders #151311

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Feb 15, 2023

Summary

closes #150624

This PR improves how the page template re-renders, preventing it from doing so when context providers in the hierarchy below it change.

logs_ui.mov

For maintainers

At given moments, the Logs UI URL changes, updating the logPosition state. This will re-render the template and its children.

@crespocarlos crespocarlos added release_note:fix Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services backport:skip This commit does not require backporting v8.8.0 labels Feb 15, 2023
@crespocarlos crespocarlos changed the title [Logs UI] Move context to template child components to avoid re-renders [Logs UI] Move context provider to avoid re-renders Feb 15, 2023
@weltenwort weltenwort requested review from weltenwort and removed request for weltenwort February 15, 2023 14:37
@crespocarlos crespocarlos marked this pull request as ready for review February 15, 2023 16:29
@crespocarlos crespocarlos requested a review from a team as a code owner February 15, 2023 16:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@crespocarlos crespocarlos changed the title [Logs UI] Move context provider to avoid re-renders [Logs UI] Move page template to avoid re-renders Feb 15, 2023
@Kerry350 Kerry350 self-requested a review February 20, 2023 10:32
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Thanks for opening this 🙏

The new position in the hierarchy looks good to me, it's a much better / clearer home for it.

Thinking aloud here - hasData and isDataLoading are hardcoded so it would be nice if we could memoize the component to stop re-renders on context change (for example, adding a filter will cause a re-render of the template). However, due to the way React.memo works with children I really can't think of a way to make that work unless I'm missing something 🤔 Any state machine changes are always going to make the two child components need to re-render, as the logStreamPageState prop will change, and that cancels out the memoization.

@weltenwort Did you have an idea how the memoization might work here? (as you wrote the issue / ACs).

@weltenwort
Copy link
Member

weltenwort commented Feb 20, 2023

💭 Good question. I don't actually see a way to achieve that and still pass the current state to its child. We could stop doing that, but then we'd loose the type safety of the state.matches(), so I'm not too eager for that either.

Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

We could stop doing that, but then we'd loose the type safety of the state.matches(), so I'm not too eager for that either.

Yeah, I would agree with that. If performance does become a concern I'm sure there are multiple places we can improve lower in the hierarchy.

LGTM for the change in hierarchy position 👍

@crespocarlos crespocarlos added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.7.0 and removed backport:skip This commit does not require backporting labels Feb 21, 2023
@crespocarlos
Copy link
Contributor Author

@Kerry350 we could actually remove the hasData and isDataLoading from there. Their default value is the same as we're hardcoding in the page_content.tsx.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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
infra 1.3MB 1.3MB +37.0B

History

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

@crespocarlos crespocarlos merged commit 6943797 into elastic:main Feb 22, 2023
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.7 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 151311

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 25, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

5 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 151311 locally

@weltenwort
Copy link
Member

@crespocarlos should we add backport:skip or do we still want to backport this?

@crespocarlos crespocarlos removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) backport missing Added to PRs automatically when the are determined to be missing a backport. labels Apr 15, 2024
@crespocarlos
Copy link
Contributor Author

@weltenwort completely missed these comments. Removed the labels. Thanks

@crespocarlos crespocarlos added the backport:skip This commit does not require backporting label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.7.0 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] The page template is re-rendered unnecessarily on any state change of the log stream page
6 participants