ui: Discovery Chain #6746

johncowen · 2019-11-06T15:37:20Z

This PR is a first draft of a visualization of the discovery chain in the UI.

Related work:

Line/edge animation and redirect inference ui: Discovery Chain Continued #6939

There are still a few little bits that are WIP, mainly the styling of the resolver subsets on rollover/selection. But thought it best to get something up soon for folks to look at.

Few notes (also see commit notes)

We chose to use ngraph.graph to figure out what nodes/edges get highlighted when you click to select a node.
Lines are drawn with svg <path>
We have to use vanilla JS for adding event listeners to SVG elements as it seems like ember can't do this whilst passing through the event object, at least not in a straightforward manner.
We looked into being able to listen for changes to arbitrary properties of an object and thus add them as computed properties dependencies. There doesn't seem to be a straightforward way to do this, which has impacted certain decisions here.
We avoid re-serializing and redrawing all the data for the discovery chain when somebody clicks a node. We use the id's of the nodes/edges and use CSS instead. We've done this in a fairly ember like manner instead of poking into the DOM. This approach means a single DOM change rather than potentially 'lots'. There's probably more explanation surrounding the reasons for this, so please ask if you're interested.

I'll potentially add more detail into this description at a later date.

Oh, no tests yet! And we are currently failing as we haven't fixed up the default ember generated tests yet.

codecov · 2019-12-11T11:14:54Z

Codecov Report

Merging #6746 into ui-staging will decrease coverage by <.01%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           ui-staging    #6746      +/-   ##
==============================================
- Coverage       65.64%   65.63%   -0.01%     
==============================================
  Files             443      443              
  Lines           53312    53312              
==============================================
- Hits            34994    34991       -3     
- Misses          14095    14099       +4     
+ Partials         4223     4222       -1

Impacted Files	Coverage Δ
api/session.go	`70.68% <0%> (-5.18%)`	⬇️
command/catalog/list/nodes/catalog_list_nodes.go	`80.34% <0%> (-3.42%)`	⬇️
agent/kvs_endpoint.go	`75.44% <0%> (-1.2%)`	⬇️
api/watch/funcs.go	`75.64% <0%> (-1.04%)`	⬇️
agent/service_manager.go	`82.47% <0%> (-0.86%)`	⬇️
command/exec/exec.go	`66.56% <0%> (-0.6%)`	⬇️
agent/consul/leader_connect.go	`70.58% <0%> (-0.44%)`	⬇️
agent/consul/state/catalog.go	`66.82% <0%> (-0.19%)`	⬇️
agent/consul/leader.go	`69.22% <0%> (-0.13%)`	⬇️
agent/consul/acl_replication.go	`75.9% <0%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f41f79d...654aff3. Read the comment docs.

DingoEatingFuzz

This is an awesome implementation of an awesome feature.

Feel free to take everything I said with a grain of salt since I know this is all still WIP.

Apologies for not getting around to this in time for the beta 😞

DingoEatingFuzz · 2019-12-12T01:47:02Z

ui-v2/app/components/discovery-chain.js

+    });
+  return failovers;
+};
+const getType = function(nodes = {}, type) {


This is pretty minor, but the name getType doesn't scream "returns an array" to me. Maybe getAllOfType or filterByType?

Ah good call, I'm thinking getNodesByType. Will switch out in #6939

DingoEatingFuzz · 2019-12-12T01:51:47Z

ui-v2/app/components/discovery-chain.js

+  }),
+  graph: computed('chain.Nodes', function() {
+    const graph = this.dataStructs.graph();
+    Object.entries(get(this, 'chain.Nodes')).forEach(function([key, item]) {


Does Object.entries get transpiled? Otherwise it very much doesn't work in IE11. I'm not opposed to that, but it seems easy enough to use Object.keys instead and avoid one IE 11 💥

I wouldn't have thought so, I've no idea if ember polyfills it either. I know ember ember polyfills a bunch of stuff as we've spoken about it before.

I've never managed to find any ember documentation that gives me a list of what it polyfills and is therefore safe to use, I don't suppose you know of any do you?

I think right now we are kind of sweeping IE11 under the rug 😄 We have quite a few IE11 💥 's

I would generally use keys for this, but since we have entries now (that is polyfillable) I'm kinda trying it out instead of using keys.

As far as I know, there is no documentation like that. They treat those extension methods as part of the API, so you can see it on the Array and String doc pages, but it doesn't explicitly call out what is a prototype extension.

But Ember definitely doesn't extend the Object prototype, "only" String, Array, and Function. And the function extensions are just the convenience methods on, property, and observes.

Ah k, thanks for the extra info!

DingoEatingFuzz · 2019-12-12T01:56:51Z

ui-v2/app/services/data-structs.js

+  graph: function(nodes) {
+    return createGraph();
+  },
+});


Why make a service for this instead of importing ngraph.graph in the disco chain component?

Typically services are only used if there is shared state to manage, but (for now at least) this is just a wrapper around the ngraph constructor.

Services are the only way to inject things using dependency injection in ember. So this is me thinking ahead a little bit to testing (and mocking things out), potentially changing my mind and using what I was originally thinking of using for graphing, and also we are using more complicated data-structs in #6821, so I'm thinking access to all those types of things will be from here - I suppose its a little similar to our dom service in that respect.

I know it looks a little overkill at the moment, but I'm thinking this service will grow.

DingoEatingFuzz · 2019-12-12T01:57:18Z

ui-v2/app/services/data-structs.js

+
+export default Service.extend({
+  graph: function(nodes) {
+    return createGraph();


Looks like nodes is a superfluous param.

Removed this in #6939

DingoEatingFuzz · 2019-12-12T02:01:35Z

ui-v2/app/components/discovery-chain.js

+    // FIXME: Use https://github.com/mathiasbynens/CSS.escape
+    return {
+      nodes: nodes.map(item => `#${CSS.escape(item)}`),
+      edges: edges.map(item => `#${CSS.escape(item)}`),
+    };
+  }),


