Skip to content
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

Scaling utilization Y axis #1772

Merged
merged 5 commits into from
Oct 12, 2023
Merged

Scaling utilization Y axis #1772

merged 5 commits into from
Oct 12, 2023

Conversation

benjaminleonard
Copy link
Contributor

@benjaminleonard benjaminleonard commented Oct 5, 2023

Fixes #1640

There's a few small changes in here.

  • We use the largest data point +20% for the graph scale clamping at maxValue (if set) which is usually overall capacity
  • Rounding the calculatedMaxValue to be divisible by 6 so we get even ticks
  • Normalizing the mock metrics data to be within max capacity (previously the walker would exceed 100%)

On the maxValue rounding – we could do it the other way round where we find a number of ticks that fits but it might be confusing to have it change between graphs. The uneven tick lines was most obvious on disk space because we are using TiB.

@vercel
Copy link

vercel bot commented Oct 5, 2023

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Oct 12, 2023 8:05pm

// We use the largest data point +20% for the graph scale
// clamping at `maxValue` (if set) which is usually overall capacity
const calculatedMaxValue = useMemo(() => {
if (!maxValue) return null
Copy link
Collaborator

@david-crespo david-crespo Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you still want a reasonable max on the chart (like dataMax * 1.2) if none is specified from outside? Or it just does it for you?

if (!maxValue) return null
if (!rawData) return maxValue
const dataMax = Math.max(...rawData.map((datum) => datum.value))
const unroundedValue = Math.min(maxValue, dataMax * 1.2)
Copy link
Collaborator

@david-crespo david-crespo Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we don't have the capacity number nailed down, I'm wary of picking maxValue as the max if dataMax is actually bigger. In the unfortunate case where dataMax is bigger than maxValue, we have to set the chart max to at least dataMax, otherwise you're not seeing all the data. Ideally in that situation we would also show a dotted flat line or something at maxValue too, though that can be a separate change. We'd have to explain it somehow too.

I came up with this after banging my head against this for bit. Does it make any sense?

Situation Exact range Desired max
Data way less than cap dataMax * 1.2 < maxValue dataMax * 1.2
Data close to cap but still under dataMax < maxValue < dataMax * 1.2 maxValue
Data over cap (pathological but possible) maxValue < dataMax dataMax * 1.2

The middle one is the odd one out. I think that's what's messing me up. What if we always used dataMax * 1.2 as the top edge of the chart, and then if maxValue happens to be inside the range of the chart, we show it as a dotted line? Or maybe not always dataMax * 1.2 — maybe if dataMax is anywhere near maxValue, like at least half of it, we make the max of the chart big enough that the maxValue line is shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds about right for me. Setting the top of the graph to the capacity doesn't really clearly communicate that if it's not being applied regardless of usage. So some other indicator is probably best.

Whilst the max capacity is still fuzzy, we could probably just omit it and the logic around keeping the maxValue line visible. Happy to just change to dataMax * 1.2 for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would work. We'd scrap maxValue altogether in that case, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now yeah, we can revisit when we have updated capacity values. Looks like we can use Recharts reference lines when thats ready: https://recharts.org/en-US/examples/LineChartWithReferenceLines

}
: {
tickSize: 6,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid of tickSize here — I thought it was weird to only specify it when there's a max, but also 6 is the default anyway so we're not doing anything.

@david-crespo david-crespo merged commit e74d5ae into main Oct 12, 2023
@david-crespo david-crespo deleted the max-y-metrics branch October 12, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using rack capacity for system utilization max y is bad when stats are small
2 participants