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

Zoom level indicator #2420

Closed
wants to merge 3 commits into from
Closed

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Apr 3, 2017

Addresses the first suggestion from #2392 - in the resource view, a small horizontal resource scale is displayed from which we can see how deeply we've zoomed. The problem of having zoomed to deeply into a wide box and not seeing any changes when scrolling was brought to me a couple of times.

Notes
  • The resource boxes of zero width are hidden from the resource view now, which fixes the zoom limit being overflown in the CPU view. Consistent resource consumption info in the resource view #2499
  • We display percentages differently on the zooming scale (from how they are shown on the nodes) - in particular, we use a flexible point precision, with up to 4 digits after the decimal point (as 2 were not precise enough for deep zooms).
  • To get a satisfying number rounding policy for zoom scale percentages, I dug up the old round helper from Update D3 to version 4.4.0 #2028 that was removed in the meantime.
Additional changes

@fbarl fbarl force-pushed the resource-view-zoom-level-indicator branch 2 times, most recently from a278e9b to d9c2680 Compare April 3, 2017 16:18
@fbarl fbarl requested a review from davkal April 3, 2017 16:29
@fbarl fbarl self-assigned this Apr 3, 2017
@fbarl fbarl changed the title [WIP] Zoom level indicator Zoom level indicator Apr 3, 2017
@fbarl fbarl force-pushed the resource-view-zoom-level-indicator branch from d9c2680 to 716f8ac Compare April 3, 2017 17:16
@davkal
Copy link
Contributor

davkal commented Apr 7, 2017

The scale helps a lot, but I found two issues:

The numbers dont add up: 130 + 77 > 160
screen shot 2017-04-07 at 14 32 20

And zooming in on the center makes the upper boxes in the upper band disappear (whereas if I zoom in on the far left, they keep getting displayed)
screen shot 2017-04-07 at 14 35 42
VS (both scales around 128MB
screen shot 2017-04-07 at 14 35 09

Copy link
Contributor

@davkal davkal left a comment

Choose a reason for hiding this comment

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

The numbers not adding up is a blocker, because it diminishes trust. If it helps, maybe show zoom % instead of metrics for the sake of getting this out fast.


// Does the same that the deprecated d3.round was doing.
// Possibly imprecise: https://github.com/d3/d3/issues/210
export function round(number, precision = 0) {

This comment was marked as abuse.

This comment was marked as abuse.

@fbarl fbarl force-pushed the resource-view-zoom-level-indicator branch from 716f8ac to 96ea505 Compare April 12, 2017 13:49
@fbarl fbarl force-pushed the resource-view-zoom-level-indicator branch from edca137 to d2c1701 Compare April 12, 2017 16:23
@fbarl
Copy link
Contributor Author

fbarl commented Apr 12, 2017

@davkal PTAL. I draw only a simple small scale in the bottom-right corner now, according to your feedback and our discussion that followed (I also tried it closer to the resource boxes, but didn't look that good to me).

The numbers not adding up is a blocker, because it diminishes trust.

I adjusted the displayed metrics info (last commit) so hopefully the trust will not be diminished now :)

@davkal
Copy link
Contributor

davkal commented Apr 20, 2017

LGTM. This can be merged as is.

I still find the two axis confusing (eg., expressing used memory both in x and y dimensions). Also, when zooming into the center part, the boxes to the left still disappear and you have no way of knowing that they are there. I am aware that other flame graphs suffer the same problems, but maybe indicators could help: either a thumbnail overview with a rectangle representing the visible part, or thin-lined boxes to the left that afford a panning action to get them into view.

@fbarl
Copy link
Contributor Author

fbarl commented Apr 20, 2017

Thanks for reviewing @davkal.

I agree with both of your points, let me try to summarize my thoughts on them:

I still find the two axis confusing (eg., expressing used memory both in x and y dimensions).

This currently only affects the Hosts layer, but in the future it might affect any layer with the notion of resource capacity, so it'd be good to decide on the final looks sooner rather than later. Here are some ideas that I had:

  1. Drop the notion of capacity and show only the resource utilization for every node.
    • PROS: full design consistency between the layers, simpler and more intuitive layout
    • CONS: the capacity information actually seems more important to show for hosts than the resource consumption as the latter can be roughly approximated by the stack of containers above it
  2. Make the above the default and then have a sort of check-box Show capacities that would switch to the view as it is now.
    • PROS: serves the more intuitive layout first, doesn't remove any information from the resource view
    • CONS: your comment would still apply to the layout with capacities, e.g. if someone lands there first
  3. Make hosts resource boxes look the same as containers/processes ones, but let their width stand for capacity instead of resource utilization.
    • PROS: showing arguably the most useful information while keeping the design consistent
    • CONS: inconsistent semantics between the layers, hosts would feel empty

Note: Show the bar for hosts horizontally instead of vertically maybe sounds like a good idea but it actually looked quite messy when I tried it.

I don't really have any other ideas that wouldn't require major changes to the layout. However, I got this comment more than once so I quickly prototyped another layout idea in another experimental branch. Looks more or less like this:

new-design-proposal

So basically all the layers are collapsed into one and the nodes are embedded into their parents instead of being shown on top of them. The trigger for me was thinking how point 3. above leaves hosts empty, so I thought of filling them with some information.

Also, when zooming into the center part, the boxes to the left still disappear and you have no way of knowing that they are there. I am aware that other flame graphs suffer the same problems, but maybe indicators could help: either a thumbnail overview with a rectangle representing the visible part, or thin-lined boxes to the left that afford a panning action to get them into view.

  1. Thin-lined boxes on the sides would break the existing zooming isometry and would not be consistent with the zooming scale info, so I'm not a huge fan of that idea (unless we modify this PR to show only the zooming factor instead of displaying the scale).
  2. Some sort of pseudo rectangle that gives hint about what is outside of the viewport sounds better to me and is definitely something I could implement in another story.
  3. Alternatively, a trick with very little impact that could hint there is some content behind the screen would be to extend the text info background for the resource box. Compare the two pictures below:

text-0
text-1


In conclusion, I'm not sure whether it makes sense to actually merge this PR, especially if we are going to do some changes to the current layout, so I'd freeze it a bit before I get more feedback on what I wrote above I think.

@fbarl
Copy link
Contributor Author

fbarl commented May 12, 2017

Dropped in favor of a more generic zoom control/indicator #2513.

@fbarl fbarl closed this May 12, 2017
@fbarl fbarl deleted the resource-view-zoom-level-indicator branch July 20, 2017 13:28
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.

2 participants