Make ClickhouseCli.log a mandatory field

oxidecomputer · Nov 26, 2024 · 1c282f9 · 1c282f9
1 parent 6c5c0f4
commit 1c282f9
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 58 deletions.
diff --git a/clickhouse-admin/src/bin/clickhouse-admin-keeper.rs b/clickhouse-admin/src/bin/clickhouse-admin-keeper.rs
@@ -8,7 +8,7 @@
 use anyhow::anyhow;
 use camino::Utf8PathBuf;
 use clap::Parser;
-use omicron_clickhouse_admin::{ClickhouseCli, Clickward, Config};
+use omicron_clickhouse_admin::{Clickward, Config};
 use omicron_common::cmd::fatal;
 use omicron_common::cmd::CmdError;
 use std::net::{SocketAddr, SocketAddrV6};
@@ -55,12 +55,10 @@ async fn main_impl() -> Result<(), CmdError> {
                 .map_err(|err| CmdError::Failure(anyhow!(err)))?;
             config.dropshot.bind_address = SocketAddr::V6(http_address);
             let clickward = Clickward::new();
-            let clickhouse_cli =
-                ClickhouseCli::new(binary_path, listen_address);
-
             let server = omicron_clickhouse_admin::start_keeper_admin_server(
                 clickward,
-                clickhouse_cli,
+                binary_path,
+                listen_address,
                 config,
             )
             .await

diff --git a/clickhouse-admin/src/bin/clickhouse-admin-server.rs b/clickhouse-admin/src/bin/clickhouse-admin-server.rs
@@ -8,7 +8,7 @@
 use anyhow::anyhow;
 use camino::Utf8PathBuf;
 use clap::Parser;
-use omicron_clickhouse_admin::{ClickhouseCli, Clickward, Config};
+use omicron_clickhouse_admin::{Clickward, Config};
 use omicron_common::cmd::fatal;
 use omicron_common::cmd::CmdError;
 use std::net::{SocketAddr, SocketAddrV6};
@@ -55,12 +55,10 @@ async fn main_impl() -> Result<(), CmdError> {
                 .map_err(|err| CmdError::Failure(anyhow!(err)))?;
             config.dropshot.bind_address = SocketAddr::V6(http_address);
             let clickward = Clickward::new();
-            let clickhouse_cli =
-                ClickhouseCli::new(binary_path, listen_address);
-
             let server = omicron_clickhouse_admin::start_server_admin_server(
                 clickward,
-                clickhouse_cli,
+                binary_path,
+                listen_address,
                 config,
             )
             .await

diff --git a/clickhouse-admin/src/bin/clickhouse-admin-single.rs b/clickhouse-admin/src/bin/clickhouse-admin-single.rs
@@ -7,7 +7,7 @@
 use anyhow::anyhow;
 use camino::Utf8PathBuf;
 use clap::Parser;
-use omicron_clickhouse_admin::{ClickhouseCli, Config};
+use omicron_clickhouse_admin::Config;
 use omicron_common::cmd::fatal;
 use omicron_common::cmd::CmdError;
 use std::net::{SocketAddr, SocketAddrV6};
@@ -53,11 +53,9 @@ async fn main_impl() -> Result<(), CmdError> {
             let mut config = Config::from_file(&config)
                 .map_err(|err| CmdError::Failure(anyhow!(err)))?;
             config.dropshot.bind_address = SocketAddr::V6(http_address);
-            let clickhouse_cli =
-                ClickhouseCli::new(binary_path, listen_address);
-
             let server = omicron_clickhouse_admin::start_single_admin_server(
-                clickhouse_cli,
+                binary_path,
+                listen_address,
                 config,
             )
             .await

diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs
@@ -84,19 +84,18 @@ impl Display for ClickhouseClientType {
 pub struct ClickhouseCli {
     /// Path to where the clickhouse binary is located
     pub binary_path: Utf8PathBuf,
-    /// Address on where the clickhouse keeper is listening on
+    /// Address at which the clickhouse keeper/server is listening
     pub listen_address: SocketAddrV6,
-    pub log: Option<Logger>,
+    pub log: Logger,
 }
 
 impl ClickhouseCli {
-    pub fn new(binary_path: Utf8PathBuf, listen_address: SocketAddrV6) -> Self {
-        Self { binary_path, listen_address, log: None }
-    }
-
-    pub fn with_log(mut self, log: Logger) -> Self {
-        self.log = Some(log);
-        self
+    pub fn new(
+        binary_path: Utf8PathBuf,
+        listen_address: SocketAddrV6,
+        log: Logger,
+    ) -> Self {
+        Self { binary_path, listen_address, log }
     }
 
     pub async fn lgif(&self) -> Result<Lgif, ClickhouseCliError> {
@@ -105,7 +104,7 @@ impl ClickhouseCli {
             "lgif",
             "Retrieve logically grouped information file",
             Lgif::parse,
-            self.log.clone().unwrap(),
+            self.log.clone(),
         )
         .await
     }
@@ -116,7 +115,7 @@ impl ClickhouseCli {
             "get /keeper/config",
             "Retrieve raft configuration information",
             RaftConfig::parse,
-            self.log.clone().unwrap(),
+            self.log.clone(),
         )
         .await
     }
@@ -127,7 +126,7 @@ impl ClickhouseCli {
             "conf",
             "Retrieve keeper node configuration information",
             KeeperConf::parse,
-            self.log.clone().unwrap(),
+            self.log.clone(),
         )
         .await
     }
@@ -163,7 +162,7 @@ impl ClickhouseCli {
             "Retrieve information about distributed ddl queries (ON CLUSTER clause) 
             that were executed on a cluster",
             DistributedDdlQueue::parse,
-            self.log.clone().unwrap(),
+            self.log.clone(),
         )
         .await
     }
@@ -172,7 +171,7 @@ impl ClickhouseCli {
         &self,
         settings: SystemTimeSeriesSettings,
     ) -> Result<Vec<SystemTimeSeries>, ClickhouseCliError> {
-        let log = self.log.clone().unwrap();
+        let log = self.log.clone();
         let query = settings.query_avg();
 
         debug!(&log, "Querying system database"; "query" => &query);

diff --git a/clickhouse-admin/src/context.rs b/clickhouse-admin/src/context.rs
@@ -17,12 +17,10 @@ pub struct ServerContext {
 }
 
 impl ServerContext {
-    pub fn new(
-        clickward: Clickward,
-        clickhouse_cli: ClickhouseCli,
-        _log: Logger,
-    ) -> Self {
-        Self { clickward, clickhouse_cli, _log }
+    pub fn new(clickward: Clickward, clickhouse_cli: ClickhouseCli) -> Self {
+        let log =
+            clickhouse_cli.log.new(slog::o!("component" => "ServerContext"));
+        Self { clickward, clickhouse_cli, _log: log }
     }
 
     pub fn clickward(&self) -> &Clickward {
@@ -44,11 +42,8 @@ impl SingleServerContext {
     pub fn new(clickhouse_cli: ClickhouseCli) -> Self {
         let ip = clickhouse_cli.listen_address.ip();
         let address = SocketAddrV6::new(*ip, CLICKHOUSE_TCP_PORT, 0, 0);
-        let log = clickhouse_cli
-            .log
-            .as_ref()
-            .expect("should have configured logging via CLI or config");
-        let oximeter_client = OximeterClient::new(address.into(), log);
+        let oximeter_client =
+            OximeterClient::new(address.into(), &clickhouse_cli.log);
         Self {
             clickhouse_cli,
             oximeter_client,

diff --git a/clickhouse-admin/src/lib.rs b/clickhouse-admin/src/lib.rs
@@ -2,13 +2,15 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
+use camino::Utf8PathBuf;
 use context::{ServerContext, SingleServerContext};
 use dropshot::HttpServer;
 use omicron_common::FileKv;
 use slog::{debug, error, Drain};
 use slog_dtrace::ProbeRegistration;
 use slog_error_chain::SlogInlineError;
 use std::io;
+use std::net::SocketAddrV6;
 use std::sync::Arc;
 
 mod clickhouse_cli;
@@ -35,7 +37,8 @@ pub enum StartError {
 /// manages clickhouse replica servers.
 pub async fn start_server_admin_server(
     clickward: Clickward,
-    clickhouse_cli: ClickhouseCli,
+    binary_path: Utf8PathBuf,
+    listen_address: SocketAddrV6,
     server_config: Config,
 ) -> Result<HttpServer<Arc<ServerContext>>, StartError> {
     let (drain, registration) = slog_dtrace::with_drain(
@@ -56,13 +59,12 @@ pub async fn start_server_admin_server(
         }
     }
 
-    let context = ServerContext::new(
-        clickward,
-        clickhouse_cli
-            .with_log(log.new(slog::o!("component" => "ClickhouseCli"))),
-        log.new(slog::o!("component" => "ServerContext")),
+    let clickhouse_cli = ClickhouseCli::new(
+        binary_path,
+        listen_address,
+        log.new(slog::o!("component" => "ClickhouseCli")),
     );
-
+    let context = ServerContext::new(clickward, clickhouse_cli);
     dropshot::ServerBuilder::new(
         http_entrypoints::clickhouse_admin_server_api(),
         Arc::new(context),
@@ -77,7 +79,8 @@ pub async fn start_server_admin_server(
 /// manages clickhouse replica servers.
 pub async fn start_keeper_admin_server(
     clickward: Clickward,
-    clickhouse_cli: ClickhouseCli,
+    binary_path: Utf8PathBuf,
+    listen_address: SocketAddrV6,
     server_config: Config,
 ) -> Result<HttpServer<Arc<ServerContext>>, StartError> {
     let (drain, registration) = slog_dtrace::with_drain(
@@ -98,13 +101,12 @@ pub async fn start_keeper_admin_server(
         }
     }
 
-    let context = ServerContext::new(
-        clickward,
-        clickhouse_cli
-            .with_log(log.new(slog::o!("component" => "ClickhouseCli"))),
-        log.new(slog::o!("component" => "ServerContext")),
+    let clickhouse_cli = ClickhouseCli::new(
+        binary_path,
+        listen_address,
+        log.new(slog::o!("component" => "ClickhouseCli")),
     );
-
+    let context = ServerContext::new(clickward, clickhouse_cli);
     dropshot::ServerBuilder::new(
         http_entrypoints::clickhouse_admin_keeper_api(),
         Arc::new(context),
@@ -118,7 +120,8 @@ pub async fn start_keeper_admin_server(
 /// Start the dropshot server for `clickhouse-admin-single` which
 /// manages a single-node ClickHouse database.
 pub async fn start_single_admin_server(
-    clickhouse_cli: ClickhouseCli,
+    binary_path: Utf8PathBuf,
+    listen_address: SocketAddrV6,
     server_config: Config,
 ) -> Result<HttpServer<Arc<SingleServerContext>>, StartError> {
     let (drain, registration) = slog_dtrace::with_drain(
@@ -139,10 +142,12 @@ pub async fn start_single_admin_server(
         }
     }
 
-    let context = SingleServerContext::new(
-        clickhouse_cli
-            .with_log(log.new(slog::o!("component" => "ClickhouseCli"))),
+    let clickhouse_cli = ClickhouseCli::new(
+        binary_path,
+        listen_address,
+        log.new(slog::o!("component" => "ClickhouseCli")),
     );
+    let context = SingleServerContext::new(clickhouse_cli);
     dropshot::ServerBuilder::new(
         http_entrypoints::clickhouse_admin_single_api(),
         Arc::new(context),