From 7d5538267e45737d951df440879d96cea533592f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 12 Oct 2023 14:54:57 -0700 Subject: [PATCH] [oximeter] Use stable hash, stable format, for deriving timeseries_key (#4251) - Uses [bcs](https://crates.io/crates/bcs) as a stable binary format for inputs to the `timeseries_key` function - Uses [highway](https://crates.io/crates/highway) as a stable hash algorithm (it's keyed, fast, portable, and well-distributed). - Additionally, adds an EXPECTORATE test validating the stability of timeseries_key values. Fixes https://github.com/oxidecomputer/omicron/issues/4008 , and also addresses the issue raised in https://github.com/oxidecomputer/omicron/issues/4221 regarding stable input NOTE: This PR itself *also* breaks the stability of the `timeseries_key` (hopefully for the last time), and will rely on https://github.com/oxidecomputer/omicron/pull/4246 to wipe the metrics DB for the next release. --- Cargo.lock | 21 +++ Cargo.toml | 1 + oximeter/db/Cargo.toml | 4 + oximeter/db/src/lib.rs | 130 ++++++++++++++++-- .../db/test-output/field-timeseries-keys.txt | 12 ++ .../db/test-output/sample-timeseries-key.txt | 1 + oximeter/oximeter/Cargo.toml | 1 + oximeter/oximeter/src/types.rs | 20 ++- 8 files changed, 180 insertions(+), 10 deletions(-) create mode 100644 oximeter/db/test-output/field-timeseries-keys.txt create mode 100644 oximeter/db/test-output/sample-timeseries-key.txt diff --git a/Cargo.lock b/Cargo.lock index d5a90f7f85..ce17dbe311 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -493,6 +493,16 @@ dependencies = [ "sha2", ] +[[package]] +name = "bcs" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4bd3ffe8b19a604421a5d461d4a70346223e535903fbc3067138bddbebddcf77" +dependencies = [ + "serde", + "thiserror", +] + [[package]] name = "bhyve_api" version = "0.0.0" @@ -3098,6 +3108,12 @@ version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6fe2267d4ed49bc07b63801559be28c718ea06c4738b7a03c94df7386d2cde46" +[[package]] +name = "highway" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4ba82c000837f4e74df01a5520f0dc48735d4aed955a99eae4428bab7cf3acd" + [[package]] name = "hkdf" version = "0.12.3" @@ -5732,6 +5748,7 @@ dependencies = [ "rstest", "schemars", "serde", + "strum", "thiserror", "trybuild", "uuid", @@ -5810,10 +5827,13 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", + "bcs", "bytes", "chrono", "clap 4.4.3", "dropshot", + "expectorate", + "highway", "itertools 0.11.0", "omicron-test-utils", "omicron-workspace-hack", @@ -5827,6 +5847,7 @@ dependencies = [ "slog-async", "slog-dtrace", "slog-term", + "strum", "thiserror", "tokio", "usdt", diff --git a/Cargo.toml b/Cargo.toml index 7521bb4d45..5c10a94706 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -199,6 +199,7 @@ headers = "0.3.9" heck = "0.4" hex = "0.4.3" hex-literal = "0.4.1" +highway = "1.1.0" hkdf = "0.12.3" http = "0.2.9" httptest = "0.15.4" diff --git a/oximeter/db/Cargo.toml b/oximeter/db/Cargo.toml index ad6d584b1b..d37c57ccce 100644 --- a/oximeter/db/Cargo.toml +++ b/oximeter/db/Cargo.toml @@ -8,10 +8,12 @@ license = "MPL-2.0" [dependencies] anyhow.workspace = true async-trait.workspace = true +bcs.workspace = true bytes = { workspace = true, features = [ "serde" ] } chrono.workspace = true clap.workspace = true dropshot.workspace = true +highway.workspace = true oximeter.workspace = true regex.workspace = true reqwest = { workspace = true, features = [ "json" ] } @@ -28,9 +30,11 @@ uuid.workspace = true omicron-workspace-hack.workspace = true [dev-dependencies] +expectorate.workspace = true itertools.workspace = true omicron-test-utils.workspace = true slog-dtrace.workspace = true +strum.workspace = true [[bin]] name = "oxdb" diff --git a/oximeter/db/src/lib.rs b/oximeter/db/src/lib.rs index 9720d6914d..c878b8ff2a 100644 --- a/oximeter/db/src/lib.rs +++ b/oximeter/db/src/lib.rs @@ -328,19 +328,46 @@ pub struct TimeseriesPageSelector { pub(crate) type TimeseriesKey = u64; pub(crate) fn timeseries_key(sample: &Sample) -> TimeseriesKey { - timeseries_key_for(sample.target_fields(), sample.metric_fields()) + timeseries_key_for( + &sample.timeseries_name, + sample.sorted_target_fields(), + sample.sorted_metric_fields(), + sample.measurement.datum_type(), + ) } -pub(crate) fn timeseries_key_for<'a>( - target_fields: impl Iterator, - metric_fields: impl Iterator, +// It's critical that values used for derivation of the timeseries_key are stable. +// We use "bcs" to ensure stability of the derivation across hardware and rust toolchain revisions. +fn canonicalize(what: &str, value: &T) -> Vec { + bcs::to_bytes(value) + .unwrap_or_else(|_| panic!("Failed to serialize {what}")) +} + +fn timeseries_key_for( + timeseries_name: &str, + target_fields: &BTreeMap, + metric_fields: &BTreeMap, + datum_type: DatumType, ) -> TimeseriesKey { - use std::collections::hash_map::DefaultHasher; + // We use HighwayHasher primarily for stability - it should provide a stable + // hash for the values used to derive the timeseries_key. + use highway::HighwayHasher; use std::hash::{Hash, Hasher}; - let mut hasher = DefaultHasher::new(); - for field in target_fields.chain(metric_fields) { - field.hash(&mut hasher); + + // NOTE: The order of these ".hash" calls matters, changing them will change + // the derivation of the "timeseries_key". We have change-detector tests for + // modifications like this, but be cautious, making such a change will + // impact all currently-provisioned databases. + let mut hasher = HighwayHasher::default(); + canonicalize("timeseries name", timeseries_name).hash(&mut hasher); + for field in target_fields.values() { + canonicalize("target field", &field).hash(&mut hasher); } + for field in metric_fields.values() { + canonicalize("metric field", &field).hash(&mut hasher); + } + canonicalize("datum type", &datum_type).hash(&mut hasher); + hasher.finish() } @@ -370,8 +397,9 @@ const TIMESERIES_NAME_REGEX: &str = #[cfg(test)] mod tests { - use super::TimeseriesName; + use super::*; use std::convert::TryFrom; + use uuid::Uuid; #[test] fn test_timeseries_name() { @@ -393,4 +421,88 @@ mod tests { assert!(TimeseriesName::try_from("a:").is_err()); assert!(TimeseriesName::try_from("123").is_err()); } + + // Validates that the timeseries_key stability for a sample is stable. + #[test] + fn test_timeseries_key_sample_stability() { + #[derive(oximeter::Target)] + pub struct TestTarget { + pub name: String, + pub num: i64, + } + + #[derive(oximeter::Metric)] + pub struct TestMetric { + pub id: Uuid, + pub datum: i64, + } + + let target = TestTarget { name: String::from("Hello"), num: 1337 }; + let metric = TestMetric { id: Uuid::nil(), datum: 0x1de }; + let sample = Sample::new(&target, &metric).unwrap(); + let key = super::timeseries_key(&sample); + + expectorate::assert_contents( + "test-output/sample-timeseries-key.txt", + &format!("{key}"), + ); + } + + // Validates that the timeseries_key stability for specific fields is + // stable. + #[test] + fn test_timeseries_key_field_stability() { + use oximeter::{Field, FieldValue}; + use strum::EnumCount; + + let values = [ + ("string", FieldValue::String(String::default())), + ("i8", FieldValue::I8(-0x0A)), + ("u8", FieldValue::U8(0x0A)), + ("i16", FieldValue::I16(-0x0ABC)), + ("u16", FieldValue::U16(0x0ABC)), + ("i32", FieldValue::I32(-0x0ABC_0000)), + ("u32", FieldValue::U32(0x0ABC_0000)), + ("i64", FieldValue::I64(-0x0ABC_0000_0000_0000)), + ("u64", FieldValue::U64(0x0ABC_0000_0000_0000)), + ( + "ipaddr", + FieldValue::IpAddr(std::net::IpAddr::V4( + std::net::Ipv4Addr::LOCALHOST, + )), + ), + ("uuid", FieldValue::Uuid(uuid::Uuid::nil())), + ("bool", FieldValue::Bool(true)), + ]; + + // Exhaustively testing enums is a bit tricky. Although it's easy to + // check "all variants of an enum are matched", it harder to test "all + // variants of an enum have been supplied". + // + // We use this as a proxy, confirming that each variant is represented + // here for the purposes of tracking stability. + assert_eq!(values.len(), FieldValue::COUNT); + + let mut output = vec![]; + for (name, value) in values { + let target_fields = BTreeMap::from([( + "field".to_string(), + Field { name: name.to_string(), value }, + )]); + let metric_fields = BTreeMap::new(); + let key = timeseries_key_for( + "timeseries name", + &target_fields, + &metric_fields, + // ... Not actually, but we are only trying to compare fields here. + DatumType::Bool, + ); + output.push(format!("{name} -> {key}")); + } + + expectorate::assert_contents( + "test-output/field-timeseries-keys.txt", + &output.join("\n"), + ); + } } diff --git a/oximeter/db/test-output/field-timeseries-keys.txt b/oximeter/db/test-output/field-timeseries-keys.txt new file mode 100644 index 0000000000..d82c143600 --- /dev/null +++ b/oximeter/db/test-output/field-timeseries-keys.txt @@ -0,0 +1,12 @@ +string -> 5554437373902071418 +i8 -> 8051527130089763326 +u8 -> 1403385090410880239 +i16 -> 4425083437960417917 +u16 -> 13883626507745758865 +i32 -> 14729289749324644435 +u32 -> 12103188004421096629 +i64 -> 961258395613152243 +u64 -> 15804125619400967189 +ipaddr -> 14737150884237616680 +uuid -> 16911606541498230091 +bool -> 10983724023695040909 \ No newline at end of file diff --git a/oximeter/db/test-output/sample-timeseries-key.txt b/oximeter/db/test-output/sample-timeseries-key.txt new file mode 100644 index 0000000000..aeb515c78e --- /dev/null +++ b/oximeter/db/test-output/sample-timeseries-key.txt @@ -0,0 +1 @@ +365003276793586811 \ No newline at end of file diff --git a/oximeter/oximeter/Cargo.toml b/oximeter/oximeter/Cargo.toml index 7d01b8f8be..8a69494d5a 100644 --- a/oximeter/oximeter/Cargo.toml +++ b/oximeter/oximeter/Cargo.toml @@ -13,6 +13,7 @@ omicron-common.workspace = true oximeter-macro-impl.workspace = true schemars = { workspace = true, features = [ "uuid1", "bytes", "chrono" ] } serde.workspace = true +strum.workspace = true thiserror.workspace = true uuid.workspace = true omicron-workspace-hack.workspace = true diff --git a/oximeter/oximeter/src/types.rs b/oximeter/oximeter/src/types.rs index aa61a426e3..d3f1b9e746 100644 --- a/oximeter/oximeter/src/types.rs +++ b/oximeter/oximeter/src/types.rs @@ -90,7 +90,15 @@ impl_field_type_from! { bool, FieldType::Bool } /// The `FieldValue` contains the value of a target or metric field. #[derive( - Clone, Debug, Hash, PartialEq, Eq, JsonSchema, Serialize, Deserialize, + Clone, + Debug, + Hash, + PartialEq, + Eq, + JsonSchema, + Serialize, + Deserialize, + strum::EnumCount, )] #[serde(tag = "type", content = "value", rename_all = "snake_case")] pub enum FieldValue { @@ -761,6 +769,11 @@ impl Sample { self.target.fields.values() } + /// Return the sorted fields of this sample's target. + pub fn sorted_target_fields(&self) -> &BTreeMap { + &self.target.fields + } + /// Return the name of this sample's metric. pub fn metric_name(&self) -> &str { &self.metric.name @@ -771,6 +784,11 @@ impl Sample { self.metric.fields.values() } + /// Return the sorted fields of this sample's metric + pub fn sorted_metric_fields(&self) -> &BTreeMap { + &self.metric.fields + } + // Check validity of field names for the target and metric. Currently this // just verifies there are no duplicate names between them. fn verify_field_names(