Consolidate the source and destination columns in the Tap and Top tables by rmars · Pull Request #1620 · linkerd/linkerd2

rmars · 2018-09-10T23:24:09Z

Consolidate the source and destination columns into one column, and add a direction column (To/From) so the user knows if the displayed resource is src/dst.

Fixes #1588 (and alleviates #1554)

siggy

looks great! a couple tioli comments...

siggy · 2018-09-10T23:47:12Z

web/app/js/components/TapEventTable.jsx

-    onFilter: (value, row) => row.base.destination.pod === value || row.base.destination.str === value,
-    render: d => srcDstColumn(_.get(d, "destination"), _.get(d, "destinationMeta.labels", {}), ResourceLink)
+    title: "Source/Destination",
+    key: "src-dst",


since the direction column already conveys src/dst info, what do you think about just calling this column name, or, resource? in the CLI we just call it NAME.

ooh, yeah, Name could be good!

siggy · 2018-09-10T23:48:33Z

web/app/js/components/TopEventTable.jsx

-    key: "source",
-    sorter: srcDstSorter("source"),
-    render: d => srcDstColumn(d.source, d.sourceLabels, ResourceLink)
+    title: " ",


curious: why no title in the Top table, but Direction title in the Tap table. i kind of like how it looks without title in the Top table. perhaps if we removed the ID column from Tap, we could render more similarly?

Mostly because of the ID column, yeah :) I think we can totally remove the ID column and have this be the first column!

Oh yeah, another reason I wanted to leave the word direction in there is because, unlike the Top tables, I wanted to keep the client side filtering for tap (so leaving the column filters in there). But the filter icon next to a blank column title looked weird....

klingerf

This is a huge improvement! I had a medium-size refactor suggestion for the code in TapUtils.jsx, but would also be ok if it makes more sense to do that in a separate branch.

klingerf · 2018-09-11T18:03:49Z

web/app/js/components/util/TapUtils.jsx

      <div className="src-dst-name">
-        { !_.isEmpty(display.pod) ? podLink : display.str }
+        { !_.isEmpty(labels.deployment) ? deployLink(labels, ResourceLink) :
+            !_.isEmpty(display.pod) ? podLink(labels, ResourceLink) : display.str


Rather than defaulting the display to show deployments, I think we should have this method take a resourceType parameter, and use the specified resourceType if a corresponding label exists. Otherwise, this setup won't work very well for other resource types, such as replication controllers.

Additionally, if I'm viewing a pod detail page, I'd prefer to see pods displayed in the top table, rather than deployments, replications controllers, etc. If we update this method to accept a resourceType string, then that should fix this issue too.

klingerf · 2018-09-11T18:11:03Z

web/app/js/components/util/TapUtils.jsx

+  <ResourceLink
+    resource={{ type: "deployment", name: labels.deployment, namespace: labels.namespace}}
+    linkText={"deploy/" + labels.deployment} />
+);


Related to my other comment below -- I'd prefer to generalize these methods to work for all resource types, not just deployments and pods. I think something like this would work:

const resourceLink = (resourceType, labels, ResourceLink) => ( <ResourceLink resource={{ type: resourceType, name: labels[resourceType], namespace: labels.namespace}} linkText={toShortResourceName(resourceType) + "/" + labels[resourceType]} /> );

klingerf · 2018-09-11T18:27:38Z

web/app/js/components/util/TapUtils.jsx

