Generalize metrics

[Kobzol]

My recent PR (#1322) has broken displaying various metrics in the compare page (#1337). This PR adds all (currently) known metric variants to the Metric enum.

[rylev]

I think there are certainly some metric types that we consider first class (e.g., instructions, max-rss, and cycles) while the others we don't normally have much logic associated with and it's fine to just pass around the string. It might make sense to limit the enum to the first class metric types and then have an Other variant for everything else

[Kobzol]

So currently we use three metrics as "first-class": instructions, cycles and max RSS. I could use these as enum variants and leave the rest as Custom(String), if you agree. To implement that, we either need to write a custom deserializer for the enum, or use something like https://docs.rs/serde-enum-str/latest/serde_enum_str/. Which would you prefer?

(There is #[serde(other)], but it only works for unit variants).

[rylev]

A custom deserializer should be easy with #[serde(deserialize_with = "path")]. I would say go with that.

[Kobzol]

Oh, I thought that we couldn't use that because we need something like that for the whole enum, but I can just probably use a deserializer function for the single variant and count on the fact that serde will try to deserialize it as the last resort. That should work, thanks, I'll try it.

[Kobzol]

Hmm, maybe it's not so straightforward after all.

Like this:

fn parse_custom_metric<'a, D: Deserializer<'a>>(deserializer: D) -> Result<String, D::Error> {
    String::deserialize(deserializer)
}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub enum Metric {
    #[serde(rename = "instructions:u")]
    Instructions,
    #[serde(rename = "cycles:u")]
    Cycles,
    #[serde(rename = "max-rss")]
    MaxRSS,
    #[serde(deserialize_with = "parse_custom_metric")]
    Custom(String),
}

it does not parse Custom, since it expects custom in the input.

Like this:

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
#[serde(untagged)]
pub enum Metric {
    #[serde(rename = "instructions:u")]
    Instructions,
    #[serde(rename = "cycles:u")]
    Cycles,
    #[serde(rename = "max-rss")]
    MaxRSS,
    #[serde(deserialize_with = "parse_custom_metric")]
    Custom(String),
}

it parses everything as Custom (which, incidentally, would be enough for the way we use the metrics now, but it's not correct).

I'm out of ideas of how to do this on the field level, so I'm inclined to write a custom deserializer for the whole struct, unless you see a better way.

[Mark-Simulacrum]

A custom deserializer seems fine to me.

[Kobzol]

I implemented it with a custom deserializer.

[Kobzol]

We decided with @Mark-Simulacrum to just explicitly enumerate the set of known metrics for now and expand them in the future.

[Mark-Simulacrum]

Hm, it looks like the database check is no longer succeeding, but it's also failing on master? Going to go ahead and remove it from the branch protection rule for now, and we can try to investigate and fix it in the future.