metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.1k stars 151 forks source link

The design of metrics::Unit #360

Open jaskij opened 1 year ago

jaskij commented 1 year ago

Why?

In #337 @tobz asks:

As an example, is metrics::Unit actually sufficient for all users as-is

And it's most definitely not: off the top of my mind, it's missing temperature, voltage, electric current, power - just to name a few used to monitor computer hardware.

And while yes, metrics is probably intended mostly as a library to measure the performance of software, if it gets popular enough at some point it will inevitably be used with more physical quantities.

Hence, this issue, because the current design of metrics::Unit is definitely not satisfactory.

Questions to answer?

There are two questions to answer here:

Possible designs

Just plain strings

There's so many units of measurement that simply passing them through is a very tempting option. So much so that the current implementation of OpenTelemetry protocol does exactly this.

Internally

Another option is to have some enumeration in metrics itself, with an escape hatch for more unusual units. This would still be a decently large undertatking. A quick estimate is that just the SI system has around a hundred different units.

A quick sketch of an API would be something like below, with the understanding that UnitName, UnitPrefix and Unit all implement Display, or a similar trait, to allow Recorders to easily convert the units to, at least, a ShardString.

enum UnitName {
    Seconds,
    Bytes,
    Bits,
    Volts,
    Amperes,
    // etc - probably close to a hundred entries
    Custom(SharedString)
}

enum UnitPrefix {
    None,
    // decimal prefixes
    Micro,
    Milli,
    Deca,
    Kilo,
    // binary prefixes
    Kibi,
    Mibi,
    // and so on
    Custom(SharedString)
}

struct Unit {
    name: UnitName,
    prefix: UnitPrefix,
}

Use an external crate

Option I admittedly have not looked much into, use an existing crate to deal with this. A few I found with a quick search:

jaskij commented 1 year ago

For the internally handled option, I would expect, at least:

tobz commented 1 year ago

My gut instinct is that the only baked-in unit names/prefixes should be the ones that metrics::Unit currently deals with, and perhaps, additionally, temperature.

While obviously there's a lot of real unit values that are standardized -- length, mass, energy, force, yadda yadda yadda -- they're by and large the exception to how metrics is used. They could just as easily be driven through the Custom variant.

Coupled with the notion of MetricAttribute (as described in #337), we could then create an adapter crate (something like metrics-extras or metrics-units) that worked off of dimensioned (or whatever else) and had provided Into impls for MetricAttribute.... or at least that would be an option.

jaskij commented 1 year ago

You are right that with the Custom escape hatch, the set of units directly represented in metrics::Unit can probably be small and lean. That said, Custom probably needs to be {name: SharedString, canonical: Option<SharedString>} so that both as_str() and as_canonical_str() can behave properly.

Another option I can see is to keep units internal, but since adding new ones will most likely end up near-trivial just encourage PRs, for when someone can't stand using the Custom escape hatch.

The one thing about using an external crate is, best I can tell, all three crates I found focus on expressing quantities with a unit, not units themselves. It's a subtle distinction, but one which might make them not really suitable for use with metrics.


You did not address splitting the unit and the prefix - can I take it as a tacit agreement? While similar to units themselves, the set is much smaller - at first I'd suggest adopting at least a part of metric prefixes and all eight binary IEC prefixes. For representing actual numerical values of the prefixes, it's probably best to use num_rational::Ratio, although the inner type is up in the air.

tobz commented 1 year ago

You did not address splitting the unit and the prefix - can I take it as a tacit agreement?

I generally agree with that aspect, yes. As an aside, one area where unit and prefix living side-by-side sort of falls down is with temperature: millicelsius or kilofahrenheit isn't exactly a thing in practice, even if you could theoretically determine that "1.48 degrees kilofahrenheit" is just 1,480 degrees fahrenheit.

Like, it's technically correct, no question, but it would definitely be weird. I guess this implies we might need to make prefix either optional or have a None variant.

jaskij commented 1 year ago

Like, it's technically correct, no question, but it would definitely be weird. I guess this implies we might need to make prefix either optional or have a None variant.

Good catch - a None variant on the prefix is a must. Not even because of temperature, but just generally - if a metric is in seconds, it does not have a prefix. I'll update that sketch to include it so it's not forgotten.

As an aside, I've heard people use "megadollars" in casual conversations.

BMorinDrifter commented 1 year ago

metrics::Unit is missing multiple unit strings supported by CloudWatch Embedded Metrics: https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_MetricDatum.html