ui: add "total" values for disk and memory to node list; add num CPUs #28189
Conversation
|
This change is |
|
Showing both the total capacity and the percentage is kind of redundant. I'd just show one or the other (probably the percentage) and leave the other for a mouseover to save a bit of horizontal space. |
vilterp
force-pushed
the
node-list-denominators
branch
from
a5f4853
to
4a0d747
Compare
|
Updated. It doesn't save a whole lot of horiz space because of the column headers, but it is less cluttered. TODO: use a real tooltip, not html
|
|
Could the same be done to CPU. In the example, 4 cores (25%) |
|
@robert-s-lee yes, although we should sort out #28154 first to avoid inconsistency with the hardware dashboard. |
There was a problem hiding this comment.
1f64cec really needs more in the description. There's a lot of code changes here and the summary in the commit message doesn't capture that.
What was the rationale behind the reordering in cfb2ab3. Please add that to the commit message.
The final commit isn't needed. It should be fixed back in the original commits for the changes.
Overall, the changes LGTM, but please go back and fixup the prior commits instead of issuing new ones.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 10 of 10 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/server/status/recorder.go, line 604 at r3 (raw file):
// GetTotalMemory returns either the total system memory or if possible the // cgroups available memory. func GetTotalMemory(ctx context.Context) (int64, error) {
Why did you move this? There's no mention the commit message.
pkg/server/status/status.proto, line 61 at r5 (raw file):
]; // total_system_memory is the total RAM available to the system // (or, if possible, the memory available to the cgroup this process is in)
This comment update should be on the commit where total_system_memory was added.
vilterp
force-pushed
the
node-list-denominators
branch
from
4a0d747
to
fc096ce
Compare
|
TFTR — squashed down to a backend and a frontend commit, and added to the commit descriptions. PTAL. |
Includes moving the GetTotalMemory function from the server package to the server/status package, so it can be used by the stats recorder and also the base Server where it was originally used. Release note: None
Also, reorder the columns of the table to keep hardware stats together: CPUs, memory, and disk usage. Release note (admin ui change): add number of CPUs and percentages of memory and disk usage to the node list on the overview page.
vilterp
force-pushed
the
node-list-denominators
branch
from
fc096ce
to
e7592e1
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r9, 2 of 2 files at r10.
Reviewable status:
|
TFTR! bors r+ |
Build failed |
|
bors r+ |
28189: ui: add "total" values for disk and memory to node list; add num CPUs r=vilterp a=vilterp  There may be a more visually concise way of showing this. Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com>
Build succeeded |
| import { INodeStatus, MetricConstants, BytesUsed } from "src/util/proto"; | ||
| import {FixLong} from "oss/src/util/fixLong"; |
| // CPUs - the number of CPUs on this node | ||
| { | ||
| title: "CPUs", | ||
| cell: (ns) => ns.num_cpus, |
There was a problem hiding this comment.
it seems weird to sort on num CPUs, but it's also a little weird that this column wouldn't be sortable...
| title: "Capacity Usage", | ||
| cell: (ns) => { | ||
| const { usable } = nodeCapacityStats(ns); | ||
| const used = BytesUsed(ns); |
There was a problem hiding this comment.
I'm a bit uncomfortable that we're discarding used from nodeCapacityStats here in favor of calling BytesUsed, but elsewhere we just use that. let's make these consistent, please
| const used = BytesUsed(ns); | ||
| return ( | ||
| <span title={`Total: ${BytesWithPrecision(usable, 0)}`}> | ||
| {BytesWithPrecision(used, 0)} |
There was a problem hiding this comment.
why are we throwing away the tenths place? the whole point of using Bytes everywhere is that we're consistent in our reporting across the whole project.
| <span title={`Total: ${BytesWithPrecision(usable, 0)}`}> | ||
| {BytesWithPrecision(used, 0)} | ||
| {" "} | ||
| ({Math.round(used / usable * 100)}%) |
There was a problem hiding this comment.
how about Percentage from util/format instead?
| cell: (ns) => Bytes(ns.metrics[MetricConstants.rss]), | ||
| cell: (ns) => { | ||
| const used = ns.metrics[MetricConstants.rss]; | ||
| const available = FixLong(ns.total_system_memory).toNumber(); |
There was a problem hiding this comment.
i'd prefer if we did similar treatment in nodeSumStats for memory as we do for capacity, to keep things consistent
Until cockroachdb#28189, GetTotalMemory was only called at startup; that PR caused it to be called every 10s, filling the logs with spam on some platforms. This changes it to only log warnings on startup. The value could be cached so it only has to be called on startup, but it seems possible that the amount of RAM on the machine could change at runtime, and we'd want to reflect that in the Admin UI, which is where the results of the every-10s calls end up. Release note: None
Until cockroachdb#28189, GetTotalMemory was only called at startup; that PR caused it to be called every 10s, filling the logs with spam on some platforms. This changes it to only log warnings on startup. The value could be cached so it only has to be called on startup, but it seems possible that the amount of RAM on the machine could change at runtime, and we'd want to reflect that in the Admin UI, which is where the results of the every-10s calls end up. Release note: None
29405: server/status: de-spam system memory log r=vilterp a=vilterp Until #28189, `GetTotalMemory` was only called at startup; that PR caused it to be called every 10s, filling the logs with spam on some platforms. This changes it to only log warnings on startup. The value could be cached so it only has to be called on startup, but it seems possible that the amount of RAM on the machine could change at runtime, and we'd want to reflect that in the Admin UI, which is where the results of these every-10s calls end up. Fixes #28891 Release note: None Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com>
Until cockroachdb#28189, GetTotalMemory was only called at startup; that PR caused it to be called every 10s, filling the logs with spam on some platforms. This changes it to only log warnings on startup. The value could be cached so it only has to be called on startup, but it seems possible that the amount of RAM on the machine could change at runtime, and we'd want to reflect that in the Admin UI, which is where the results of the every-10s calls end up. Release note: None
There may be a more visually concise way of showing this.