-
Notifications
You must be signed in to change notification settings - Fork 498
db: reformat Metrics.String and print value separation metrics #4772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
bf324ad
to
2eca91b
Compare
Add a simple whiteboard package (inspired by datadriven/diagram.Whiteboard) for writing ASCII within a two-dimensional buffer.
Define a new package with facilities for rendering an ASCII table to a whiteboard.ASCIIBoard.
Reformat the string outputted by Metrics.String to add more organization, and add the value separation related metrics to the string's output. Informs cockroachdb#4581.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 16 files reviewed, 11 unresolved discussions
-- commits
line 2 at r1:
[nit] maybe a better name would be internal/ascii
with ascii.Board
internal/whiteboard/table/examples_test.go
line 23 at r2 (raw file):
tbl := table.Define( table.String("name", 7, table.AlignLeft, func(c Cat) string { return c.Name }), table.Div[Cat](),
[nit] Having to say [Cat]
here is unfortunate. I'd create a "base" Element
interface that doesn't have a renderValue
function, and two more specific interfaces Field
and (e.g.) StaticElement
; the latter has a renderStatic() func that doesn't depend on T
. When we render, we cast each element to either a Field[T]
or StaticElement
.
internal/whiteboard/table/examples_test.go
line 81 at r2 (raw file):
{Name: "Sugar", Age: 8, Cuteness: 10}, {Name: "Yaya", Age: 5, Cuteness: 10}, {Name: "Yuumi", Age: 5, Cuteness: 10},
[nit] Can we add a row that has a value that is wider than the field width?
testdata/event_listener
line 261 at r3 (raw file):
count | 1 0 0 0 0 0 0 0 0 -------------------------------------------------------------------------------------------------- COMMIT PIPELINE ITERATORS TABLES CGO MEMORY COMPACTIONS
This layout is much harder to read than before, it's not intuitive that we have to read down, and that there's no relation between different columns of the same row
testdata/event_listener
line 361 at r3 (raw file):
6 | 1.5KB | 2 1.5KB | 0 0 | 0B 0B | 1.5KB | 1 750B | 1 0.50 total | 4.4KB | 6 4.4KB | 0 0 | 0B 0B | 2.3KB | 3 2.2KB | 5 2.57 -------------------------------------------------------------------------------------------------
[nit] These lines of dashes after tables are more clutter than anything; a blank line would make it easier on the eyes
internal/whiteboard/table/table.go
line 17 at r2 (raw file):
) // Define defines a new table layout with the given fields.
[nit] Would be good to show a full example here. Ideally, the reader won't have to go further down to understand how they can use it, at least for the most basic cases.
internal/whiteboard/table/table.go
line 60 at r2 (raw file):
// A Definition defines the layout of a table. type Definition[T any] struct {
[nit] Layout seems like a better name
internal/whiteboard/table/table.go
line 73 at r2 (raw file):
} // RenderAll renders all the rows of the table, calling fn for each row.
I don't get what's the benefit of having RenderAll
and RenderFunc
. Why not just a Definition.Render()
which takes []T
( or iter.Seq[T]
)?
internal/whiteboard/table/table.go
line 138 at r2 (raw file):
// A RenderContext provides the context for rendering a table. type RenderContext[T any] struct {
It feels like we have a lot of T
parameters. Do we really need the entire Definition here, or could we store just the MaxFieldWidth?
internal/whiteboard/table/table.go
line 237 at r2 (raw file):
) type Orientation uint8
[nit] Could use a comment
internal/whiteboard/table/table.go
line 267 at r2 (raw file):
} func Count[T any](header string, width int, align Align, fn func(r T) uint64) Field[T] {
We can use a N numeric
parameter so we don't have to have int64 and uint64 variants, and use crhumanize
functions.
newline needed |
Previously, RaduBerinde wrote…
Wait, really? Tbh, I’m shocked. Here we actually have related metrics organized together with some structure. Previously we just had a sequence of unorganized printlns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 16 files reviewed, 12 unresolved discussions (waiting on @jbowens)
testdata/event_listener
line 261 at r3 (raw file):
Previously, jbowens (Jackson Owens) wrote…
Wait, really? Tbh, I’m shocked. Here we actually have related metrics organized together with some structure. Previously we just had a sequence of unorganized printlns.
It's the column-oriented format Anyway, maybe Sumeer can
that I find hard to read. Can't chime in too since this is
we achieve the same organization clearly subjective.
improvement but keep it row
oriented? Or at least add some
vertical separators.
These compaction stats (estimated debt, in-progress, etc.) could be another one-row table after the count table. Maybe more of these could be one or two row tables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 16 files reviewed, 15 unresolved discussions (waiting on @jbowens)
testdata/event_listener
line 350 at r3 (raw file):
metrics ---- LSM | | virtual | value sep | | ingested | amp
[nit] "virtual" is too generic, I'd keep "vtables size"
testdata/event_listener
line 351 at r3 (raw file):
---- LSM | | virtual | value sep | | ingested | amp level | aggsize | tables size | count size | refsz valblk | in | tables size | r w
[nit] "aggsize" is a bit confusing, not obvious what we're "aggregating". Maybe simply "size" and remove the separator so it's together with the level number? Btw this is always equal to tables size + refsz?
Previously, RaduBerinde wrote…
Also, not super important but something to consider: we may want to copy/paste a relevant section in an escalation, the column-oriented format makes that harder |
Reformat the string outputted by Metrics.String to add more organization, and
add the value separation related metrics to the string's output.
Informs #4581.