Dynamically render units on metrics tab

[charliepark]

Fixes #1866.

We're pulling in data for the instance page's metrics tab, showing read / write counts and size, and the number of flushes. Currently, we aren't adjusting the numbers, and are just showing the raw count / bytes.

This PR updates how we render the data. Because the data is cumulative, we can use the largest data value to determine the unit of measurement for the set of data. We then divide the given datapoint by the appropriate divisor and pass that data along to the chart.

I'm open to suggestions for better ways to label the overall chart.

While it's fine on datapoints using Byte counts …

… it's a little meh on Count charts …

(The thing you're supposed to be looking at is at the very top of the chart, currently showing "count x K".)

@benjaminleonard if you have any thoughts, I'm all ears.

I'm also open to extracting this into a utility function, if we think it'll be useful in other places.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Jan 30, 2024 4:46am

[benjaminleonard]

Byte counts look good – wondering if for the counts we show the full number in the tooltip and use K in the axis. And just stick with "Writes (Count)"?

Took me a second to realise K wasn't a unit. Lowercase K with no space between it and the number will likely help too.

[charliepark]

In the latest commit I pulled in @benjaminleonard's suggestions for that Y-Axis labeling vs. popup number.

[benjaminleonard]

Wondering if there's some logic we can write, something like a find comfortable axis function – that given a range and a number of desired steps quantises to multiples. In this case we'd say snap to 0.5k and it'd give us 7 steps up to 3.5k.

We have a similar issue with the KiB / MiB / GiB where they aren't universally divisible in a neat way.

Not to blow up the complexity of this PR. You could merge this and we could tackle in a follow up.

[david-crespo]

Yeah, I was thinking that is the main outstanding issue here — would be much nicer if those were round numbers. I will think about it a bit.

[benjaminleonard]

Actual implementation requires some experimentation to get something more robust, but in the realm of this might work:

function findAxis(range: number, targetSteps: number, snap: number): number[] {
    let stepSize = Math.ceil((range / targetSteps) / snap) * snap;
    let steps = Math.ceil(range / stepSize);

    let axis: number[] = [];
    for(let i = 0; i <= steps; i++) {
        axis.push(i * stepSize);
    }

    return axis;
}

console.log(findAxis(3.3, 7, 0.5));
// [0, 0.5, 1, 1.5, 2, 2.5, 3, 3.5]
console.log(findAxis(10, 6, 0.5));
// [0, 2, 4, 6, 8, 10]

[david-crespo]

So.. we are already doing work to get clean round numbers for the ticks. We pick a maxY that's divisible by the number of ticks, and then we get ticks evenly spaced out between 0 and maxY.

console/app/components/TimeSeriesChart.tsx

Lines 133 to 147 in 764f731

	// We use the largest data point +20% for the graph scale. !rawData doesn't
	// mean it's empty (it will never be empty because we fill in artificial 0s at
	// beginning and end), it means the metrics requests haven't come back yet
	const maxY = useMemo(() => {
	if (!rawData) return null
	const dataMax = Math.max(...rawData.map((datum) => datum.value))
	return roundUpToDivBy(dataMax * 1.2, TICK_COUNT) // avoid uneven ticks
	}, [rawData])
	// If max value is set we normalize the graph so that
	// is the maximum, we also use our own function as recharts
	// doesn't fill the whole domain (just up to the data max)
	const yTicks = maxY
	? { domain: [0, maxY], ticks: getVerticalTicks(TICK_COUNT, maxY) }
	: undefined

console/app/components/TimeSeriesChart.tsx

Lines 42 to 45 in 764f731

	function getVerticalTicks(n: number, max: number): number[] {
	const idxs = new Array(n).fill(0)
	return idxs.map((_, i) => Math.floor(((i + 1) / n) * max))
	}

So I think all we need to do to avoid the decimals is pass TICK_COUNT * <relevant power of 10> to roundUpToDivBy. The hard bit is figuring out the power of 10. Or it would be if you hadn't already done it — that's what divisor is. If I pass in divisor to TimeSeriesChart and do this roundUpToDivBy(dataMax * 1.2, TICK_COUNT * divisor) (and turn the toFixed(2) into toFixed(0) in yAxisTickFormatter) I am able to get the following:

That confirms I'm thinking about this roughly the right way. However, the bytes numbers are pre-divided while the counts are not, and this messes up the bytes charts, which don't want their axis multiplied by 1024:

So I'll need to figure out how to reconcile this.

[david-crespo]

Not sure how this happens (we should look into it) but it seems we cannot assume these numbers are monotonically increasing and therefore the last value is the largest.

https://oxide.sys.rack2.eng.oxide.computer/projects/trey-trey-trey/instances/apache/metrics
https://oxide.sys.rack2.eng.oxide.computer/projects/trey-trey-trey/instances/foo/metrics

[david-crespo]

I think that's this: oxidecomputer/omicron#1624

I knew it sounded familiar.

[github-actions]

license-eye has totally checked 467 files.

Valid	Invalid	Ignored	Fixed
414	1	52	0

Click to see the invalid file list

• app/pages/project/instances/instance/tabs/MetricsTab.spec.ts

[david-crespo]

While we could improve this, it's clearly an improvement on what's there and works well with big numbers (tested by changing the fake data generator). I want to wrap up #1910 for the release, so I am good with merging this and fixing it up later.

[david-crespo]

I fixed the last value not necessarily being the largest by looking at all values and finding the max. I always feel weird about spreading 3000 args into Math.max but I tested it a while ago (we do this elsewhere) and it is apparently fine.

[charliepark]

There are a few more enhancements we've talked about in chat, but we'll merge this PR and then decide on prioritization and path forward on general metrics improvements separately.