+  return (
+    <React.Fragment>
+      <div>{ !labels.deployment ? null : deployLink(labels, ResourceLink) }</div>
+      <div>{ !labels.pod ? null : podLink(labels, ResourceLink) }</div>


And similarly here, I think we should take into account resource type when generating the list of links in the popover. Or it might be cool to render a link for every label that exists, minus the namespace label. That way if we're viewing a pod, we see all of it's linkable owners in the popover.

yeah! I like the idea of trying all of these!

klingerf · 2018-09-11T18:32:46Z

web/app/js/components/TapEventTable.jsx

    title: "Direction",
+    key: "direction",
    dataIndex: "base.proxyDirection",
+    width: "60px",


It looks like 60px isn't quite wide enough for this column, causing the title text to wrap:

I'll increase the width. I also suspect #1629 will help with this.

klingerf · 2018-09-11T18:36:13Z

web/app/js/components/TapEventTable.jsx

+    width: "60px",
    filters: [
      { text: "Inbound", value: "INBOUND" },
      { text: "Outbound", value: "OUTBOUND" }


I think we should change the filter text here as well, to match the TO/FROM language that's used in the table.

Something like this:

filters: [ { text: "FROM", value: "INBOUND" }, { text: "TO", value: "OUTBOUND" } ],

Consolidate the source and destination columns

klingerf

⭐️ Thanks for making the resource type update -- looks great. Just had the 2 other nits we discussed, but otherwise looks good to me.

klingerf · 2018-09-12T19:25:38Z

web/app/js/components/util/TapUtils.jsx

+    <React.Fragment>
+      {
+        _.map(labels, (labelVal, labelName) => {
+          if (_.has(shortNameLookup, labelName) && labelName !== "namespace") {


Let's also filter "service" here, since we don't handle services on the resource detail page.

klingerf · 2018-09-12T19:29:11Z

web/app/js/components/TapEventTable.jsx

    title: "Direction",
+    key: "direction",
    dataIndex: "base.proxyDirection",
+    width: "80px",


I think we need to make this column just slightly wider so that the filter icon doesn't wrap to the next line:

Looks like 100px would do it.

* master: Update CHANGES.md for v18.9.1 release (linkerd#1631) Cleanly shutdown tap stream to data plane proxies (linkerd#1624) Change breadcrumb header to default font in styles.css (linkerd#1633) Improve top table to better cope with high RPS traffic (linkerd#1634) Add small success rate chart to table, misc web tweaks (linkerd#1628) Consolidate the source and destination columns in the Tap and Top tables (linkerd#1620) remove extraneous calc function in sidebar.css (linkerd#1632) Display more helpful websocket errors (linkerd#1626) Add breadcrumb navigation at the top of linkerd dashboard (linkerd#1613) Introduce inject check for known sidecars (linkerd#1619) Bump Prometheus to v2.4.0, Grafana to 5.2.4 (linkerd#1625) Improve performance of tap table by throttling updates (linkerd#1623) Add with-source flag to top (linkerd#1614) Conflicts: web/app/css/styles.css web/app/js/components/ResourceDetail.jsx web/app/js/index.js

rmars added the area/web label Sep 10, 2018

rmars self-assigned this Sep 10, 2018

rmars requested review from dadjeibaah, klingerf and siggy September 10, 2018 23:24

siggy approved these changes Sep 10, 2018

View reviewed changes

rmars force-pushed the rmars/ui-small-fixes branch from 3491727 to 48255eb Compare September 11, 2018 17:50

klingerf reviewed Sep 11, 2018

View reviewed changes

rmars added 7 commits September 11, 2018 17:19

Add a direction column to Top

e5f05f1

Consolidate the source and destination columns

Rename Source/Destination column to name

84c172e

Remove ID column

223a636

Refactor srcDstColumn to accept a resourceType

4d4d755

Show all linkable resource labels in popover

fd09977

Widen Direction column, change filter words

14035de

Fix eslint errors

158cc8e

rmars force-pushed the rmars/ui-small-fixes branch from 9de063b to 158cc8e Compare September 12, 2018 00:20

Don't link to a service page - we don't have one

921c6f5

klingerf approved these changes Sep 12, 2018

View reviewed changes

bump up the pixels. I'm sure it will help

3faf7a0

rmars mentioned this pull request Sep 12, 2018

Improve top table to better cope with high RPS traffic #1634

Merged

rmars merged commit 01be78e into master Sep 12, 2018

rmars deleted the rmars/ui-small-fixes branch September 12, 2018 20:32

rmars mentioned this pull request Sep 12, 2018

Truncate src/dst names in tap/top tables #1554

Closed

Conversation

rmars commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rmars commented Sep 10, 2018 •

edited

Loading