Add small success rate chart to table, misc web tweaks#1628
Conversation
- Add a small success rate chart to the metrics tables - Improve latency formatting for seconds latencies - Rename upstream/downstream to inbound/outbound - Make Top table look consistent with rest of tables on page - Fix widths of metrics column columns so that tables align
rmars
force-pushed
the
rmars/little-success
branch
from
c23e034 to
b0fff85
Compare
siggy
left a comment
siggy
left a comment
There was a problem hiding this comment.
changes all look great, a couple tioli comments...
- some latency values have 3 sig-dig's and some have 1:
is it possible to format these values so we always have 3 sig-digs, regardless of unit?
- the table columns line up, but there are some cases where the column headers differ between tables, specifically the Authorities table doesn't have a
DASHcolumn. is it possible to make the columns in Authorities vertically align with their counterparts in the tables above it?
I think we should find the right mix. It'll be jarring to have like... 615µs and 10.3ms... the idea was just so that at the second level, we don't lose a bunch of granularity. But I feel like ms and µs are easier to comprehend at the non-decimal granularity. But if we format all the numbers to be non-decimal, we lose some info at the second display between 1 and 10 seconds (hence this change). |
|
The Authorities not being aligned would actually be fixed by #1225 so I'm inclined to just leave it as is until we can get to that ticket? Or if we urgently want it to be aligned, I think we should just add the Authorities in Grafana. |
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
⭐️ Awesome! Just had that one nit about column width, but otherwise looks good to me.
| dataIndex: "successRate", | ||
| key: "successRateRollup", | ||
| className: "numeric", | ||
| width: "120px", |
There was a problem hiding this comment.
This width needs to be slightly wider for when the sortable header is present:
Testing locally it looks like 130px would work.
There was a problem hiding this comment.
(note that this applies to the definition in TopEventTable.jsx and not the one in MetricsTable)
* 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
The success rate mini chart is last in the Top table, but near first in the metric table. I felt like it might be good to put it farther away from the other coloured column (Grafana link), but the column order can easily be changed.
Something to think about: the VotePoop endpoint is showing up as grey (0% SR). You'd expect it to be red but because the gauge is also reflecting the actual percent, the coloured part of this is 0, so the red part doesn't show. An alternative is to actually just show a full arc no matter the value of the success rate. We could also colour the rows or text instead of adding this arc.
Fixes #1598
Fixes #1600
Fixes #1589
Fixes #1610