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

The design of metrics::Unit #360

Open
jaskij opened this issue Apr 17, 2023 · 6 comments
Open

The design of metrics::Unit #360

jaskij opened this issue Apr 17, 2023 · 6 comments
Labels
C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-enhancement Type: enhancement. T-ergonomics Type: ergonomics.

Comments

@jaskij
Copy link

jaskij commented Apr 17, 2023

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:

  • Should metrics deal with units at all? Or just pass them through?
  • If so how should it be done?

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:

@tobz tobz added C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-enhancement Type: enhancement. T-ergonomics Type: ergonomics. labels Apr 17, 2023
@jaskij
Copy link
Author

jaskij commented Apr 17, 2023

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

  • an easy way to convert UnitName and Unit to a string, for the (common?) case where the underlying protocol has the unit as a string
  • similarly, a way to convert UnitPrefix into a number, since at least Prometheus expects the exporter to send an un-prefixed value

@tobz
Copy link
Member

tobz commented Apr 18, 2023

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
Copy link
Author

jaskij commented Apr 18, 2023

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
Copy link
Member

tobz commented Apr 18, 2023

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
Copy link
Author

jaskij commented Apr 18, 2023

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-enhancement Type: enhancement. T-ergonomics Type: ergonomics.
Projects
None yet
Development

No branches or pull requests

3 participants