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

Dynamic units #2494

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Dynamic units #2494

wants to merge 14 commits into from

Conversation

charliepark
Copy link
Contributor

@charliepark charliepark commented Oct 9, 2024

Open to general conversation on this.

Currently, we have a few set units that we apply to our Memory (GiB) and Storage (TiB) utilization representations. The important thing is that they should be the same unit within each tile. Here are the tiles, for example:
Screenshot 2024-10-09 at 1 30 55 AM

As we move to having larger drives in the future, we probably won't be able to assume that GiB and TiB will be reliable values for the Memory and Storage tiles (and other representations). Note that 7,792.4 GiB is a bit unwieldy.
Screenshot 2024-10-07 at 5 08 49 PM

This PR introduces a dynamic way to determine the appropriate unit and to convert the numbers.
Screenshot 2024-10-09 at 1 33 22 AM

I am open to any thoughts on this. For example, is the core assumption here off? Is it more straightforward to a user to say 3000 GiB rather than recasting it as TiB?

There are a few places where we'd need to revisit our existing "toGiB"-style conversions, but I think this is a good proof of concept so we can talk through it.

Here's one spot, for example, where we're always running a "to GiB" conversion (note the bottom row), but where a dynamic converter might make more sense …
Screenshot 2024-10-09 at 1 17 46 AM
… I was focused on embiggening the middle row, but then noticed that since I added the dynamic conversion, both rows now show the proper (TiB) value. (Note: TiB for the bottom row was because it was already large enough for that, not based off of the middle row's new TiB scale.)
Screenshot 2024-10-09 at 1 17 28 AM

Copy link

vercel bot commented Oct 9, 2024

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Dec 16, 2024 6:19pm

app/util/units.ts Outdated Show resolved Hide resolved
@david-crespo
Copy link
Collaborator

I agree it would be nice to get things into more natural units, though sometimes we might be abbreviating too much. Maybe it makes sense to always show some digits after the decimal? I know it's relatively rare to have exactly 7.00 TiB as the value , but when I see 7 TiB, I wonder: is that rounded? What is the real number?

@benjaminleonard
Copy link
Contributor

Should we use toLocaleString to keep the commas for values >= 1,000?

That might be less frequent with the dynamic units, but still. Especially where the units are set within a tile.

My other question is whether those are the right thresholds to switch? I had a vague thought that we might have a preference for a unit and switch if it's significantly higher or lower but that's probably overcomplicating it.

@david-crespo
Copy link
Collaborator

round uses Intl.NumberFormat internally, so we should still get the nice commas. I actually agree about complicating the thresholds, I think we could get a better result.

console/app/util/math.ts

Lines 51 to 61 in 98103a2

export function round(num: number, digits: number) {
// unlike with splitDecimal, we hard-code en-US to ensure that Number() will
// be able to parse the result
const nf = Intl.NumberFormat('en-US', {
maximumFractionDigits: digits,
// very important, otherwise turning back into number will fail on n >= 1000
// due to commas
useGrouping: false,
})
return Number(nf.format(num))
}

@david-crespo
Copy link
Collaborator

david-crespo commented Oct 9, 2024

Oh god, what am I saying. It says right there in round that it does not use commas, plus it runs the result through Number(...). Whether this gets formatted properly at display time depends on us running it through displayBigNum, which does run it through toLocaleString. The problem here is that we need to make sure we call that everywhere we need to. There may be no reason to have a formatAsBytes function that returns a number. If it's always for display, it's probably safest to have it return the string as we'd want to see it.

console/app/util/math.ts

Lines 68 to 94 in f6219f1

/**
* Abbreviates big numbers, first in compact mode like 10.2M, then in eng
* notation above 10^15. Used internally by BigNum, which you should generally
* use instead for display as it includes a tooltip with the full value.
*
* Boolean represents whether the number was abbreviated.
*/
export function displayBigNum(
num: bigint | number,
/** Argument here for testing purposes. Leave undefined in app code! */
locale?: string
): [string, boolean] {
const compact = Intl.NumberFormat(locale, {
notation: 'compact',
maximumFractionDigits: 1,
})
const abbreviated = num >= 1_000_000
const result = abbreviated
? num < 1e15 // this the threshold where compact stops using nice letters. see tests
? compact.format(num)
: toEngNotation(num, locale)
: num.toLocaleString(locale)
return [result, abbreviated]
}

@charliepark
Copy link
Contributor Author

charliepark commented Oct 9, 2024

On the decimals / commas / return types …
One of the issues with returning strings is that we do sometimes use the values downstream, like in CapacityBar, where we call const pct = percentage(provisioned, capacity). Also, there are a few places where it looks like we use BigInt values, like — also in CapacityBar — where we use bigint for the allocated IPv6 addresses. From http://localhost:4000/system/networking/ip-pools/ip-pool-4:
Screenshot 2024-10-09 at 5 29 08 PM

The good news is that the decimals are getting dropped because they're .00 or .X0. If they're non-zero (.XY, 0X), which I suspect they will be most of the time, they'll be rendered.

So — open to thoughts, but — I think we can leave them as-is, as the likelihood that they'll be an even BinaryUnit multiple — while non-zero (wokka wokka) — is probably low.

@david-crespo
Copy link
Collaborator

david-crespo commented Oct 9, 2024

We could/should calculate percentages from the raw byte values.

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.

3 participants