What's stopping you from using the polyfill? It seems especially important in this case, since window.CSS is still experimental.

Nothing stopping me, just I haven't done it yet. I was waiting for a review of this to make sure the direction is sound. If we don't do the CSS thing then we don't need this. I'm still quite happy with the way I've done this though and it sounds like although you have a few queries and comments that its generally a-ok - so I'll probably add this polyfill in now in #6939

Btw I'm trying to follow @backspace s suggestion of using FIXMEs for things that should be looked at before merging to master and TODOs for things that could be done any time. Converting a FIXME to a TODO is also fine in my 👀 . Most important thing right now is getting people using this so we can get any feedback or suggestions on usage/interaction/look and feel.

I'll ping you on #6939 when the polyfill is in there.

Got it. I'll make sure to eye the destination branch before commenting on the FIXMEs in the future 👍

DingoEatingFuzz · 2019-12-12T03:00:21Z

ui-v2/app/styles/components/discovery-chain/layout.scss

+%discovery-chain .splitter-inlets {
+  left: 50%;
+  /* keep the extra calculations here for doc purposes */
+  margin-left: calc(calc(-32% / 2) + 1% - 3px);


I was gonna scroll right past this, but you said the extra calc is for doc purposes, but I'm not sure what it is documenting.

It isn't clear to me what the significance of these numbers are. Is the `-32% representing the width of one column?

Is the `-32% representing the width of one column?

Yes, you can see the width further up https://github.com/hashicorp/consul/pull/6746/files#diff-fa97384713a77522da399d8e63c5a579R49

Using 16% instead of 32% / 2 isn't as obvious as to what it is. 32% is almost a third so I was guessing folks would assume that its the width of one column (then divided by 2 to get the center)

I've added an extra comment in #6939, lemme know what you think.

DingoEatingFuzz · 2019-12-12T03:08:50Z

ui-v2/app/templates/components/discovery-chain.hbs

+      opacity: 1;
+    }
+  {{/if}}
+</style>


This is an anti-pattern in the sense that I would never think to look at a component's templates to find CSS.

Why not use active classes or something similar instead of rewriting the underlying stylesheet?

Yeah so this is obviously the tough one with this entire feature. I can't quite remember the exact detail - but, all the data here comes in as keyed objects, not arrays. The keys are arbitrary id's so you can't specify them in computed properties (I think I mentioned this problem offline a while ago).

This approach is so we can highlight the related nodes/edges in the graph.

This means you have to detect a change on the root of the entire chain. So if you change a property you probably have to re-filter out the entire chain with loops and conditionals again, recheck all the dom positions etc etc. Basically lots of stuff.

Once it's done that, it will have to mutate multiple (possibly lots) of DOM nodes in order to apply an 'active' class.

Using this approach we just use our graphing lib to give us the id's of all the things that should be highlighted (as we would have to do with the above approach anyway), make one single DOM mutation, and let CSS/the browser do the rest. Seems to me to be the simplest, least intensive approach. There might be other ways to do it, but this works great and is simple so I went with it. I started with this DOM poking version of this, but figured I could use ember templates to do it instead to make it as ember-y as possible.

Agree with the 'never thinking to look in the template for styles though' I'll add a comment in the stylesheet so its clear there's a little bit of CSS in the template. I'll drop it in #6939

I don't know the numbers off the top of my head, but my (potentially very wrong) intuition is that even if going the route of having an array representation of the underlying data will mean decorating N DOM nodes with a class marker, the alternative of injecting a stylesheet, re-baking selectors, and verifying the entire dom tree is slower.

It's similar work to what you're avoiding, just done by the browser instead. Granted it could be highly optimized C++ instructions, so I can't say for sure if there's actually a performance tradeoff here.

Granted it could be highly optimized C++ instructions

Sounds like a good assumption that would be faster 😀 I could be completely wrong of course, but I don't think it matters that much here.

I think the thing that matters more is - the best code is zero code. You can't have bugs in code if it doesn't exist, and it means I can spend time doing other stuff (better performance 😄 )

DingoEatingFuzz · 2019-12-12T03:10:49Z

ui-v2/app/templates/components/discovery-chain.hbs

+</svg>
+<div class={{concat 'tooltip' (if activeTooltip ' active' '')}} style={{{ concat 'position: absolute;top:' y 'px;left:' x 'px;'}}}>
+  <span role="tooltip">{{round tooltip decimals=2}}%</span>
+</div>


I appreciate that all the DOM lives in templates instead of dynamically added in didReceiveAttrs.

DingoEatingFuzz · 2019-12-12T03:14:20Z

ui-v2/app/templates/components/resolver-card.hbs

@@ -0,0 +1,46 @@
+<div class="resolver-card">
+  <header onclick={{onclick}} id={{concat 'resolver:' item.ID}}>
+    <a name="">


If the anchor has no name or href, what's the value in using an anchor? To preserve the hover/click semantics?

Hmm, yeah it's weird I remember playing with taking these name=""s out, let me take another look, I can't remember what the thinking was. I know that I was thinking 'i can see that these nodes are eventually gonna link somewhere to give users more info about the service' so thats why they are as. Right now they act more like checkboxes rather than a's or buttons's, so semantically as they stand right now label/checkboxes might fit, but again I'm thinking they will eventually link somewhere.

I won't be making any major changes here for this I don't think, unless you feel super strongly, we can iterate on the accessibility aspect of it in another iteration, potentially before 1.7.0 final release.

DingoEatingFuzz · 2019-12-12T03:40:41Z

ui-v2/app/templates/components/resolver-card.hbs

+      <li onclick={{onclick}} id={{concat 'resolver:' item.ID}}>
+        <a name="">


I think you can add keyboard navigability/accessibility to this pretty easily by changing to buttons.

I made a codepen to test my own assumptions. Try tabbing through and hitting enter and space on stuff. https://codepen.io/DingoEatingFuzz/pen/gObMPoz

See above comments re: anchor/checkboxes.

I also had a feeling that buttons have a restriction over what you should put inside them - I wasn't sure about this as it was just a feeling, but I've had a quick look at it seems like you shouldn't put block elements inside a button, so that would severely restrict us here. Although I wonder if thats an old thing like as?

The only other thing would be to do some absolute positioning of a button over the rest of the element - but I really don't want to go there and seem like that would create more problems than we are trying to solve here.

Ha, sounds like the toggle I just implemented for node drain. Uses a hidden checkbox for accessibility/keyboard and is wrapped in a label that contains the richer markup.

Yes, exactly!

I was curious as to whether this was an old pre-HTML5 thing I was remembering (like for a's) or if the "don't put block things inside buttons" was just some feeling I had, so I took a little bit of a deeper look:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button

Permitted content: Phrasing content but there must be no Interactive content

https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#Phrasing_content

So we can't use dl's, ols etc in buttons, which we use here.

I think generally I'd continue to steer clear of using buttons to wrap anything that could potentially end up being anything more than just a piece of text.

😮

not only consisting of white spaces characters

I'm sure we do this somewhere 😆 ! Well I think we have empty buttons somewhere, so not whitespace as such, but just empty - might check on that further down the line!

johncowen · 2019-12-12T12:47:17Z

Thanks @DingoEatingFuzz , I've done addressed most of your suggestions in #6939, please take a look there to see what you think.

I'm yet to add the CSS escape polyfill, but that should be on the end of #6939 pretty soon (although potentially another PR).

Please don't let the absence of that polyfill hold you up for a review of #6939 though. All of this will eventually go onto ui-staging, and I always give that branch a final overall review/test/poke/TODO read etc before I merge it down to master.

Thanks!

1. discovery-chain to orchestrate/view controller 2. route-card, splitter-card, resolver card to represent the 3 different node types. 3. route-match helper for easy formatting of route rules 4. dom-position to figure out where things are in order to draw lines 5. svg-curve, simple wrapper around svg's <path d=""> attribute format. 6. data-structs service. This isn't super required but we are using other data-structures provided by other third party npm modules in other yet to be merged PRs. All of these types of things will live here for easy access/injection/changability 7. Some additions to our css-var 'polyfill' for a couple of extra needed rules

Interesting bits: 1. We add a %card base component here, eventually this will go into our base folder and %stats-card will also use it for a base component. 2. New icon for failovers

1. We rebased and got some sort of ember 'make sure you use `set`' error occasionally. We added that and then got a lint error so we've added some FIXMEs here to look at this 2. Event listeners are added one tick too early, so we add a `next` here. Also added a FIXME to look at this later incase we can get rid of the `next`

This stops resolvers and splitters from jumping around as you make configuration changes

1. Add in the things we use for the animations 2 Use IntersectionObserver so we know when the tab is visible, otherwise the dom-position helper won't work as the dom elements don't have any display. 3. Add some base work for animations and use them a little 4. Try to detect if a resolver is a redirect. Right now this works for datacenters and namespaces, but it can't work for services and subsets - we are awaiting backend support for doing this properly. 5. Add a fake 'this service has no routes' route that says 'Default' 6. redirect icon 7. Add CSS.escape polyfill for Edge

1. Readd discovery-chain repo as an event sourable repo 2. Fix up link colors in the disco chain

* Add data layer for discovery chain (model/adapter/serializer/repo) * Add routing plus template for routing tab * Add extra deps - consul-api-double upgrade plus ngraph for graphing * Add discovery-chain and related components and helpers: 1. discovery-chain to orchestrate/view controller 2. route-card, splitter-card, resolver card to represent the 3 different node types. 3. route-match helper for easy formatting of route rules 4. dom-position to figure out where things are in order to draw lines 5. svg-curve, simple wrapper around svg's <path d=""> attribute format. 6. data-structs service. This isn't super required but we are using other data-structures provided by other third party npm modules in other yet to be merged PRs. All of these types of things will live here for easy access/injection/changability 7. Some additions to our css-var 'polyfill' for a couple of extra needed rules * Related CSS for discovery chain 1. We add a %card base component here, eventually this will go into our base folder and %stats-card will also use it for a base component. 2. New icon for failovers * ui: Discovery Chain Continued (#6939) 1. Add in the things we use for the animations 2 Use IntersectionObserver so we know when the tab is visible, otherwise the dom-position helper won't work as the dom elements don't have any display. 3. Add some base work for animations and use them a little 4. Try to detect if a resolver is a redirect. Right now this works for datacenters and namespaces, but it can't work for services and subsets - we are awaiting backend support for doing this properly. 5. Add a fake 'this service has no routes' route that says 'Default' 6. redirect icon 7. Add CSS.escape polyfill for Edge

ghost · 2020-01-25T01:53:28Z

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

johncowen requested a review from a team November 6, 2019 15:37

johncowen added the theme/ui Anything related to the UI label Nov 6, 2019

johncowen force-pushed the feature/ui-disco-chain branch 2 times, most recently from f6569d8 to 46c4308 Compare November 18, 2019 11:32

johncowen added this to the 1.7.0 milestone Nov 18, 2019

johncowen force-pushed the feature/ui-disco-chain branch from 46c4308 to 1a2dfff Compare November 19, 2019 10:39

johncowen force-pushed the feature/ui-disco-chain branch 2 times, most recently from dc7fed1 to 0d2f406 Compare December 4, 2019 12:23

johncowen force-pushed the ui-staging branch from 6830f7c to 3e381a7 Compare December 11, 2019 09:13

johncowen force-pushed the feature/ui-disco-chain branch from 0d2f406 to 5eca6bb Compare December 11, 2019 10:09

johncowen force-pushed the feature/ui-disco-chain branch from 5eca6bb to da48aac Compare December 11, 2019 11:21

DingoEatingFuzz approved these changes Dec 12, 2019

View reviewed changes

johncowen force-pushed the feature/ui-disco-chain branch from da48aac to 400ff1d Compare December 12, 2019 11:28

johncowen mentioned this pull request Dec 12, 2019

ui: Discovery Chain Continued #6939

Merged

johncowen force-pushed the ui-staging branch from c3d2fe1 to b330b80 Compare December 17, 2019 15:40

johncowen force-pushed the feature/ui-disco-chain branch 2 times, most recently from faa2d8d to 835bd31 Compare December 17, 2019 16:23

John Cowen and others added 9 commits December 17, 2019 19:01

Add data layer for discovery chain (model/adapter/serializer/repo)

fe1a34c

Add routing plus template for routing tab

e523981

Add extra deps - consul-api-double upgrade plus ngraph for graphing

e94914d

Related CSS for discovery chain

033eb93

Interesting bits: 1. We add a %card base component here, eventually this will go into our base folder and %stats-card will also use it for a base component. 2. New icon for failovers

Enable blocking queries

33eb753

Add alpha sorting for splitters and resolvers

f5cdd40

This stops resolvers and splitters from jumping around as you make configuration changes

johncowen force-pushed the feature/ui-disco-chain branch from aca9374 to f52f7a0 Compare December 17, 2019 19:06

Amends due to rebase

9184e3d

1. Readd discovery-chain repo as an event sourable repo 2. Fix up link colors in the disco chain

Tweak tests to account for new namespace work

654aff3

johncowen merged this pull request into ui-staging Dec 17, 2019

johncowen deleted the feature/ui-disco-chain branch December 17, 2019 19:27

ghost locked and limited conversation to collaborators Jan 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Discovery Chain #6746

ui: Discovery Chain #6746

johncowen commented Nov 6, 2019 •

edited

Loading

codecov bot commented Dec 11, 2019 •

edited

Loading

DingoEatingFuzz left a comment

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 13, 2019

johncowen Dec 13, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 13, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 13, 2019

johncowen Dec 13, 2019

DingoEatingFuzz Dec 12, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019

DingoEatingFuzz Dec 12, 2019

johncowen Dec 12, 2019 •

edited

Loading

DingoEatingFuzz Dec 13, 2019

johncowen Dec 13, 2019

johncowen Dec 13, 2019 •

edited

Loading

johncowen Dec 13, 2019

johncowen commented Dec 12, 2019

ghost commented Jan 25, 2020

		<li onclick={{onclick}} id={{concat 'resolver:' item.ID}}>
		<a name="">

ui: Discovery Chain #6746

ui: Discovery Chain #6746

Conversation

johncowen commented Nov 6, 2019 • edited Loading

codecov bot commented Dec 11, 2019 • edited Loading

Codecov Report

DingoEatingFuzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen commented Dec 12, 2019

ghost commented Jan 25, 2020

johncowen commented Nov 6, 2019 •

edited

Loading

codecov bot commented Dec 11, 2019 •

edited

Loading

johncowen Dec 12, 2019 •

edited

Loading

johncowen Dec 13, 2019 •

edited

Loading