[profiler-viz] Several improvements requested #5337
Conversation
mihaibudiu
force-pushed
the
profiler-fixes
branch
from
c2b66aa to
b69c359
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the profiler visualization by adding operation information to node displays, improving tooltip scrollability, and streamlining the top nodes API by removing the hardcoded limit parameter.
Key changes:
- Added operation field to node display in tooltips and top nodes tables
- Enhanced tooltip styling with sticky headers and scrollbars for better navigation
- Removed the hardcoded "n=20" parameter from top nodes functions across the codebase
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ProfilerTooltip.svelte | Added operation column display and improved scrolling with sticky headers |
| ProfilerLayout.svelte | Added operation field to node data and updated metric selection handling |
| ProfilerDiagram.svelte | Removed node count parameter from showTopNodes and cleaned up minimap styling |
| profiler.ts | Updated showTopNodes and topNodes methods to remove count parameter |
| profile.ts | Added new rebalancing metrics and operation field to NodeAndMetric class |
| cytograph.ts | Modified topNodes to include operation field and removed result limiting |
| index.ts | Updated event listeners to remove hardcoded count parameter |
| fileLoader.ts | Removed count parameter from topNodes and showTopNodes methods |
| index.html | Reformatted button text from "Top 20 nodes" to "Top nodes" |
js-packages/web-console/src/lib/components/profiler/ProfilerTooltip.svelte
Outdated
Show resolved
Hide resolved
Karakatiza666
left a comment
Karakatiza666
left a comment
There was a problem hiding this comment.
To fix the style issues with the tooltip, copy .profiler-tooltip-container and .profiler-tooltip CSS classes definition verbatim from issue4632
js-packages/web-console/src/lib/components/profiler/ProfilerTooltip.svelte
Outdated
Show resolved
Hide resolved
|
Removing the shadow from the minimap makes you lose its borders, so it looks like two abstract boxes rather than a minimap I am in favor of leaving the shadow - it is not as visually cumbersome as a real border, but it still provides a useful silhouette I find useful for the minimap |
|
the shadow does not mean anything - there is no square object to represent. |
I don't want this to happen |
mihaibudiu
force-pushed
the
profiler-fixes
branch
from
b69c359 to
52e5281
Compare
Why not? This seems like a sensible behavior (to update the list of top nodes to reflect the current selected metric). |
|
You can always click a second time on top nodes if you want that to happen. |
If we want this behavior, the right way to implement it would be to separate the top-N UI into a separate panel with its own controls. But if we have a single control to select a metric, it should be applied consistently to the graph and the top-N list. |
|
what is a panel? |
I believe it's confusing: you allow the user to select a metric, but only apply their selection to some of the widgets. It's easy for the user to get confused as to what metric the top-N list applies to. |
Signed-off-by: Mihai Budiu <[email protected]>
Signed-off-by: Mihai Budiu <[email protected]>
Signed-off-by: Karakatiza666 <[email protected]>
Karakatiza666
force-pushed
the
profiler-fixes
branch
from
52e5281 to
0fcf7fb
Compare
Karakatiza666
left a comment
Karakatiza666
left a comment
There was a problem hiding this comment.
I implemented the requested change myself
Fixes #5333
This PR contains several changes that were originally part of #4632
It contains all changes from the first commit and some changes from the second commit that modify the profiler.
(and a few additional changes)