From 5610bb4670582d36d24cf711a601d8b7a63af7a8 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Tue, 18 Jun 2024 23:11:59 +0000 Subject: [PATCH] First round review feedback - Remove serde defaults. These are new fields, and we generate defaults on conversion from the DB model type, so they're not needed. Also remove methods needed for serde defaults. - Add methods for target/metric name of a timeseries schema, rather than tuple indexing. --- oximeter/db/src/lib.rs | 8 ++++---- oximeter/db/src/query.rs | 24 ++++++++++++------------ oximeter/impl/src/schema/codegen.rs | 7 +++---- oximeter/impl/src/schema/ir.rs | 2 +- oximeter/impl/src/schema/mod.rs | 26 +++++++++++--------------- 5 files changed, 31 insertions(+), 36 deletions(-) diff --git a/oximeter/db/src/lib.rs b/oximeter/db/src/lib.rs index e78e546d4ab..f04f1a99f04 100644 --- a/oximeter/db/src/lib.rs +++ b/oximeter/db/src/lib.rs @@ -163,8 +163,8 @@ impl From for TimeseriesSchema { // TODO(ben): Convert from real values once in the DB. description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema: schema.field_schema.into(), datum_type: schema.datum_type.into(), created: schema.created, @@ -414,8 +414,8 @@ mod tests { timeseries_name: timeseries_name.clone(), description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema, datum_type, created: Utc::now(), diff --git a/oximeter/db/src/query.rs b/oximeter/db/src/query.rs index 2eac8f2a12b..4f050ef52f9 100644 --- a/oximeter/db/src/query.rs +++ b/oximeter/db/src/query.rs @@ -779,8 +779,8 @@ mod tests { timeseries_name: TimeseriesName::try_from("foo:bar").unwrap(), description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema: [ FieldSchema { name: "f0".to_string(), @@ -918,8 +918,8 @@ mod tests { timeseries_name: TimeseriesName::try_from("foo:bar").unwrap(), description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema: BTreeSet::new(), datum_type: DatumType::I64, created: Utc::now(), @@ -944,8 +944,8 @@ mod tests { timeseries_name: TimeseriesName::try_from("foo:bar").unwrap(), description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema: BTreeSet::new(), datum_type: DatumType::I64, created: Utc::now(), @@ -1018,8 +1018,8 @@ mod tests { timeseries_name: TimeseriesName::try_from("foo:bar").unwrap(), description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema: [ FieldSchema { name: "f0".to_string(), @@ -1087,8 +1087,8 @@ mod tests { timeseries_name: TimeseriesName::try_from("foo:bar").unwrap(), description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema: [ FieldSchema { name: "f0".to_string(), @@ -1144,8 +1144,8 @@ mod tests { timeseries_name: TimeseriesName::try_from("foo:bar").unwrap(), description: Default::default(), version: oximeter::schema::default_schema_version(), - authz_scope: oximeter::schema::AuthzScope::fleet(), - units: oximeter::schema::Units::count(), + authz_scope: oximeter::schema::AuthzScope::Fleet, + units: oximeter::schema::Units::Count, field_schema: [ FieldSchema { name: "f0".to_string(), diff --git a/oximeter/impl/src/schema/codegen.rs b/oximeter/impl/src/schema/codegen.rs index f33d1d113c6..3e4785609c5 100644 --- a/oximeter/impl/src/schema/codegen.rs +++ b/oximeter/impl/src/schema/codegen.rs @@ -32,7 +32,7 @@ use quote::quote; pub fn use_timeseries(contents: &str) -> Result { let schema = load_schema(contents)?; let latest = find_schema_version(schema.iter().cloned(), None); - let mod_name = quote::format_ident!("{}", latest[0].component_names().0); + let mod_name = quote::format_ident!("{}", latest[0].target_name()); let types = emit_schema_types(latest); let func = emit_schema_function(schema.into_iter()); Ok(quote! { @@ -75,10 +75,9 @@ fn emit_target(schema: &TimeseriesSchema) -> TokenStream { } fn emit_one(source: FieldSource, schema: &TimeseriesSchema) -> TokenStream { - let names = schema.component_names(); let name = match source { - FieldSource::Target => names.0, - FieldSource::Metric => names.1, + FieldSource::Target => schema.target_name(), + FieldSource::Metric => schema.metric_name(), }; let struct_name = quote::format_ident!("{}", format!("{}", heck::AsPascalCase(name))); diff --git a/oximeter/impl/src/schema/ir.rs b/oximeter/impl/src/schema/ir.rs index 819fc410d7b..89149335edf 100644 --- a/oximeter/impl/src/schema/ir.rs +++ b/oximeter/impl/src/schema/ir.rs @@ -417,7 +417,7 @@ pub(super) fn find_schema_version( None => { let mut last_version = BTreeMap::new(); for schema in list { - let metric_name = schema.component_names().1.to_string(); + let metric_name = schema.metric_name().to_string(); match last_version.entry(metric_name) { Entry::Vacant(entry) => { entry.insert((schema.version, schema.clone())); diff --git a/oximeter/impl/src/schema/mod.rs b/oximeter/impl/src/schema/mod.rs index 031427b8bda..035d05c911b 100644 --- a/oximeter/impl/src/schema/mod.rs +++ b/oximeter/impl/src/schema/mod.rs @@ -43,7 +43,6 @@ pub struct FieldSchema { pub name: String, pub field_type: FieldType, pub source: FieldSource, - #[serde(default)] pub description: String, } @@ -177,13 +176,6 @@ pub enum Units { Bytes, } -impl Units { - /// Construct a `Units::Count`. - pub const fn count() -> Self { - Units::Count - } -} - /// The schema for a timeseries. /// /// This includes the name of the timeseries, as well as the datum type of its metric and the @@ -191,17 +183,11 @@ impl Units { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct TimeseriesSchema { pub timeseries_name: TimeseriesName, - // TODO-cleanup: This default should be removed once schema are tracked in - // CockroachDB. Same for the version and scope fields. - #[serde(default)] pub description: TimeseriesDescription, pub field_schema: BTreeSet, pub datum_type: DatumType, - #[serde(default = "default_schema_version")] pub version: NonZeroU8, - #[serde(default = "AuthzScope::fleet")] pub authz_scope: AuthzScope, - #[serde(default = "Units::count")] pub units: Units, pub created: DateTime, } @@ -240,7 +226,7 @@ impl From<&Sample> for TimeseriesSchema { field_schema, datum_type, version: default_schema_version(), - authz_scope: AuthzScope::fleet(), + authz_scope: AuthzScope::Fleet, units: Units::Count, created: Utc::now(), } @@ -306,6 +292,16 @@ impl TimeseriesSchema { .split_once(':') .expect("Incorrectly formatted timseries name") } + + /// Return the name of the target for this timeseries. + pub fn target_name(&self) -> &str { + self.component_names().0 + } + + /// Return the name of the metric for this timeseries. + pub fn metric_name(&self) -> &str { + self.component_names().1 + } } impl PartialEq for TimeseriesSchema {