Skip to content

Conversation

@zackcl
Copy link
Collaborator

@zackcl zackcl commented Aug 5, 2024

This PR (Resolves #1796) addresses a critical issue where the Metrics component causes the app to freeze when rendering large metrics datasets up front.

The PR implements a lazy loading mechanism for the metrics tree structure, which:

  • Prepares metrics data for on-demand loading
  • Supports asynchronous child node loading
  • Renders child nodes only when a parent node is expanded
  • Improves rendering performance by removing tree-invisible class usage (reference)

Copy link
Collaborator

@bcb37 bcb37 left a comment

Choose a reason for hiding this comment

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

This looks good Just one question.
Also, search by 'All' or 'Context' throws an error on lines 168 and 174 of metrics.component.ts in cases where there's no context. It could be fixed by coalescing like : !!data.context?.filter(...

@zackcl
Copy link
Collaborator Author

zackcl commented Aug 6, 2024

@bcb37 I resolved all your comments. Please feel free to approve this PR.

Copy link
Collaborator

@bcb37 bcb37 left a comment

Choose a reason for hiding this comment

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

Just a clarification.

@zackcl
Copy link
Collaborator Author

zackcl commented Aug 7, 2024

@bcb37 I have applied the suggested change. Please feel free to approve the PR.

@zackcl zackcl requested a review from bcb37 August 7, 2024 09:31
bcb37
bcb37 previously approved these changes Aug 7, 2024
@danoswaltCL
Copy link
Collaborator

looks good to me too, just one request to avoid the awkward conversion from promise to an observable. Please try that out, I tested it quickly and it worked on the smaller metric dataset that I have loaded in, or seemed to.

@zackcl
Copy link
Collaborator Author

zackcl commented Aug 8, 2024

@danoswaltCL Thanks for you suggestions. I have resolved all your comments and updated the deleteNode function, which was not working previously. I've tested it with large metrics datasets, and everything seems to work fine. Please feel free to approve this PR.

@zackcl zackcl requested a review from danoswaltCL August 8, 2024 11:19
@zackcl zackcl marked this pull request as draft August 8, 2024 14:38
@zackcl
Copy link
Collaborator Author

zackcl commented Aug 8, 2024

I will review this one more time.

@danoswaltCL
Copy link
Collaborator

I'd approve, looks to be back to draft though, did you find an issue?

@zackcl
Copy link
Collaborator Author

zackcl commented Aug 8, 2024

I realized I didn’t need to update the deleteNode and findParents functions to make the deletion work. Instead, I needed to update the insertNode function to include the id in a node again, which I had previously removed. I will discard some of my later commits and redo them properly.

@zackcl zackcl force-pushed the bugfix/1796-metrics-freeze branch from 0bfee49 to e6e82fb Compare August 8, 2024 16:12
@zackcl zackcl marked this pull request as ready for review August 8, 2024 16:29
@zackcl
Copy link
Collaborator Author

zackcl commented Aug 8, 2024

@danoswaltCL Sorry for the delay, the PR is ready now. After resolving your comments (e6e82fb), I made one more commit (0db0de5) to make the node deletion work properly. Please feel free to approve this PR.

@danoswaltCL danoswaltCL merged commit 3dba666 into dev Aug 8, 2024
@danoswaltCL danoswaltCL deleted the bugfix/1796-metrics-freeze branch August 8, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rendering of nested profile metrics runs out of memory

4 participants