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

Initial version of the resource view #2296

Merged
merged 32 commits into from
Mar 24, 2017
Merged

Initial version of the resource view #2296

merged 32 commits into from
Mar 24, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Mar 3, 2017

First step towards resolving #2242.

Pretty much copying the layout from the slides with some metrics that are already supported by backend:

  • Metrics: CPU, Memory
  • Layers: Hosts -> Containers -> Processes
  • Resource capacity only shown for Hosts
Changes
  • Replaced gridMode global state boolean with topologyViewMode string (which can now have values topo, grid, resource).
  • Moved bunch of stuff to selectors:
    • availableCanvasMetrics and pinnedMetric, to simplify the transition between topologies in the resource view
    • Canvas margins, which are now customizable per view mode
    • Generalized viewport/canvas selectors
    • All the node networks logic
  • Zooming logic:
    • Extracted all of it (with caching) into a special component, as now we'll have zooming both in the resource view and the graph view
    • Separated zooming limits from zoom state and made them always reflect the layout to ensure e.g. we can always zoom out enough to view the whole layout, even when the layout grows a lot comparing to the initial state
  • Moved metric selector to the top, just below the view mode selector.
  • Contrast mode and saving to SVG are supported.
Action items (by priority)
  1. Processes metrics given by the backend are inconsistent with the Containers metrics, especially in terms of maximum utilization i.e. capacity. In this PR I'm using a hack to fix the view on the frontend, but a more correct solution would require making backend send consistent data from a single source of truth - Resource view - reconcile inconsistencies between metrics of different topologies #2389
  2. The resource view layout is currently kept static by design, i.e. it's not being periodically updated like the other two view modes, since it would require more work on polling and we also have to see how to make dynamic view relatively stable when zoomed in - Resource view - do nodes live polling #2387
  3. No filtering of the nodes in the resource view is currently supported:
    a. Search bar should be shown and functioning - Resource view - make nodes searchable #2384
    b. Topology options should be shown and working - Resource view - show topologies filters in bottom-left corner #2385
  4. Report resource view metrics and topologies' layers info from the backend instead of having it hardcoded on frontend - Resource view - report metrics and layers info from the backend #2388
Other UI/optimizations ideas

@fbarl fbarl self-assigned this Mar 3, 2017
@fbarl fbarl force-pushed the basic-resource-view branch 4 times, most recently from 10433f0 to b31443a Compare March 15, 2017 11:56
@fbarl fbarl force-pushed the basic-resource-view branch from 8d329b3 to 2b92f25 Compare March 20, 2017 10:10
@rade
Copy link
Member

rade commented Mar 20, 2017

cedfc8b seems unrelated to this PR. Furthermore, it appears to contain changes - the logo.js update - unrelated to its supposed purpose.

@fbarl
Copy link
Contributor Author

fbarl commented Mar 20, 2017

@rade In that commit I actually reverted the changes to logo.js that I did earlier in the PR, so the net code diff contains minimal changes in that file. In the previous commits I broke saving to SVG and as I was fixing it, I noticed it didn't work for the table mode, which required two lines more to fix - do you think I should revert it here and create a separate issue for that?

@rade
Copy link
Member

rade commented Mar 20, 2017

create a separate issue for that

just do a separate PR for disabling SVG saving in table mode.

@fbarl fbarl force-pushed the basic-resource-view branch from 136f534 to b26f5fe Compare March 21, 2017 11:08
@fbarl fbarl requested review from foot and davkal March 21, 2017 17:05
@fbarl fbarl changed the title [WIP] Prototype version of the resource view Initial version of the resource view Mar 21, 2017
@davkal
Copy link
Contributor

davkal commented Mar 22, 2017

Looks great, neat, and informative! Some UI nits:

Initial zoom:
The initial zoom on an incognito window was too far in:
screen shot 2017-03-22 at 13 21 38
At that point it was hard to tell that I had to zoom out to see this:
screen shot 2017-03-22 at 13 22 00

Layers:
I find it confusing that hosts and containers disappear when I load the process topology. What is the benefit?

Small boxes:
There is no interaction that allows me to figure out what a certain box is:
screen shot 2017-03-22 at 13 31 56
There is no hover and no click to give me more info.

Blinking "Select a metric":
It's clear why that is there, but only to me as a developer of Scope. I prefer sensible defaults instead, the empty screen is a confusing surprise.

Wording:
"consumed" means that something is gone. Maybe "used"?

@fbarl fbarl force-pushed the basic-resource-view branch from 60bc152 to f77d9cb Compare March 22, 2017 14:42
@fbarl
Copy link
Contributor Author

fbarl commented Mar 22, 2017

I have addressed all the UI feedback I got for this story:

@davkal,

The initial zoom on an incognito window was too far in.

This was fixed automatically as I made the resource view for each topology have HOSTS as the base layer. Since that topology has no filters, all hosts are always loaded at once and since they are the base layer, they determine max width of the layout, so it doesn't get resized after it's rendered once.

I find it confusing that hosts and containers disappear when I load the process topology. What is the benefit?

As we agreed, I (temporarily) made all the topologies show all three layers, so there is actually no difference in what they are displaying now (and therefore I'm using the same zoom caching key across all the topologies in the resource view now).

There is no hover and no click to give me more info.

I added a tooltip with a short summary.

Blinking "Select a metric"

I hardcoded CPU and Memory as the only metrics for the resource view, which enabled me to always pin a default metric in the resource view when none (or a different one) was previously pinned.

Wording

Changed consumed to used.

@2opremio,

I moved the metric selector to the top, just below the view mode selector in all the views, making it more consistent. If you want, you can take a final look at the resource view behaviour.

@fbarl fbarl force-pushed the basic-resource-view branch 3 times, most recently from 9d8eb39 to dd7107b Compare March 23, 2017 10:51
@fbarl
Copy link
Contributor Author

fbarl commented Mar 23, 2017

@davkal @foot I'm going to merge this PR in a couple of hours, with all the commits squashed. Be free to take a final look (or peek through the code :)) in the meantime.

@fbarl fbarl force-pushed the basic-resource-view branch from dd7107b to 3dc4e9d Compare March 24, 2017 10:52
@fbarl fbarl merged commit 69fd397 into master Mar 24, 2017
@fbarl fbarl deleted the basic-resource-view branch March 31, 2017 10:53
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.

3 participants