Conversation
Signed-off-by: Alex Leong <[email protected]>
cli/cmd/top.go
Outdated
| if row.by == by && row.source == source && row.destination == destination { | ||
| match := row.by == by && row.destination == destination | ||
| if withSource { | ||
| match = row.by == by && row.source == source && row.destination == destination |
There was a problem hiding this comment.
maybe match = match && row.source == source slightly clearer?
...or, a big ol' one-liner...
if row.by == by && row.destination == destination && (row.source == source || !withSource)| tbprint(0, 0, "(press q to quit)") | ||
| x := 0 | ||
| for i, header := range columnNames { | ||
| if i == 0 && !withSource { |
There was a problem hiding this comment.
consider:
if header == "Source" && !withSource {There was a problem hiding this comment.
I think this is kinda brittle either way (brittle to changing the header string vs brittle to reordering the columns) so I'd lean towards keeping it the way it is.
Signed-off-by: Alex Leong <[email protected]>
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
The implementation looks good. I just had one comment about the flag itself.
cli/cmd/top.go
Outdated
| "Display requests with this :authority") | ||
| cmd.PersistentFlags().StringVar(&options.path, "path", options.path, | ||
| "Display requests with paths that start with this prefix") | ||
| cmd.PersistentFlags().BoolVar(&options.withSource, "with-source", options.withSource, "Include the source column") |
There was a problem hiding this comment.
I find boolean flags that default to true to be awkward to use from a command line perspective, since they require a boolean value to disable. For instance, to suppress the source column, you would need to run:
linkerd top --with-source=false
Whereas if the flag were defaulted to false, then you could run:
linkerd top --hide-sources
Or whatever you'd choose to call it in that case.
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
⭐️ Realized I didn't click Approve with my previous review. I didn't mean for that comment to be a blocker.
Signed-off-by: Alex Leong <[email protected]>
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
⭐️ Great, thanks for updating!
* 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
3 participants
Fixes #1593
Add a
--with-sourceflag tolinkerd topwhich defaults to true. Setting this to false remove the source column from the output.Here is an example of the difference when the emojivoto demo is used with the vote-bot deployment scaled up to 3 pods:
Signed-off-by: Alex Leong [email protected]