From 6d7e6ecb8c19fe379dca6d31e0d134464bd00473 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Wed, 15 May 2024 01:09:51 +0000 Subject: [PATCH] Remove unused `base_route` field from oximeter producer info - Remove from DB schema, add migration to drop the column - Remove all references, except in the timeseries schema reporting oximeter self-statistics. That needs to wait for functionality to update timeseries schema. - Fixes #5658 --- clients/nexus-client/src/lib.rs | 2 -- clients/oximeter-client/src/lib.rs | 1 - common/src/api/internal/nexus.rs | 10 ---------- dev-tools/omdb/src/bin/omdb/oximeter.rs | 2 -- nexus/db-model/src/producer_endpoint.rs | 8 -------- nexus/db-model/src/schema.rs | 1 - nexus/db-model/src/schema_versions.rs | 3 ++- nexus/db-queries/src/db/datastore/oximeter.rs | 2 -- nexus/metrics-producer-gc/src/lib.rs | 2 -- .../src/app/background/metrics_producer_gc.rs | 1 - nexus/src/lib.rs | 2 -- nexus/test-utils/src/lib.rs | 1 - openapi/nexus-internal.json | 5 ----- openapi/oximeter.json | 5 ----- oximeter/collector/src/agent.rs | 19 +++---------------- oximeter/collector/src/self_stats.rs | 14 +++++++++++--- oximeter/producer/examples/producer.rs | 1 - oximeter/producer/src/lib.rs | 10 ---------- schema/crdb/dbinit.sql | 4 +--- sled-agent/src/metrics.rs | 1 - sled-agent/src/sim/disk.rs | 1 - 21 files changed, 17 insertions(+), 78 deletions(-) diff --git a/clients/nexus-client/src/lib.rs b/clients/nexus-client/src/lib.rs index 92cc3ff27e..ae8f0c93db 100644 --- a/clients/nexus-client/src/lib.rs +++ b/clients/nexus-client/src/lib.rs @@ -232,7 +232,6 @@ impl From<&omicron_common::api::internal::nexus::ProducerEndpoint> ) -> Self { Self { address: s.address.to_string(), - base_route: s.base_route.clone(), id: s.id, kind: s.kind.into(), interval: s.interval.into(), @@ -415,7 +414,6 @@ impl TryFrom id: ep.id, kind: ep.kind.into(), address, - base_route: ep.base_route, interval: ep.interval.into(), }) } diff --git a/clients/oximeter-client/src/lib.rs b/clients/oximeter-client/src/lib.rs index 11aa1452f8..74fc6968e8 100644 --- a/clients/oximeter-client/src/lib.rs +++ b/clients/oximeter-client/src/lib.rs @@ -41,7 +41,6 @@ impl From<&omicron_common::api::internal::nexus::ProducerEndpoint> ) -> Self { Self { address: s.address.to_string(), - base_route: s.base_route.clone(), id: s.id, kind: s.kind.into(), interval: s.interval.into(), diff --git a/common/src/api/internal/nexus.rs b/common/src/api/internal/nexus.rs index 5a44921c26..20516e702b 100644 --- a/common/src/api/internal/nexus.rs +++ b/common/src/api/internal/nexus.rs @@ -110,20 +110,10 @@ pub struct ProducerEndpoint { /// The IP address and port at which `oximeter` can collect metrics from the /// producer. pub address: SocketAddr, - /// NOTE: This field is deprecated, and will be ignored. It will be removed - /// in future releases. - pub base_route: String, /// The interval on which `oximeter` should collect metrics. pub interval: Duration, } -impl ProducerEndpoint { - /// Return the route that can be used to request metric data. - pub fn collection_route(&self) -> String { - format!("{}/{}", &self.base_route, &self.id) - } -} - /// Response to a successful producer registration. #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] pub struct ProducerRegistrationResponse { diff --git a/dev-tools/omdb/src/bin/omdb/oximeter.rs b/dev-tools/omdb/src/bin/omdb/oximeter.rs index 573a91a093..a6dc2ce011 100644 --- a/dev-tools/omdb/src/bin/omdb/oximeter.rs +++ b/dev-tools/omdb/src/bin/omdb/oximeter.rs @@ -90,7 +90,6 @@ impl OximeterArgs { struct Producer { id: Uuid, address: SocketAddr, - base_route: String, interval: String, } @@ -100,7 +99,6 @@ impl From for Producer { Self { id: p.id, address: p.address.parse().unwrap(), - base_route: p.base_route, interval: humantime::format_duration(interval).to_string(), } } diff --git a/nexus/db-model/src/producer_endpoint.rs b/nexus/db-model/src/producer_endpoint.rs index aea087360b..74a7356adb 100644 --- a/nexus/db-model/src/producer_endpoint.rs +++ b/nexus/db-model/src/producer_endpoint.rs @@ -53,7 +53,6 @@ impl From for internal::nexus::ProducerEndpoint { id: ep.id(), kind: ep.kind.into(), address: SocketAddr::new(ep.ip.ip(), *ep.port), - base_route: ep.base_route.clone(), interval: Duration::from_secs_f64(ep.interval), } } @@ -71,7 +70,6 @@ pub struct ProducerEndpoint { pub ip: ipnetwork::IpNetwork, pub port: SqlU16, pub interval: f64, - pub base_route: String, pub oximeter_id: Uuid, } @@ -87,14 +85,8 @@ impl ProducerEndpoint { kind: endpoint.kind.into(), ip: endpoint.address.ip().into(), port: endpoint.address.port().into(), - base_route: endpoint.base_route.clone(), interval: endpoint.interval.as_secs_f64(), oximeter_id, } } - - /// Return the route that can be used to request metric data. - pub fn collection_route(&self) -> String { - format!("{}/{}", &self.base_route, self.id()) - } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 3d16b978f6..224c461da0 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -463,7 +463,6 @@ table! { ip -> Inet, port -> Int4, interval -> Float8, - base_route -> Text, oximeter_id -> Uuid, } } diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index afdf91074e..cb229274fe 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(62, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(63, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(63, "remove-producer-base-route-column"), KnownVersion::new(62, "allocate-subnet-decommissioned-sleds"), KnownVersion::new(61, "blueprint-add-sled-state"), KnownVersion::new(60, "add-lookup-vmm-by-sled-id-index"), diff --git a/nexus/db-queries/src/db/datastore/oximeter.rs b/nexus/db-queries/src/db/datastore/oximeter.rs index 9ac16eafac..1aa3435cb6 100644 --- a/nexus/db-queries/src/db/datastore/oximeter.rs +++ b/nexus/db-queries/src/db/datastore/oximeter.rs @@ -109,7 +109,6 @@ impl DataStore { dsl::ip.eq(producer.ip), dsl::port.eq(producer.port), dsl::interval.eq(producer.interval), - dsl::base_route.eq(producer.base_route.clone()), )) .execute_async(&*self.pool_connection_authorized(opctx).await?) .await @@ -297,7 +296,6 @@ mod tests { id: Uuid::new_v4(), kind: nexus::ProducerKind::Service, address: "[::1]:0".parse().unwrap(), // unused - base_route: "/".to_string(), // unused interval: Duration::from_secs(0), // unused }, collector_info.id, diff --git a/nexus/metrics-producer-gc/src/lib.rs b/nexus/metrics-producer-gc/src/lib.rs index ba2cd0460b..4ed8f1bbb5 100644 --- a/nexus/metrics-producer-gc/src/lib.rs +++ b/nexus/metrics-producer-gc/src/lib.rs @@ -244,7 +244,6 @@ mod tests { id: Uuid::new_v4(), kind: nexus::ProducerKind::Service, address: "[::1]:0".parse().unwrap(), // unused - base_route: "/".to_string(), // unused interval: Duration::from_secs(0), // unused }, collector_info.id, @@ -327,7 +326,6 @@ mod tests { id: Uuid::new_v4(), kind: nexus::ProducerKind::Service, address: "[::1]:0".parse().unwrap(), // unused - base_route: "/".to_string(), // unused interval: Duration::from_secs(0), // unused }, collector_info.id, diff --git a/nexus/src/app/background/metrics_producer_gc.rs b/nexus/src/app/background/metrics_producer_gc.rs index 1e3b070249..ad8a524579 100644 --- a/nexus/src/app/background/metrics_producer_gc.rs +++ b/nexus/src/app/background/metrics_producer_gc.rs @@ -209,7 +209,6 @@ mod tests { id: Uuid::new_v4(), kind: nexus::ProducerKind::Service, address: "[::1]:0".parse().unwrap(), // unused - base_route: "/".to_string(), // unused interval: Duration::from_secs(0), // unused }, collector_info.id, diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index e1b327de91..6a23048693 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -468,8 +468,6 @@ fn start_producer_server( id: registry.producer_id(), kind: ProducerKind::Service, address, - // NOTE: This is now unused, and will be removed in the future. - base_route: String::new(), interval: std::time::Duration::from_secs(10), }, // Some(_) here prevents DNS resolution, using our own address to diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index 8bbb6ef38c..a078ce2a61 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -1468,7 +1468,6 @@ pub fn start_producer_server( id, kind: ProducerKind::Service, address: producer_address, - base_route: String::new(), // Unused, will be removed. interval: Duration::from_secs(1), }; let config = oximeter_producer::Config { diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index d694e5ee0d..5ec8e58417 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -3894,10 +3894,6 @@ "description": "The IP address and port at which `oximeter` can collect metrics from the producer.", "type": "string" }, - "base_route": { - "description": "NOTE: This field is deprecated, and will be ignored. It will be removed in future releases.", - "type": "string" - }, "id": { "description": "A unique ID for this producer.", "type": "string", @@ -3922,7 +3918,6 @@ }, "required": [ "address", - "base_route", "id", "interval", "kind" diff --git a/openapi/oximeter.json b/openapi/oximeter.json index 3fa58426c3..c567e9421d 100644 --- a/openapi/oximeter.json +++ b/openapi/oximeter.json @@ -200,10 +200,6 @@ "description": "The IP address and port at which `oximeter` can collect metrics from the producer.", "type": "string" }, - "base_route": { - "description": "NOTE: This field is deprecated, and will be ignored. It will be removed in future releases.", - "type": "string" - }, "id": { "description": "A unique ID for this producer.", "type": "string", @@ -228,7 +224,6 @@ }, "required": [ "address", - "base_route", "id", "interval", "kind" diff --git a/oximeter/collector/src/agent.rs b/oximeter/collector/src/agent.rs index a86b528cd5..5da9a1dfa8 100644 --- a/oximeter/collector/src/agent.rs +++ b/oximeter/collector/src/agent.rs @@ -80,11 +80,7 @@ async fn perform_collection( ) { debug!(log, "collecting from producer"); let res = client - .get(format!( - "http://{}{}", - producer.address, - producer.collection_route() - )) + .get(format!("http://{}/{}", producer.address, producer.id,)) .send() .await; match res { @@ -155,10 +151,7 @@ async fn collection_task( mut inbox: mpsc::Receiver, outbox: mpsc::Sender<(Option, ProducerResults)>, ) { - let mut log = orig_log.new(o!( - "route" => producer.collection_route(), - "address" => producer.address, - )); + let mut log = orig_log.new(o!("address" => producer.address)); let client = reqwest::Client::new(); let mut collection_timer = interval(producer.interval); collection_timer.tick().await; // completes immediately @@ -199,10 +192,7 @@ async fn collection_task( ); // Update the logger with the new information as well. - log = orig_log.new(o!( - "route" => producer.collection_route(), - "address" => producer.address, - )); + log = orig_log.new(o!("address" => producer.address)); collection_timer = interval(producer.interval); collection_timer.tick().await; // completes immediately } @@ -940,7 +930,6 @@ mod tests { id: Uuid::new_v4(), kind: ProducerKind::Service, address, - base_route: String::from("/"), interval: COLLECTION_INTERVAL, }; collector @@ -1009,7 +998,6 @@ mod tests { 0, 0, )), - base_route: String::from("/"), interval: COLLECTION_INTERVAL, }; collector @@ -1089,7 +1077,6 @@ mod tests { id: Uuid::new_v4(), kind: ProducerKind::Service, address, - base_route: String::from("/"), interval: COLLECTION_INTERVAL, }; collector diff --git a/oximeter/collector/src/self_stats.rs b/oximeter/collector/src/self_stats.rs index b72c21d0e3..ab9e5bedf4 100644 --- a/oximeter/collector/src/self_stats.rs +++ b/oximeter/collector/src/self_stats.rs @@ -46,6 +46,10 @@ pub struct Collections { /// The base route in the producer server used to collect metrics. /// /// The full route is `{base_route}/{producer_id}`. + /// + // TODO-cleanup: This is no longer relevant, but removing it entirely + // relies on nonexistent functionality for updating timeseries schema. When + // that lands, we should remove this. pub base_route: String, pub datum: Cumulative, } @@ -98,6 +102,10 @@ pub struct FailedCollections { /// The base route in the producer server used to collect metrics. /// /// The full route is `{base_route}/{producer_id}`. + /// + // TODO-cleanup: This is no longer relevant, but removing it entirely + // relies on nonexistent functionality for updating timeseries schema. When + // that lands, we should remove this. pub base_route: String, /// The reason we could not collect. // @@ -125,7 +133,7 @@ impl CollectionTaskStats { producer_id: producer.id, producer_ip: producer.address.ip(), producer_port: producer.address.port(), - base_route: producer.base_route.clone(), + base_route: String::new(), datum: Cumulative::new(0), }, failed_collections: BTreeMap::new(), @@ -203,7 +211,7 @@ mod tests { producer_id: uuid::uuid!("718452ab-7cca-42f6-b8b1-1aaaa1b09104"), producer_ip: IpAddr::V6(Ipv6Addr::LOCALHOST), producer_port: 12345, - base_route: String::from("/"), + base_route: String::new(), datum: Cumulative::new(0), } } @@ -213,7 +221,7 @@ mod tests { producer_id: uuid::uuid!("718452ab-7cca-42f6-b8b1-1aaaa1b09104"), producer_ip: IpAddr::V6(Ipv6Addr::LOCALHOST), producer_port: 12345, - base_route: String::from("/"), + base_route: String::new(), reason: FailureReason::Unreachable.as_string(), datum: Cumulative::new(0), } diff --git a/oximeter/producer/examples/producer.rs b/oximeter/producer/examples/producer.rs index f5b00526b9..87748dd12d 100644 --- a/oximeter/producer/examples/producer.rs +++ b/oximeter/producer/examples/producer.rs @@ -120,7 +120,6 @@ async fn main() -> anyhow::Result<()> { id: registry.producer_id(), kind: ProducerKind::Service, address: args.address, - base_route: "/collect".to_string(), interval: Duration::from_secs(10), }; let config = Config { diff --git a/oximeter/producer/src/lib.rs b/oximeter/producer/src/lib.rs index 5e9e576b12..6bf8954ae0 100644 --- a/oximeter/producer/src/lib.rs +++ b/oximeter/producer/src/lib.rs @@ -210,14 +210,6 @@ impl Server { return Err(Error::UuidMismatch); } - // Overwrite any provided base_route. - // - // This will be removed in future releases, as users no longer have or - // need any control over the route the producer server exposes. - // - // TODO-cleanup: Remove this field entirely. - server_info.base_route = String::from("/"); - // Build the logger / server. let log = Self::build_logger(log)?; let dropshot = ConfigDropshot { @@ -284,7 +276,6 @@ impl Server { info!( log, "starting oximeter metric producer server"; - "route" => server_info.collection_route(), "producer_id" => ?registry.producer_id(), "address" => server.local_addr(), "interval" => ?server_info.interval, @@ -542,7 +533,6 @@ mod tests { id: Uuid::new_v4(), kind: ProducerKind::Service, address, - base_route: String::new(), interval: Duration::from_secs(10), }, registration_address: Some(fake_nexus.local_addr()), diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index fa0c74aac2..cc298e4565 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1294,8 +1294,6 @@ CREATE TABLE IF NOT EXISTS omicron.public.metric_producer ( ip INET NOT NULL, port INT4 CHECK (port BETWEEN 0 AND 65535) NOT NULL, interval FLOAT NOT NULL, - /* TODO: Is this length appropriate? */ - base_route STRING(512) NOT NULL, /* Oximeter collector instance to which this metric producer is assigned. */ oximeter_id UUID NOT NULL ); @@ -3861,7 +3859,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '62.0.0', NULL) + (TRUE, NOW(), NOW(), '63.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/sled-agent/src/metrics.rs b/sled-agent/src/metrics.rs index 64c5a806b4..62eaaf6154 100644 --- a/sled-agent/src/metrics.rs +++ b/sled-agent/src/metrics.rs @@ -169,7 +169,6 @@ fn start_producer_server( id: registry.producer_id(), kind: ProducerKind::SledAgent, address, - base_route: String::new(), // Unused, will be removed. interval: METRIC_COLLECTION_INTERVAL, }, registration_address, diff --git a/sled-agent/src/sim/disk.rs b/sled-agent/src/sim/disk.rs index 6e51e21311..284e424ebf 100644 --- a/sled-agent/src/sim/disk.rs +++ b/sled-agent/src/sim/disk.rs @@ -170,7 +170,6 @@ impl SimDisk { id, kind: ProducerKind::SledAgent, address: producer_address, - base_route: String::new(), // Unused, will be removed. interval: Duration::from_millis(200), }; let config = oximeter_producer::Config {