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

Remove unused base_route field from oximeter producer info #5769

Merged
merged 2 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions clients/nexus-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -415,7 +414,6 @@ impl TryFrom<types::ProducerEndpoint>
id: ep.id,
kind: ep.kind.into(),
address,
base_route: ep.base_route,
interval: ep.interval.into(),
})
}
Expand Down
1 change: 0 additions & 1 deletion clients/oximeter-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
10 changes: 0 additions & 10 deletions common/src/api/internal/nexus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 0 additions & 2 deletions dev-tools/omdb/src/bin/omdb/oximeter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ impl OximeterArgs {
struct Producer {
id: Uuid,
address: SocketAddr,
base_route: String,
interval: String,
}

Expand All @@ -100,7 +99,6 @@ impl From<ProducerEndpoint> for Producer {
Self {
id: p.id,
address: p.address.parse().unwrap(),
base_route: p.base_route,
interval: humantime::format_duration(interval).to_string(),
}
}
Expand Down
8 changes: 0 additions & 8 deletions nexus/db-model/src/producer_endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ impl From<ProducerEndpoint> 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),
}
}
Expand All @@ -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,
}

Expand All @@ -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())
}
}
1 change: 0 additions & 1 deletion nexus/db-model/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,6 @@ table! {
ip -> Inet,
port -> Int4,
interval -> Float8,
base_route -> Text,
oximeter_id -> Uuid,
}
}
Expand Down
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand All @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = 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"),
bnaecker marked this conversation as resolved.
Show resolved Hide resolved
KnownVersion::new(62, "allocate-subnet-decommissioned-sleds"),
KnownVersion::new(61, "blueprint-add-sled-state"),
KnownVersion::new(60, "add-lookup-vmm-by-sled-id-index"),
Expand Down
2 changes: 0 additions & 2 deletions nexus/db-queries/src/db/datastore/oximeter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 0 additions & 2 deletions nexus/metrics-producer-gc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
1 change: 0 additions & 1 deletion nexus/src/app/background/metrics_producer_gc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 0 additions & 2 deletions nexus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion nexus/test-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 0 additions & 5 deletions openapi/nexus-internal.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -3922,7 +3918,6 @@
},
"required": [
"address",
"base_route",
"id",
"interval",
"kind"
Expand Down
5 changes: 0 additions & 5 deletions openapi/oximeter.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -228,7 +224,6 @@
},
"required": [
"address",
"base_route",
"id",
"interval",
"kind"
Expand Down
19 changes: 3 additions & 16 deletions oximeter/collector/src/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -155,10 +151,7 @@ async fn collection_task(
mut inbox: mpsc::Receiver<CollectionMessage>,
outbox: mpsc::Sender<(Option<CollectionToken>, 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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -940,7 +930,6 @@ mod tests {
id: Uuid::new_v4(),
kind: ProducerKind::Service,
address,
base_route: String::from("/"),
interval: COLLECTION_INTERVAL,
};
collector
Expand Down Expand Up @@ -1009,7 +998,6 @@ mod tests {
0,
0,
)),
base_route: String::from("/"),
interval: COLLECTION_INTERVAL,
};
collector
Expand Down Expand Up @@ -1089,7 +1077,6 @@ mod tests {
id: Uuid::new_v4(),
kind: ProducerKind::Service,
address,
base_route: String::from("/"),
interval: COLLECTION_INTERVAL,
};
collector
Expand Down
14 changes: 11 additions & 3 deletions oximeter/collector/src/self_stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u64>,
}
Expand Down Expand Up @@ -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.
//
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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),
}
}
Expand All @@ -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),
}
Expand Down
1 change: 0 additions & 1 deletion oximeter/producer/examples/producer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 0 additions & 10 deletions oximeter/producer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()),
Expand Down
4 changes: 1 addition & 3 deletions schema/crdb/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
Expand Down Expand Up @@ -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;
1 change: 0 additions & 1 deletion sled-agent/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 0 additions & 1 deletion sled-agent/src/sim/disk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading