From 0cf9d0002d84606b9f990f8c1b75a35a9951701d Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 19 Nov 2024 19:52:36 +1300 Subject: [PATCH 01/16] Skeleton for monitoring endpoints --- clickhouse-admin/src/clickhouse_cli.rs | 22 ++++++++- clickhouse-admin/types/src/lib.rs | 63 ++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index fbdbe46e5f..521fa6412d 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -6,7 +6,7 @@ use anyhow::Result; use camino::Utf8PathBuf; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperId, Lgif, RaftConfig, OXIMETER_CLUSTER, + KeeperId, Lgif, Monitoring, RaftConfig, OXIMETER_CLUSTER, }; use dropshot::HttpError; use illumos_utils::{output_to_exec_error, ExecutionError}; @@ -161,6 +161,26 @@ impl ClickhouseCli { .await } + pub async fn monitoring_queries_per_second_avg( + &self, + ) -> Result, ClickhouseCliError> { + self.client_non_interactive( + ClickhouseClientType::Server, + // TODO: Have this query as a helper function? + "SELECT toStartOfInterval(event_time, INTERVAL 60 SECOND)::INT AS t, avg(ProfileEvent_Query) + FROM system.metric_log + WHERE event_date >= toDate(now() - 86400) AND event_time >= now() - 86400 + GROUP BY t + ORDER BY t WITH FILL STEP 60 + FORMAT JSONEachRow", + "Retrieve information about distributed ddl queries (ON CLUSTER clause) + that were executed on a cluster", + Monitoring::parse, + self.log.clone().unwrap(), + ) + .await + } + async fn client_non_interactive( &self, client: ClickhouseClientType, diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index e563f6da75..262b4b070b 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1033,6 +1033,69 @@ impl DistributedDdlQueue { } } +// TODO: Should I have settings for each system table? +// or should I just add an enum here? +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +pub struct MonitoringSettings { + /// The interval to collect monitoring metrics in seconds. + /// Default is 60 seconds. + pub interval: u64, + /// Range of time to collect monitoring metrics in seconds. + /// Default is 86400 seconds (24 hrs). + pub time_range: u64, + // TODO: Have an enum? + /// Name of the metric to retrieve + pub metric: String, +} + +impl MonitoringSettings { + pub fn query(&self) -> String { + let interval = self.interval; + let time_range = self.time_range; + let metric = &self.metric; + // TODO: Should there be different methods for each system table? + let query = format!("SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric}) AS value + FROM system.metric_log + WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} + GROUP BY time + ORDER BY time WITH FILL STEP {interval} + FORMAT JSONEachRow + SETTINGS date_time_output_format = 'iso'"); + query + } +} + +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +// TODO: Come up with a better name +pub struct Monitoring { + pub time: DateTime, + pub value: u64, + // TODO: Have an enum with possible units? (s, ms, bytes) + // Not sure if I can even add this, the table doesn't mention units at all + // pub unit: String, +} + +impl Monitoring { + pub fn parse(log: &Logger, data: &[u8]) -> Result> { + let s = String::from_utf8_lossy(data); + info!( + log, + // TODO: Should this be per table? Is it necessary? + "Retrieved data from `system.metric_log`"; + "output" => ?s + ); + + let mut m = vec![]; + + for line in s.lines() { + let item: Monitoring = serde_json::from_str(line)?; + m.push(item); + } + + Ok(m) + } +} + #[cfg(test)] mod tests { use camino::Utf8PathBuf; From c19983c0a3e7b17e6e0da6a6eedfc15bb3f338c9 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 20 Nov 2024 15:01:30 +1300 Subject: [PATCH 02/16] functional /timeseries/metric-log/{metric} endpoint --- clickhouse-admin/api/src/lib.rs | 20 ++++-- clickhouse-admin/src/clickhouse_cli.rs | 21 +++---- clickhouse-admin/src/http_entrypoints.rs | 27 ++++++++- clickhouse-admin/types/src/lib.rs | 51 +++++++++++++--- openapi/clickhouse-admin-server.json | 77 ++++++++++++++++++++++++ 5 files changed, 167 insertions(+), 29 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index 398fb30f06..3e9ee914a6 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -4,12 +4,12 @@ use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, RaftConfig, ReplicaConfig, - ServerConfigurableSettings, + KeeperConfig, KeeperConfigurableSettings, Lgif, MetricName, MetricSettings, + RaftConfig, ReplicaConfig, SystemTimeSeries, ServerConfigurableSettings, }; use dropshot::{ - HttpError, HttpResponseCreated, HttpResponseOk, - HttpResponseUpdatedNoContent, RequestContext, TypedBody, + HttpError, HttpResponseCreated, HttpResponseOk, HttpResponseUpdatedNoContent, + Path, Query, RequestContext, TypedBody, }; /// API interface for our clickhouse-admin-keeper server @@ -116,6 +116,18 @@ pub trait ClickhouseAdminServerApi { async fn distributed_ddl_queue( rqctx: RequestContext, ) -> Result>, HttpError>; + + /// Generate a ClickHouse configuration file for a server node on a specified + /// directory and enable the SMF service. + #[endpoint { + method = GET, + path = "/timeseries/metric-log/{metric}" + }] + async fn system_metric_log_timeseries( + rqctx: RequestContext, + path_params: Path, + query_params: Query, + ) -> Result>, HttpError>; } /// API interface for our clickhouse-admin-single server diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index 521fa6412d..61ec44d2ab 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -6,7 +6,8 @@ use anyhow::Result; use camino::Utf8PathBuf; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperId, Lgif, Monitoring, RaftConfig, OXIMETER_CLUSTER, + KeeperId, Lgif, RaftConfig, MetricLogTimeSeriesSettings,SystemTimeSeries, + OXIMETER_CLUSTER, }; use dropshot::HttpError; use illumos_utils::{output_to_exec_error, ExecutionError}; @@ -161,21 +162,15 @@ impl ClickhouseCli { .await } - pub async fn monitoring_queries_per_second_avg( + pub async fn system_metric_log_timeseries( &self, - ) -> Result, ClickhouseCliError> { + settings: MetricLogTimeSeriesSettings + ) -> Result, ClickhouseCliError> { self.client_non_interactive( ClickhouseClientType::Server, - // TODO: Have this query as a helper function? - "SELECT toStartOfInterval(event_time, INTERVAL 60 SECOND)::INT AS t, avg(ProfileEvent_Query) - FROM system.metric_log - WHERE event_date >= toDate(now() - 86400) AND event_time >= now() - 86400 - GROUP BY t - ORDER BY t WITH FILL STEP 60 - FORMAT JSONEachRow", - "Retrieve information about distributed ddl queries (ON CLUSTER clause) - that were executed on a cluster", - Monitoring::parse, + settings.query().as_str(), + "Retrieve time series from the system.metric_log table", + SystemTimeSeries::parse, self.log.clone().unwrap(), ) .await diff --git a/clickhouse-admin/src/http_entrypoints.rs b/clickhouse-admin/src/http_entrypoints.rs index 4380318476..f820e132bc 100644 --- a/clickhouse-admin/src/http_entrypoints.rs +++ b/clickhouse-admin/src/http_entrypoints.rs @@ -6,12 +6,13 @@ use crate::context::{ServerContext, SingleServerContext}; use clickhouse_admin_api::*; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, RaftConfig, ReplicaConfig, - ServerConfigurableSettings, + KeeperConfig, KeeperConfigurableSettings, Lgif, MetricName, MetricSettings, + MetricLogTimeSeriesSettings,RaftConfig, ReplicaConfig, + ServerConfigurableSettings, SystemTimeSeries, }; use dropshot::{ ApiDescription, HttpError, HttpResponseCreated, HttpResponseOk, - HttpResponseUpdatedNoContent, RequestContext, TypedBody, + HttpResponseUpdatedNoContent, Path, Query, RequestContext, TypedBody, }; use illumos_utils::svcadm::Svcadm; use omicron_common::address::CLICKHOUSE_TCP_PORT; @@ -64,6 +65,26 @@ impl ClickhouseAdminServerApi for ClickhouseAdminServerImpl { let output = ctx.clickhouse_cli().distributed_ddl_queue().await?; Ok(HttpResponseOk(output)) } + + async fn system_metric_log_timeseries( + rqctx:RequestContext, + path_params:Path, + query_params:Query, + ) -> Result>, HttpError> { + let ctx = rqctx.context(); + + // TODO: REMOVEME + println!("PATH PARAMS: {path_params:?}"); + println!("QUERY PARAMS: {query_params:?}"); + + let settings = MetricLogTimeSeriesSettings{ + interval: 60, + time_range: 86400, + metric: "ProfileEvent_Query".to_string(), + }; + let output = ctx.clickhouse_cli().system_metric_log_timeseries(settings).await?; + Ok(HttpResponseOk(output)) + } } enum ClickhouseAdminKeeperImpl {} diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index 262b4b070b..bf178547de 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1033,10 +1033,43 @@ impl DistributedDdlQueue { } } +#[inline] +fn default_interval() -> u64 { + 60 +} + +#[inline] +fn default_time_range() -> u64 { + 86400 +} + +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +pub struct MetricName { + // TODO: Have an enum? + /// Name of the metric to retrieve + pub metric: String, +} + +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +pub struct MetricSettings { + /// The interval to collect monitoring metrics in seconds. + /// Default is 60 seconds. + // TODO: How can I actually get the default in the API spec? + #[serde(default = "default_interval")] + pub interval: u64, + /// Range of time to collect monitoring metrics in seconds. + /// Default is 86400 seconds (24 hrs). + #[serde(default = "default_time_range")] + pub time_range: u64, +} + + // TODO: Should I have settings for each system table? // or should I just add an enum here? #[derive(Debug, Serialize, Deserialize, JsonSchema)] -pub struct MonitoringSettings { +pub struct MetricLogTimeSeriesSettings { + // TODO: Use the above structs here + /// The interval to collect monitoring metrics in seconds. /// Default is 60 seconds. pub interval: u64, @@ -1048,7 +1081,7 @@ pub struct MonitoringSettings { pub metric: String, } -impl MonitoringSettings { +impl MetricLogTimeSeriesSettings { pub fn query(&self) -> String { let interval = self.interval; let time_range = self.time_range; @@ -1065,30 +1098,30 @@ impl MonitoringSettings { } } +// TODO: Do the above for AsyncMetricLogTimeSeriesSettings + #[derive(Debug, Serialize, Deserialize, JsonSchema)] -// TODO: Come up with a better name -pub struct Monitoring { +pub struct SystemTimeSeries { pub time: DateTime, - pub value: u64, + pub value: f64, // TODO: Have an enum with possible units? (s, ms, bytes) // Not sure if I can even add this, the table doesn't mention units at all // pub unit: String, } -impl Monitoring { +impl SystemTimeSeries { pub fn parse(log: &Logger, data: &[u8]) -> Result> { let s = String::from_utf8_lossy(data); info!( log, - // TODO: Should this be per table? Is it necessary? - "Retrieved data from `system.metric_log`"; + "Retrieved data from `system` database"; "output" => ?s ); let mut m = vec![]; for line in s.lines() { - let item: Monitoring = serde_json::from_str(line)?; + let item: SystemTimeSeries = serde_json::from_str(line)?; m.push(item); } diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index a42ee25bbb..3fcaa4e364 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -73,6 +73,66 @@ } } } + }, + "/timeseries/metric-log/{metric}": { + "get": { + "summary": "Generate a ClickHouse configuration file for a server node on a specified", + "description": "directory and enable the SMF service.", + "operationId": "system_metric_log_timeseries", + "parameters": [ + { + "in": "path", + "name": "metric", + "description": "Name of the metric to retrieve", + "required": true, + "schema": { + "type": "string" + } + }, + { + "in": "query", + "name": "interval", + "description": "The interval to collect monitoring metrics in seconds. Default is 60 seconds.", + "schema": { + "type": "integer", + "format": "uint64", + "minimum": 0 + } + }, + { + "in": "query", + "name": "time_range", + "description": "Range of time to collect monitoring metrics in seconds. Default is 86400 seconds (24 hrs).", + "schema": { + "type": "integer", + "format": "uint64", + "minimum": 0 + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Array_of_SystemTimeSeries", + "type": "array", + "items": { + "$ref": "#/components/schemas/SystemTimeSeries" + } + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } } }, "components": { @@ -533,6 +593,23 @@ "listen_addr", "remote_servers" ] + }, + "SystemTimeSeries": { + "type": "object", + "properties": { + "time": { + "type": "string", + "format": "date-time" + }, + "value": { + "type": "number", + "format": "double" + } + }, + "required": [ + "time", + "value" + ] } }, "responses": { From 22c1862d384126f9c684b06a53135c0e24674ca8 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 20 Nov 2024 16:54:17 +1300 Subject: [PATCH 03/16] clean up --- clickhouse-admin/api/src/lib.rs | 15 ++++++------ clickhouse-admin/src/clickhouse_cli.rs | 4 +-- clickhouse-admin/src/http_entrypoints.rs | 31 ++++++++++-------------- clickhouse-admin/types/src/lib.rs | 21 +++++----------- openapi/clickhouse-admin-server.json | 4 +-- 5 files changed, 31 insertions(+), 44 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index 3e9ee914a6..e64c18301b 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -4,12 +4,13 @@ use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, MetricName, MetricSettings, - RaftConfig, ReplicaConfig, SystemTimeSeries, ServerConfigurableSettings, + KeeperConfig, KeeperConfigurableSettings, Lgif, MetricName, RaftConfig, + ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, + TimeSeriesSettings, }; use dropshot::{ - HttpError, HttpResponseCreated, HttpResponseOk, HttpResponseUpdatedNoContent, - Path, Query, RequestContext, TypedBody, + HttpError, HttpResponseCreated, HttpResponseOk, + HttpResponseUpdatedNoContent, Path, Query, RequestContext, TypedBody, }; /// API interface for our clickhouse-admin-keeper server @@ -117,8 +118,8 @@ pub trait ClickhouseAdminServerApi { rqctx: RequestContext, ) -> Result>, HttpError>; - /// Generate a ClickHouse configuration file for a server node on a specified - /// directory and enable the SMF service. + /// Retrieve time series from the system.metric_log table. + /// These are internal ClickHouse metrics. #[endpoint { method = GET, path = "/timeseries/metric-log/{metric}" @@ -126,7 +127,7 @@ pub trait ClickhouseAdminServerApi { async fn system_metric_log_timeseries( rqctx: RequestContext, path_params: Path, - query_params: Query, + query_params: Query, ) -> Result>, HttpError>; } diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index 61ec44d2ab..e8647beade 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -6,7 +6,7 @@ use anyhow::Result; use camino::Utf8PathBuf; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperId, Lgif, RaftConfig, MetricLogTimeSeriesSettings,SystemTimeSeries, + KeeperId, Lgif, MetricLogTimeSeriesSettings, RaftConfig, SystemTimeSeries, OXIMETER_CLUSTER, }; use dropshot::HttpError; @@ -164,7 +164,7 @@ impl ClickhouseCli { pub async fn system_metric_log_timeseries( &self, - settings: MetricLogTimeSeriesSettings + settings: MetricLogTimeSeriesSettings, ) -> Result, ClickhouseCliError> { self.client_non_interactive( ClickhouseClientType::Server, diff --git a/clickhouse-admin/src/http_entrypoints.rs b/clickhouse-admin/src/http_entrypoints.rs index f820e132bc..26ffa4f27d 100644 --- a/clickhouse-admin/src/http_entrypoints.rs +++ b/clickhouse-admin/src/http_entrypoints.rs @@ -6,9 +6,9 @@ use crate::context::{ServerContext, SingleServerContext}; use clickhouse_admin_api::*; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, MetricName, MetricSettings, - MetricLogTimeSeriesSettings,RaftConfig, ReplicaConfig, - ServerConfigurableSettings, SystemTimeSeries, + KeeperConfig, KeeperConfigurableSettings, Lgif, + MetricLogTimeSeriesSettings, MetricName, RaftConfig, ReplicaConfig, + ServerConfigurableSettings, SystemTimeSeries, TimeSeriesSettings, }; use dropshot::{ ApiDescription, HttpError, HttpResponseCreated, HttpResponseOk, @@ -66,23 +66,18 @@ impl ClickhouseAdminServerApi for ClickhouseAdminServerImpl { Ok(HttpResponseOk(output)) } - async fn system_metric_log_timeseries( - rqctx:RequestContext, - path_params:Path, - query_params:Query, + async fn system_metric_log_timeseries( + rqctx: RequestContext, + path_params: Path, + query_params: Query, ) -> Result>, HttpError> { let ctx = rqctx.context(); - - // TODO: REMOVEME - println!("PATH PARAMS: {path_params:?}"); - println!("QUERY PARAMS: {query_params:?}"); - - let settings = MetricLogTimeSeriesSettings{ - interval: 60, - time_range: 86400, - metric: "ProfileEvent_Query".to_string(), - }; - let output = ctx.clickhouse_cli().system_metric_log_timeseries(settings).await?; + let settings = query_params.into_inner(); + let metric = path_params.into_inner(); + + let settings = MetricLogTimeSeriesSettings { settings, metric }; + let output = + ctx.clickhouse_cli().system_metric_log_timeseries(settings).await?; Ok(HttpResponseOk(output)) } } diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index bf178547de..c98f9bce15 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1043,7 +1043,7 @@ fn default_time_range() -> u64 { 86400 } -#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Serialize, Deserialize, Display, JsonSchema)] pub struct MetricName { // TODO: Have an enum? /// Name of the metric to retrieve @@ -1051,7 +1051,7 @@ pub struct MetricName { } #[derive(Debug, Serialize, Deserialize, JsonSchema)] -pub struct MetricSettings { +pub struct TimeSeriesSettings { /// The interval to collect monitoring metrics in seconds. /// Default is 60 seconds. // TODO: How can I actually get the default in the API spec? @@ -1063,30 +1063,21 @@ pub struct MetricSettings { pub time_range: u64, } - // TODO: Should I have settings for each system table? // or should I just add an enum here? #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct MetricLogTimeSeriesSettings { - // TODO: Use the above structs here - - /// The interval to collect monitoring metrics in seconds. - /// Default is 60 seconds. - pub interval: u64, - /// Range of time to collect monitoring metrics in seconds. - /// Default is 86400 seconds (24 hrs). - pub time_range: u64, + pub settings: TimeSeriesSettings, // TODO: Have an enum? /// Name of the metric to retrieve - pub metric: String, + pub metric: MetricName, } impl MetricLogTimeSeriesSettings { pub fn query(&self) -> String { - let interval = self.interval; - let time_range = self.time_range; + let interval = self.settings.interval; + let time_range = self.settings.time_range; let metric = &self.metric; - // TODO: Should there be different methods for each system table? let query = format!("SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric}) AS value FROM system.metric_log WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index 3fcaa4e364..fe01c73939 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -76,8 +76,8 @@ }, "/timeseries/metric-log/{metric}": { "get": { - "summary": "Generate a ClickHouse configuration file for a server node on a specified", - "description": "directory and enable the SMF service.", + "summary": "Retrieve time series from the system.metric_log table.", + "description": "These are internal ClickHouse metrics.", "operationId": "system_metric_log_timeseries", "parameters": [ { From 9f59fea13ad6aa954b1d58d1e71400bd40b3d48a Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 20 Nov 2024 17:24:36 +1300 Subject: [PATCH 04/16] Clean up naming --- clickhouse-admin/api/src/lib.rs | 9 ++++----- clickhouse-admin/src/clickhouse_cli.rs | 4 ++-- clickhouse-admin/src/http_entrypoints.rs | 11 +++++------ clickhouse-admin/types/src/lib.rs | 17 ++++++++--------- openapi/clickhouse-admin-server.json | 2 +- 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index e64c18301b..b71ea50d2c 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -4,9 +4,8 @@ use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, MetricName, RaftConfig, - ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, - TimeSeriesSettings, + KeeperConfig, KeeperConfigurableSettings, Lgif, MetricNamePath, RaftConfig, + ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, TimeSeriesSettingsQuery }; use dropshot::{ HttpError, HttpResponseCreated, HttpResponseOk, @@ -126,8 +125,8 @@ pub trait ClickhouseAdminServerApi { }] async fn system_metric_log_timeseries( rqctx: RequestContext, - path_params: Path, - query_params: Query, + path_params: Path, + query_params: Query, ) -> Result>, HttpError>; } diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index e8647beade..e63638af7d 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -6,7 +6,7 @@ use anyhow::Result; use camino::Utf8PathBuf; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperId, Lgif, MetricLogTimeSeriesSettings, RaftConfig, SystemTimeSeries, + KeeperId, Lgif, SystemTimeSeriesSettings, RaftConfig, SystemTimeSeries, OXIMETER_CLUSTER, }; use dropshot::HttpError; @@ -164,7 +164,7 @@ impl ClickhouseCli { pub async fn system_metric_log_timeseries( &self, - settings: MetricLogTimeSeriesSettings, + settings: SystemTimeSeriesSettings, ) -> Result, ClickhouseCliError> { self.client_non_interactive( ClickhouseClientType::Server, diff --git a/clickhouse-admin/src/http_entrypoints.rs b/clickhouse-admin/src/http_entrypoints.rs index 26ffa4f27d..745ebac69d 100644 --- a/clickhouse-admin/src/http_entrypoints.rs +++ b/clickhouse-admin/src/http_entrypoints.rs @@ -7,8 +7,8 @@ use clickhouse_admin_api::*; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, KeeperConfig, KeeperConfigurableSettings, Lgif, - MetricLogTimeSeriesSettings, MetricName, RaftConfig, ReplicaConfig, - ServerConfigurableSettings, SystemTimeSeries, TimeSeriesSettings, + SystemTimeSeriesSettings, MetricNamePath, RaftConfig, ReplicaConfig, + ServerConfigurableSettings, SystemTimeSeries, TimeSeriesSettingsQuery, }; use dropshot::{ ApiDescription, HttpError, HttpResponseCreated, HttpResponseOk, @@ -68,14 +68,13 @@ impl ClickhouseAdminServerApi for ClickhouseAdminServerImpl { async fn system_metric_log_timeseries( rqctx: RequestContext, - path_params: Path, - query_params: Query, + path_params: Path, + query_params: Query, ) -> Result>, HttpError> { let ctx = rqctx.context(); let settings = query_params.into_inner(); let metric = path_params.into_inner(); - - let settings = MetricLogTimeSeriesSettings { settings, metric }; + let settings = SystemTimeSeriesSettings { settings, metric }; let output = ctx.clickhouse_cli().system_metric_log_timeseries(settings).await?; Ok(HttpResponseOk(output)) diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index c98f9bce15..9c937aa7c1 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1043,15 +1043,15 @@ fn default_time_range() -> u64 { 86400 } +// TODO: Have an enum? #[derive(Debug, Serialize, Deserialize, Display, JsonSchema)] -pub struct MetricName { - // TODO: Have an enum? - /// Name of the metric to retrieve +pub struct MetricNamePath { + /// Name of the metric to retrieve. pub metric: String, } #[derive(Debug, Serialize, Deserialize, JsonSchema)] -pub struct TimeSeriesSettings { +pub struct TimeSeriesSettingsQuery { /// The interval to collect monitoring metrics in seconds. /// Default is 60 seconds. // TODO: How can I actually get the default in the API spec? @@ -1066,14 +1066,13 @@ pub struct TimeSeriesSettings { // TODO: Should I have settings for each system table? // or should I just add an enum here? #[derive(Debug, Serialize, Deserialize, JsonSchema)] -pub struct MetricLogTimeSeriesSettings { - pub settings: TimeSeriesSettings, - // TODO: Have an enum? +pub struct SystemTimeSeriesSettings { + pub settings: TimeSeriesSettingsQuery, /// Name of the metric to retrieve - pub metric: MetricName, + pub metric: MetricNamePath, } -impl MetricLogTimeSeriesSettings { +impl SystemTimeSeriesSettings { pub fn query(&self) -> String { let interval = self.settings.interval; let time_range = self.settings.time_range; diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index fe01c73939..b776a13204 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -83,7 +83,7 @@ { "in": "path", "name": "metric", - "description": "Name of the metric to retrieve", + "description": "Name of the metric to retrieve.", "required": true, "schema": { "type": "string" From 75f8fbb30fe7a6d80e0736e459ffc1810437a7c5 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 20 Nov 2024 18:01:41 +1300 Subject: [PATCH 05/16] endpoint for async_metric_log --- clickhouse-admin/api/src/lib.rs | 17 ++++++- clickhouse-admin/src/clickhouse_cli.rs | 22 ++++++++- clickhouse-admin/src/http_entrypoints.rs | 22 +++++++-- clickhouse-admin/types/src/lib.rs | 49 ++++++++++++++----- openapi/clickhouse-admin-server.json | 61 ++++++++++++++++++++++++ 5 files changed, 152 insertions(+), 19 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index b71ea50d2c..58fddc36ca 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -5,7 +5,8 @@ use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, KeeperConfig, KeeperConfigurableSettings, Lgif, MetricNamePath, RaftConfig, - ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, TimeSeriesSettingsQuery + ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, + TimeSeriesSettingsQuery, }; use dropshot::{ HttpError, HttpResponseCreated, HttpResponseOk, @@ -128,6 +129,18 @@ pub trait ClickhouseAdminServerApi { path_params: Path, query_params: Query, ) -> Result>, HttpError>; + + /// Retrieve time series from the system.asynchronous_metric_log table. + /// These are internal ClickHouse metrics. + #[endpoint { + method = GET, + path = "/timeseries/async-metric-log/{metric}" + }] + async fn system_async_metric_log_timeseries( + rqctx: RequestContext, + path_params: Path, + query_params: Query, + ) -> Result>, HttpError>; } /// API interface for our clickhouse-admin-single server @@ -148,4 +161,6 @@ pub trait ClickhouseAdminSingleApi { async fn init_db( rqctx: RequestContext, ) -> Result; + + // TODO: Retrieve time series here too } diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index e63638af7d..9bcb4d4f95 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -6,7 +6,7 @@ use anyhow::Result; use camino::Utf8PathBuf; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperId, Lgif, SystemTimeSeriesSettings, RaftConfig, SystemTimeSeries, + KeeperId, Lgif, RaftConfig, SystemTimeSeries, SystemTimeSeriesSettings, OXIMETER_CLUSTER, }; use dropshot::HttpError; @@ -168,12 +168,30 @@ impl ClickhouseCli { ) -> Result, ClickhouseCliError> { self.client_non_interactive( ClickhouseClientType::Server, - settings.query().as_str(), + settings.query_metric_log().as_str(), "Retrieve time series from the system.metric_log table", SystemTimeSeries::parse, self.log.clone().unwrap(), ) .await + + // TODO: log query? + } + + pub async fn system_async_metric_log_timeseries( + &self, + settings: SystemTimeSeriesSettings, + ) -> Result, ClickhouseCliError> { + self.client_non_interactive( + ClickhouseClientType::Server, + settings.query_async_metric_log().as_str(), + "Retrieve time series from the system.asynchronous_metric_log table", + SystemTimeSeries::parse, + self.log.clone().unwrap(), + ) + .await + + // TODO: log query? } async fn client_non_interactive( diff --git a/clickhouse-admin/src/http_entrypoints.rs b/clickhouse-admin/src/http_entrypoints.rs index 745ebac69d..b3ab3b6785 100644 --- a/clickhouse-admin/src/http_entrypoints.rs +++ b/clickhouse-admin/src/http_entrypoints.rs @@ -6,9 +6,9 @@ use crate::context::{ServerContext, SingleServerContext}; use clickhouse_admin_api::*; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, - SystemTimeSeriesSettings, MetricNamePath, RaftConfig, ReplicaConfig, - ServerConfigurableSettings, SystemTimeSeries, TimeSeriesSettingsQuery, + KeeperConfig, KeeperConfigurableSettings, Lgif, MetricNamePath, RaftConfig, + ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, + SystemTimeSeriesSettings, TimeSeriesSettingsQuery, }; use dropshot::{ ApiDescription, HttpError, HttpResponseCreated, HttpResponseOk, @@ -79,6 +79,22 @@ impl ClickhouseAdminServerApi for ClickhouseAdminServerImpl { ctx.clickhouse_cli().system_metric_log_timeseries(settings).await?; Ok(HttpResponseOk(output)) } + + async fn system_async_metric_log_timeseries( + rqctx: RequestContext, + path_params: Path, + query_params: Query, + ) -> Result>, HttpError> { + let ctx = rqctx.context(); + let settings = query_params.into_inner(); + let metric = path_params.into_inner(); + let settings = SystemTimeSeriesSettings { settings, metric }; + let output = ctx + .clickhouse_cli() + .system_async_metric_log_timeseries(settings) + .await?; + Ok(HttpResponseOk(output)) + } } enum ClickhouseAdminKeeperImpl {} diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index 9c937aa7c1..1224ef1198 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1043,14 +1043,15 @@ fn default_time_range() -> u64 { 86400 } -// TODO: Have an enum? #[derive(Debug, Serialize, Deserialize, Display, JsonSchema)] +#[serde(rename_all = "snake_case")] pub struct MetricNamePath { /// Name of the metric to retrieve. pub metric: String, } #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] pub struct TimeSeriesSettingsQuery { /// The interval to collect monitoring metrics in seconds. /// Default is 60 seconds. @@ -1063,9 +1064,9 @@ pub struct TimeSeriesSettingsQuery { pub time_range: u64, } -// TODO: Should I have settings for each system table? -// or should I just add an enum here? +/// Settings to specify which time series to retrieve. #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] pub struct SystemTimeSeriesSettings { pub settings: TimeSeriesSettingsQuery, /// Name of the metric to retrieve @@ -1073,24 +1074,45 @@ pub struct SystemTimeSeriesSettings { } impl SystemTimeSeriesSettings { - pub fn query(&self) -> String { + // TODO: Use more aggregate functions than just avg? + + pub fn query_metric_log(&self) -> String { let interval = self.settings.interval; let time_range = self.settings.time_range; let metric = &self.metric; - let query = format!("SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric}) AS value - FROM system.metric_log - WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} - GROUP BY time - ORDER BY time WITH FILL STEP {interval} - FORMAT JSONEachRow - SETTINGS date_time_output_format = 'iso'"); + let query = format!( + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric}) AS value + FROM system.metric_log + WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} + GROUP BY time + ORDER BY time WITH FILL STEP {interval} + FORMAT JSONEachRow + SETTINGS date_time_output_format = 'iso'" + ); query } -} -// TODO: Do the above for AsyncMetricLogTimeSeriesSettings + pub fn query_async_metric_log(&self) -> String { + let interval = self.settings.interval; + let time_range = self.settings.time_range; + let metric = &self.metric; + let query = format!( + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg(value) AS value + FROM system.asynchronous_metric_log + WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} + AND metric = '{metric}' + GROUP BY time + ORDER BY time WITH FILL STEP {interval} + FORMAT JSONEachRow + SETTINGS date_time_output_format = 'iso'" + ); + query + } +} +/// Retrieved time series from the internal `system` database. #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] pub struct SystemTimeSeries { pub time: DateTime, pub value: f64, @@ -1105,6 +1127,7 @@ impl SystemTimeSeries { info!( log, "Retrieved data from `system` database"; + // TODO: Log output here? feels like a lot of noise? "output" => ?s ); diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index b776a13204..95574a4cf7 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -74,6 +74,66 @@ } } }, + "/timeseries/async-metric-log/{metric}": { + "get": { + "summary": "Retrieve time series from the system.asynchronous_metric_log table.", + "description": "These are internal ClickHouse metrics.", + "operationId": "system_async_metric_log_timeseries", + "parameters": [ + { + "in": "path", + "name": "metric", + "description": "Name of the metric to retrieve.", + "required": true, + "schema": { + "type": "string" + } + }, + { + "in": "query", + "name": "interval", + "description": "The interval to collect monitoring metrics in seconds. Default is 60 seconds.", + "schema": { + "type": "integer", + "format": "uint64", + "minimum": 0 + } + }, + { + "in": "query", + "name": "time_range", + "description": "Range of time to collect monitoring metrics in seconds. Default is 86400 seconds (24 hrs).", + "schema": { + "type": "integer", + "format": "uint64", + "minimum": 0 + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Array_of_SystemTimeSeries", + "type": "array", + "items": { + "$ref": "#/components/schemas/SystemTimeSeries" + } + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/timeseries/metric-log/{metric}": { "get": { "summary": "Retrieve time series from the system.metric_log table.", @@ -595,6 +655,7 @@ ] }, "SystemTimeSeries": { + "description": "Retrieved time series from the internal `system` database.", "type": "object", "properties": { "time": { From 76c44b91259d7660178119ef66017bd81f64f0d7 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 21 Nov 2024 13:15:07 +1300 Subject: [PATCH 06/16] Set avg to a single endpoint --- clickhouse-admin/api/src/lib.rs | 28 ++++----- clickhouse-admin/src/clickhouse_cli.rs | 32 +++++----- clickhouse-admin/src/http_entrypoints.rs | 38 ++++++------ clickhouse-admin/types/src/lib.rs | 76 +++++++++++++++++------- oximeter/db/src/client/mod.rs | 2 +- 5 files changed, 105 insertions(+), 71 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index 58fddc36ca..aa69eb88ef 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -4,7 +4,7 @@ use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, MetricNamePath, RaftConfig, + KeeperConfig, KeeperConfigurableSettings, Lgif, MetricInfoPath, RaftConfig, ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, TimeSeriesSettingsQuery, }; @@ -122,25 +122,25 @@ pub trait ClickhouseAdminServerApi { /// These are internal ClickHouse metrics. #[endpoint { method = GET, - path = "/timeseries/metric-log/{metric}" + path = "/timeseries/{table}/{metric}" }] async fn system_metric_log_timeseries( rqctx: RequestContext, - path_params: Path, + path_params: Path, query_params: Query, ) -> Result>, HttpError>; - /// Retrieve time series from the system.asynchronous_metric_log table. - /// These are internal ClickHouse metrics. - #[endpoint { - method = GET, - path = "/timeseries/async-metric-log/{metric}" - }] - async fn system_async_metric_log_timeseries( - rqctx: RequestContext, - path_params: Path, - query_params: Query, - ) -> Result>, HttpError>; + // /// Retrieve time series from the system.asynchronous_metric_log table. + // /// These are internal ClickHouse metrics. + // #[endpoint { + // method = GET, + // path = "/timeseries/async-metric-log/{metric}" + // }] + // async fn system_async_metric_log_timeseries( + // rqctx: RequestContext, + // path_params: Path, + // query_params: Query, + // ) -> Result>, HttpError>; } /// API interface for our clickhouse-admin-single server diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index 9bcb4d4f95..4057fa6fa4 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -168,7 +168,7 @@ impl ClickhouseCli { ) -> Result, ClickhouseCliError> { self.client_non_interactive( ClickhouseClientType::Server, - settings.query_metric_log().as_str(), + settings.query().as_str(), "Retrieve time series from the system.metric_log table", SystemTimeSeries::parse, self.log.clone().unwrap(), @@ -178,21 +178,21 @@ impl ClickhouseCli { // TODO: log query? } - pub async fn system_async_metric_log_timeseries( - &self, - settings: SystemTimeSeriesSettings, - ) -> Result, ClickhouseCliError> { - self.client_non_interactive( - ClickhouseClientType::Server, - settings.query_async_metric_log().as_str(), - "Retrieve time series from the system.asynchronous_metric_log table", - SystemTimeSeries::parse, - self.log.clone().unwrap(), - ) - .await - - // TODO: log query? - } +// pub async fn system_async_metric_log_timeseries( +// &self, +// settings: SystemTimeSeriesSettings, +// ) -> Result, ClickhouseCliError> { +// self.client_non_interactive( +// ClickhouseClientType::Server, +// settings.query_async_metric_log().as_str(), +// "Retrieve time series from the system.asynchronous_metric_log table", +// SystemTimeSeries::parse, +// self.log.clone().unwrap(), +// ) +// .await +// +// // TODO: log query? +// } async fn client_non_interactive( &self, diff --git a/clickhouse-admin/src/http_entrypoints.rs b/clickhouse-admin/src/http_entrypoints.rs index b3ab3b6785..4afc71270f 100644 --- a/clickhouse-admin/src/http_entrypoints.rs +++ b/clickhouse-admin/src/http_entrypoints.rs @@ -6,7 +6,7 @@ use crate::context::{ServerContext, SingleServerContext}; use clickhouse_admin_api::*; use clickhouse_admin_types::{ ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf, - KeeperConfig, KeeperConfigurableSettings, Lgif, MetricNamePath, RaftConfig, + KeeperConfig, KeeperConfigurableSettings, Lgif, MetricInfoPath, RaftConfig, ReplicaConfig, ServerConfigurableSettings, SystemTimeSeries, SystemTimeSeriesSettings, TimeSeriesSettingsQuery, }; @@ -68,33 +68,33 @@ impl ClickhouseAdminServerApi for ClickhouseAdminServerImpl { async fn system_metric_log_timeseries( rqctx: RequestContext, - path_params: Path, + path_params: Path, query_params: Query, ) -> Result>, HttpError> { let ctx = rqctx.context(); let settings = query_params.into_inner(); - let metric = path_params.into_inner(); - let settings = SystemTimeSeriesSettings { settings, metric }; + let metric_info = path_params.into_inner(); + let settings = SystemTimeSeriesSettings { settings, metric_info }; let output = ctx.clickhouse_cli().system_metric_log_timeseries(settings).await?; Ok(HttpResponseOk(output)) } - async fn system_async_metric_log_timeseries( - rqctx: RequestContext, - path_params: Path, - query_params: Query, - ) -> Result>, HttpError> { - let ctx = rqctx.context(); - let settings = query_params.into_inner(); - let metric = path_params.into_inner(); - let settings = SystemTimeSeriesSettings { settings, metric }; - let output = ctx - .clickhouse_cli() - .system_async_metric_log_timeseries(settings) - .await?; - Ok(HttpResponseOk(output)) - } + // async fn system_async_metric_log_timeseries( + // rqctx: RequestContext, + // path_params: Path, + // query_params: Query, + // ) -> Result>, HttpError> { + // let ctx = rqctx.context(); + // let settings = query_params.into_inner(); + // let metric = path_params.into_inner(); + // let settings = SystemTimeSeriesSettings { settings, metric }; + // let output = ctx + // .clickhouse_cli() + // .system_async_metric_log_timeseries(settings) + // .await?; + // Ok(HttpResponseOk(output)) + // } } enum ClickhouseAdminKeeperImpl {} diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index 1224ef1198..ef6da815de 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -17,6 +17,7 @@ use schemars::{ use serde::{Deserialize, Serialize}; use slog::{info, Logger}; use std::collections::{BTreeMap, BTreeSet}; +use std::fmt; use std::fs::create_dir; use std::io::{ErrorKind, Write}; use std::net::Ipv6Addr; @@ -1043,9 +1044,29 @@ fn default_time_range() -> u64 { 86400 } -#[derive(Debug, Serialize, Deserialize, Display, JsonSchema)] +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +/// Available metrics tables in the `system` database +pub enum SystemTable { + AsynchronousMetricLog, + MetricLog, +} + +impl fmt::Display for SystemTable { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let table = match self { + SystemTable::MetricLog => "metric_log", + SystemTable::AsynchronousMetricLog => "asynchronous_metric_log" + }; + write!(f, "{}", table) + } +} + +#[derive(Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] -pub struct MetricNamePath { +pub struct MetricInfoPath { + /// Table to query in the `system` database + pub table: SystemTable, /// Name of the metric to retrieve. pub metric: String, } @@ -1069,45 +1090,58 @@ pub struct TimeSeriesSettingsQuery { #[serde(rename_all = "snake_case")] pub struct SystemTimeSeriesSettings { pub settings: TimeSeriesSettingsQuery, - /// Name of the metric to retrieve - pub metric: MetricNamePath, + /// Database table and name of the metric to retrieve + pub metric_info: MetricInfoPath, } impl SystemTimeSeriesSettings { // TODO: Use more aggregate functions than just avg? - pub fn query_metric_log(&self) -> String { + pub fn query(&self) -> String { let interval = self.settings.interval; let time_range = self.settings.time_range; - let metric = &self.metric; - let query = format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric}) AS value - FROM system.metric_log + let metric_name = &self.metric_info.metric; + let table = &self.metric_info.table; + let query = match table { + SystemTable::MetricLog => format!( + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric_name}) AS value + FROM system.{table} WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} GROUP BY time ORDER BY time WITH FILL STEP {interval} FORMAT JSONEachRow SETTINGS date_time_output_format = 'iso'" - ); - query - } - - pub fn query_async_metric_log(&self) -> String { - let interval = self.settings.interval; - let time_range = self.settings.time_range; - let metric = &self.metric; - let query = format!( + ), + SystemTable::AsynchronousMetricLog => format!( "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg(value) AS value - FROM system.asynchronous_metric_log + FROM system.{table} WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} - AND metric = '{metric}' + AND metric = '{metric_name}' GROUP BY time ORDER BY time WITH FILL STEP {interval} FORMAT JSONEachRow SETTINGS date_time_output_format = 'iso'" - ); + ), + }; query } + +// pub fn query_async_metric_log(&self) -> String { +// let interval = self.settings.interval; +// let time_range = self.settings.time_range; +// let metric = &self.metric; +// let query = format!( +// "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg(value) AS value +// FROM system.asynchronous_metric_log +// WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} +// AND metric = '{metric}' +// GROUP BY time +// ORDER BY time WITH FILL STEP {interval} +// FORMAT JSONEachRow +// SETTINGS date_time_output_format = 'iso'" +// ); +// query +// } } /// Retrieved time series from the internal `system` database. diff --git a/oximeter/db/src/client/mod.rs b/oximeter/db/src/client/mod.rs index e8db4a4b61..b3a53a5cbc 100644 --- a/oximeter/db/src/client/mod.rs +++ b/oximeter/db/src/client/mod.rs @@ -1156,7 +1156,7 @@ impl Client { // block. // // TODO-robustness This currently does no validation of the statement. - async fn execute_with_block( + pub async fn execute_with_block( &self, sql: &str, ) -> Result { From 71471dbde4c43a9f5202a61d1857a17de02d8699 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 21 Nov 2024 15:02:20 +1300 Subject: [PATCH 07/16] specify endpoint as average --- clickhouse-admin/api/src/lib.rs | 16 +---- clickhouse-admin/src/clickhouse_cli.rs | 23 +------ clickhouse-admin/src/http_entrypoints.rs | 25 ++------ clickhouse-admin/types/src/lib.rs | 78 ++++++++++-------------- openapi/clickhouse-admin-server.json | 71 +++++---------------- 5 files changed, 57 insertions(+), 156 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index aa69eb88ef..593cfa58b1 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -118,29 +118,17 @@ pub trait ClickhouseAdminServerApi { rqctx: RequestContext, ) -> Result>, HttpError>; - /// Retrieve time series from the system.metric_log table. + /// Retrieve time series from the system database. /// These are internal ClickHouse metrics. #[endpoint { method = GET, path = "/timeseries/{table}/{metric}" }] - async fn system_metric_log_timeseries( + async fn system_timeseries_avg( rqctx: RequestContext, path_params: Path, query_params: Query, ) -> Result>, HttpError>; - - // /// Retrieve time series from the system.asynchronous_metric_log table. - // /// These are internal ClickHouse metrics. - // #[endpoint { - // method = GET, - // path = "/timeseries/async-metric-log/{metric}" - // }] - // async fn system_async_metric_log_timeseries( - // rqctx: RequestContext, - // path_params: Path, - // query_params: Query, - // ) -> Result>, HttpError>; } /// API interface for our clickhouse-admin-single server diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index 4057fa6fa4..360e677b6d 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -162,38 +162,21 @@ impl ClickhouseCli { .await } - pub async fn system_metric_log_timeseries( + pub async fn system_timeseries_avg( &self, settings: SystemTimeSeriesSettings, ) -> Result, ClickhouseCliError> { + // TODO: log query self.client_non_interactive( ClickhouseClientType::Server, - settings.query().as_str(), + settings.query_avg().as_str(), "Retrieve time series from the system.metric_log table", SystemTimeSeries::parse, self.log.clone().unwrap(), ) .await - - // TODO: log query? } -// pub async fn system_async_metric_log_timeseries( -// &self, -// settings: SystemTimeSeriesSettings, -// ) -> Result, ClickhouseCliError> { -// self.client_non_interactive( -// ClickhouseClientType::Server, -// settings.query_async_metric_log().as_str(), -// "Retrieve time series from the system.asynchronous_metric_log table", -// SystemTimeSeries::parse, -// self.log.clone().unwrap(), -// ) -// .await -// -// // TODO: log query? -// } - async fn client_non_interactive( &self, client: ClickhouseClientType, diff --git a/clickhouse-admin/src/http_entrypoints.rs b/clickhouse-admin/src/http_entrypoints.rs index 4afc71270f..aea1d097c0 100644 --- a/clickhouse-admin/src/http_entrypoints.rs +++ b/clickhouse-admin/src/http_entrypoints.rs @@ -66,35 +66,20 @@ impl ClickhouseAdminServerApi for ClickhouseAdminServerImpl { Ok(HttpResponseOk(output)) } - async fn system_metric_log_timeseries( + async fn system_timeseries_avg( rqctx: RequestContext, path_params: Path, query_params: Query, ) -> Result>, HttpError> { let ctx = rqctx.context(); - let settings = query_params.into_inner(); + let retrieval_settings = query_params.into_inner(); let metric_info = path_params.into_inner(); - let settings = SystemTimeSeriesSettings { settings, metric_info }; + let settings = + SystemTimeSeriesSettings { retrieval_settings, metric_info }; let output = - ctx.clickhouse_cli().system_metric_log_timeseries(settings).await?; + ctx.clickhouse_cli().system_timeseries_avg(settings).await?; Ok(HttpResponseOk(output)) } - - // async fn system_async_metric_log_timeseries( - // rqctx: RequestContext, - // path_params: Path, - // query_params: Query, - // ) -> Result>, HttpError> { - // let ctx = rqctx.context(); - // let settings = query_params.into_inner(); - // let metric = path_params.into_inner(); - // let settings = SystemTimeSeriesSettings { settings, metric }; - // let output = ctx - // .clickhouse_cli() - // .system_async_metric_log_timeseries(settings) - // .await?; - // Ok(HttpResponseOk(output)) - // } } enum ClickhouseAdminKeeperImpl {} diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index ef6da815de..2088ee2d03 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1056,7 +1056,7 @@ impl fmt::Display for SystemTable { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let table = match self { SystemTable::MetricLog => "metric_log", - SystemTable::AsynchronousMetricLog => "asynchronous_metric_log" + SystemTable::AsynchronousMetricLog => "asynchronous_metric_log", }; write!(f, "{}", table) } @@ -1089,7 +1089,8 @@ pub struct TimeSeriesSettingsQuery { #[derive(Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] pub struct SystemTimeSeriesSettings { - pub settings: TimeSeriesSettingsQuery, + /// Time series retrieval settings (time range and interval) + pub retrieval_settings: TimeSeriesSettingsQuery, /// Database table and name of the metric to retrieve pub metric_info: MetricInfoPath, } @@ -1097,51 +1098,39 @@ pub struct SystemTimeSeriesSettings { impl SystemTimeSeriesSettings { // TODO: Use more aggregate functions than just avg? - pub fn query(&self) -> String { - let interval = self.settings.interval; - let time_range = self.settings.time_range; + pub fn query_avg(&self) -> String { + let interval = self.retrieval_settings.interval; + let time_range = self.retrieval_settings.time_range; let metric_name = &self.metric_info.metric; let table = &self.metric_info.table; - let query = match table { + + let mut query = match table { SystemTable::MetricLog => format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric_name}) AS value - FROM system.{table} - WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} - GROUP BY time - ORDER BY time WITH FILL STEP {interval} - FORMAT JSONEachRow - SETTINGS date_time_output_format = 'iso'" - ), - SystemTable::AsynchronousMetricLog => format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg(value) AS value - FROM system.{table} - WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} - AND metric = '{metric_name}' - GROUP BY time - ORDER BY time WITH FILL STEP {interval} - FORMAT JSONEachRow - SETTINGS date_time_output_format = 'iso'" - ), - }; + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric_name}) AS value + FROM system.{table} + WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} + " + ), + SystemTable::AsynchronousMetricLog => format!( + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg(value) AS value + FROM system.{table} + WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} + AND metric = '{metric_name}' + " + ), + }; + + query.push_str( + format!( + "GROUP BY time + ORDER BY time WITH FILL STEP {interval} + FORMAT JSONEachRow + SETTINGS date_time_output_format = 'iso'" + ) + .as_str(), + ); query } - -// pub fn query_async_metric_log(&self) -> String { -// let interval = self.settings.interval; -// let time_range = self.settings.time_range; -// let metric = &self.metric; -// let query = format!( -// "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg(value) AS value -// FROM system.asynchronous_metric_log -// WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} -// AND metric = '{metric}' -// GROUP BY time -// ORDER BY time WITH FILL STEP {interval} -// FORMAT JSONEachRow -// SETTINGS date_time_output_format = 'iso'" -// ); -// query -// } } /// Retrieved time series from the internal `system` database. @@ -1150,9 +1139,8 @@ impl SystemTimeSeriesSettings { pub struct SystemTimeSeries { pub time: DateTime, pub value: f64, - // TODO: Have an enum with possible units? (s, ms, bytes) - // Not sure if I can even add this, the table doesn't mention units at all - // pub unit: String, + // TODO: Would be really nice to have an enum with possible units (s, ms, bytes) + // Not sure if I can even add this, the system tables don't mention units at all. } impl SystemTimeSeries { diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index 95574a4cf7..bfbd064b68 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -74,11 +74,11 @@ } } }, - "/timeseries/async-metric-log/{metric}": { + "/timeseries/{table}/{metric}": { "get": { - "summary": "Retrieve time series from the system.asynchronous_metric_log table.", + "summary": "Retrieve time series from the system database.", "description": "These are internal ClickHouse metrics.", - "operationId": "system_async_metric_log_timeseries", + "operationId": "system_timeseries_avg", "parameters": [ { "in": "path", @@ -89,64 +89,13 @@ "type": "string" } }, - { - "in": "query", - "name": "interval", - "description": "The interval to collect monitoring metrics in seconds. Default is 60 seconds.", - "schema": { - "type": "integer", - "format": "uint64", - "minimum": 0 - } - }, - { - "in": "query", - "name": "time_range", - "description": "Range of time to collect monitoring metrics in seconds. Default is 86400 seconds (24 hrs).", - "schema": { - "type": "integer", - "format": "uint64", - "minimum": 0 - } - } - ], - "responses": { - "200": { - "description": "successful operation", - "content": { - "application/json": { - "schema": { - "title": "Array_of_SystemTimeSeries", - "type": "array", - "items": { - "$ref": "#/components/schemas/SystemTimeSeries" - } - } - } - } - }, - "4XX": { - "$ref": "#/components/responses/Error" - }, - "5XX": { - "$ref": "#/components/responses/Error" - } - } - } - }, - "/timeseries/metric-log/{metric}": { - "get": { - "summary": "Retrieve time series from the system.metric_log table.", - "description": "These are internal ClickHouse metrics.", - "operationId": "system_metric_log_timeseries", - "parameters": [ { "in": "path", - "name": "metric", - "description": "Name of the metric to retrieve.", + "name": "table", + "description": "Table to query in the `system` database", "required": true, "schema": { - "type": "string" + "$ref": "#/components/schemas/SystemTable" } }, { @@ -671,6 +620,14 @@ "time", "value" ] + }, + "SystemTable": { + "description": "Available metrics tables in the `system` database", + "type": "string", + "enum": [ + "asynchronous_metric_log", + "metric_log" + ] } }, "responses": { From 9637d2511c5dd9effaf5dcd08656bdc0f6ce26de Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 21 Nov 2024 15:25:33 +1300 Subject: [PATCH 08/16] add a timeout --- clickhouse-admin/src/clickhouse_cli.rs | 44 +++++++++++++++++++------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index 360e677b6d..69521eff74 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -18,8 +18,11 @@ use std::ffi::OsStr; use std::fmt::Display; use std::io; use std::net::SocketAddrV6; +use std::time::Duration; use tokio::process::Command; +const DEFAULT_COMMAND_TIMEOUT: Duration = Duration::from_secs(30); + #[derive(Debug, thiserror::Error, SlogInlineError)] pub enum ClickhouseCliError { #[error("failed to run `clickhouse {subcommand}`")] @@ -39,6 +42,8 @@ pub enum ClickhouseCliError { #[source] err: anyhow::Error, }, + #[error("clickhouse server unavailable: {0}")] + ServerUnavailable(String), } impl From for HttpError { @@ -46,6 +51,7 @@ impl From for HttpError { match err { ClickhouseCliError::Run { .. } | ClickhouseCliError::Parse { .. } + | ClickhouseCliError::ServerUnavailable { .. } | ClickhouseCliError::ExecutionError(_) => { let message = InlineErrorChain::new(&err).to_string(); HttpError { @@ -198,19 +204,33 @@ impl ClickhouseCli { .arg("--query") .arg(query); - let output = command.output().await.map_err(|err| { - let err_args: Vec<&OsStr> = command.as_std().get_args().collect(); - let err_args_parsed: Vec = err_args - .iter() - .map(|&os_str| os_str.to_string_lossy().into_owned()) - .collect(); - let err_args_str = err_args_parsed.join(" "); - ClickhouseCliError::Run { - description: subcommand_description, - subcommand: err_args_str, - err, + let now = tokio::time::Instant::now(); + let result = + tokio::time::timeout(DEFAULT_COMMAND_TIMEOUT, command.output()) + .await; + + let elapsed = now.elapsed(); + let output = match result { + Ok(result) => result.map_err(|err| { + let err_args: Vec<&OsStr> = + command.as_std().get_args().collect(); + let err_args_parsed: Vec = err_args + .iter() + .map(|&os_str| os_str.to_string_lossy().into_owned()) + .collect(); + let err_args_str = err_args_parsed.join(" "); + ClickhouseCliError::Run { + description: subcommand_description, + subcommand: err_args_str, + err: err.into(), + } + })?, + Err(e) => { + return Err(ClickhouseCliError::ServerUnavailable(format!( + "command timed out after {elapsed:?}: {e}" + ))) } - })?; + }; if !output.status.success() { return Err(output_to_exec_error(command.as_std(), &output).into()); From 8412fb1c43654f30452930cb7c415e30a5647a02 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 21 Nov 2024 16:11:01 +1300 Subject: [PATCH 09/16] clarify endpoint documentation --- clickhouse-admin/api/src/lib.rs | 5 +++-- clickhouse-admin/src/clickhouse_cli.rs | 16 ++++++++++------ openapi/clickhouse-admin-server.json | 6 +++--- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index 593cfa58b1..540c3933be 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -118,11 +118,12 @@ pub trait ClickhouseAdminServerApi { rqctx: RequestContext, ) -> Result>, HttpError>; - /// Retrieve time series from the system database. + /// Retrieve time series from the system database. The value the average of all + /// values within the interval. /// These are internal ClickHouse metrics. #[endpoint { method = GET, - path = "/timeseries/{table}/{metric}" + path = "/timeseries/{table}/{metric}/avg" }] async fn system_timeseries_avg( rqctx: RequestContext, diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index 69521eff74..c6557cfb30 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -11,7 +11,7 @@ use clickhouse_admin_types::{ }; use dropshot::HttpError; use illumos_utils::{output_to_exec_error, ExecutionError}; -use slog::Logger; +use slog::{info, Logger}; use slog_error_chain::{InlineErrorChain, SlogInlineError}; use std::collections::BTreeSet; use std::ffi::OsStr; @@ -172,13 +172,17 @@ impl ClickhouseCli { &self, settings: SystemTimeSeriesSettings, ) -> Result, ClickhouseCliError> { - // TODO: log query + let log = self.log.clone().unwrap(); + let query = settings.query_avg(); + + info!(&log, "Querying system database"; "query" => &query); + self.client_non_interactive( ClickhouseClientType::Server, - settings.query_avg().as_str(), - "Retrieve time series from the system.metric_log table", + &query, + "Retrieve time series from the system database", SystemTimeSeries::parse, - self.log.clone().unwrap(), + log, ) .await } @@ -222,7 +226,7 @@ impl ClickhouseCli { ClickhouseCliError::Run { description: subcommand_description, subcommand: err_args_str, - err: err.into(), + err, } })?, Err(e) => { diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index bfbd064b68..b558d83698 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -74,10 +74,10 @@ } } }, - "/timeseries/{table}/{metric}": { + "/timeseries/{table}/{metric}/avg": { "get": { - "summary": "Retrieve time series from the system database.", - "description": "These are internal ClickHouse metrics.", + "summary": "Retrieve time series from the system database. The value the average of all", + "description": "values within the interval. These are internal ClickHouse metrics.", "operationId": "system_timeseries_avg", "parameters": [ { From 8077fd998a7a676cba558d375c8d06464c95515d Mon Sep 17 00:00:00 2001 From: karencfv Date: Fri, 22 Nov 2024 14:13:54 +1300 Subject: [PATCH 10/16] give th option to format timestamp as unix epoch --- clickhouse-admin/types/src/lib.rs | 44 ++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index 2088ee2d03..51ea11ef5d 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1044,6 +1044,11 @@ fn default_time_range() -> u64 { 86400 } +#[inline] +fn default_timestamp_format() -> TimestampFormat { + TimestampFormat::Utc +} + #[derive(Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] /// Available metrics tables in the `system` database @@ -1062,6 +1067,24 @@ impl fmt::Display for SystemTable { } } +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +/// Which format should the timestamp be in. +pub enum TimestampFormat { + Utc, + UnixEpoch, +} + +impl fmt::Display for TimestampFormat { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let table = match self { + TimestampFormat::Utc => "utc", + TimestampFormat::UnixEpoch => "unix_epoch", + }; + write!(f, "{}", table) + } +} + #[derive(Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] pub struct MetricInfoPath { @@ -1083,6 +1106,10 @@ pub struct TimeSeriesSettingsQuery { /// Default is 86400 seconds (24 hrs). #[serde(default = "default_time_range")] pub time_range: u64, + /// Format in which each timeseries timestamp will be in. + /// Default is UTC + #[serde(default = "default_timestamp_format")] + pub timestamp_format: TimestampFormat, } /// Settings to specify which time series to retrieve. @@ -1103,16 +1130,20 @@ impl SystemTimeSeriesSettings { let time_range = self.retrieval_settings.time_range; let metric_name = &self.metric_info.metric; let table = &self.metric_info.table; + let timestamp_format = match &self.retrieval_settings.timestamp_format { + TimestampFormat::Utc => "", + TimestampFormat::UnixEpoch => "::INT" + }; let mut query = match table { SystemTable::MetricLog => format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({metric_name}) AS value + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND){timestamp_format} AS time, avg({metric_name}) AS value FROM system.{table} WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} " ), SystemTable::AsynchronousMetricLog => format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg(value) AS value + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND){timestamp_format} AS time, avg(value) AS value FROM system.{table} WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} AND metric = '{metric_name}' @@ -1133,11 +1164,18 @@ impl SystemTimeSeriesSettings { } } +#[derive(Debug, Display, Serialize, Deserialize, JsonSchema)] +#[serde(untagged)] +pub enum Timestamp { + Unix(u64), + Utc(DateTime), +} + /// Retrieved time series from the internal `system` database. #[derive(Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] pub struct SystemTimeSeries { - pub time: DateTime, + pub time: Timestamp,//DateTime, pub value: f64, // TODO: Would be really nice to have an enum with possible units (s, ms, bytes) // Not sure if I can even add this, the system tables don't mention units at all. From 629819905257e34c258d9c95a80ff48ef3e6d9f2 Mon Sep 17 00:00:00 2001 From: karencfv Date: Fri, 22 Nov 2024 14:51:50 +1300 Subject: [PATCH 11/16] clean up --- clickhouse-admin/types/src/lib.rs | 42 ++++++++++++++++------------ openapi/clickhouse-admin-server.json | 30 ++++++++++++++++++-- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index 51ea11ef5d..b4f46457f4 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1130,24 +1130,30 @@ impl SystemTimeSeriesSettings { let time_range = self.retrieval_settings.time_range; let metric_name = &self.metric_info.metric; let table = &self.metric_info.table; - let timestamp_format = match &self.retrieval_settings.timestamp_format { - TimestampFormat::Utc => "", - TimestampFormat::UnixEpoch => "::INT" + let ts_fmt = match &self.retrieval_settings.timestamp_format { + TimestampFormat::Utc => "iso", + TimestampFormat::UnixEpoch => "unix_timestamp", + }; + let avg_value = match table { + SystemTable::MetricLog => metric_name, + SystemTable::AsynchronousMetricLog => "value", }; - let mut query = match table { - SystemTable::MetricLog => format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND){timestamp_format} AS time, avg({metric_name}) AS value - FROM system.{table} - WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} - " - ), - SystemTable::AsynchronousMetricLog => format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND){timestamp_format} AS time, avg(value) AS value - FROM system.{table} - WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} - AND metric = '{metric_name}' + let mut query = format!( + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({avg_value}) AS value + FROM system.{table} + WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} + " + ); + + match table { + SystemTable::MetricLog => (), + SystemTable::AsynchronousMetricLog => query.push_str( + format!( + "AND metric = '{metric_name}' " + ) + .as_str(), ), }; @@ -1156,7 +1162,7 @@ impl SystemTimeSeriesSettings { "GROUP BY time ORDER BY time WITH FILL STEP {interval} FORMAT JSONEachRow - SETTINGS date_time_output_format = 'iso'" + SETTINGS date_time_output_format = '{ts_fmt}'" ) .as_str(), ); @@ -1167,7 +1173,7 @@ impl SystemTimeSeriesSettings { #[derive(Debug, Display, Serialize, Deserialize, JsonSchema)] #[serde(untagged)] pub enum Timestamp { - Unix(u64), + Unix(String), Utc(DateTime), } @@ -1175,7 +1181,7 @@ pub enum Timestamp { #[derive(Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] pub struct SystemTimeSeries { - pub time: Timestamp,//DateTime, + pub time: Timestamp, pub value: f64, // TODO: Would be really nice to have an enum with possible units (s, ms, bytes) // Not sure if I can even add this, the system tables don't mention units at all. diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index b558d83698..05042fbc68 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -117,6 +117,14 @@ "format": "uint64", "minimum": 0 } + }, + { + "in": "query", + "name": "timestamp_format", + "description": "Format in which each timeseries timestamp will be in. Default is UTC", + "schema": { + "$ref": "#/components/schemas/TimestampFormat" + } } ], "responses": { @@ -608,8 +616,7 @@ "type": "object", "properties": { "time": { - "type": "string", - "format": "date-time" + "$ref": "#/components/schemas/Timestamp" }, "value": { "type": "number", @@ -621,6 +628,17 @@ "value" ] }, + "Timestamp": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "string", + "format": "date-time" + } + ] + }, "SystemTable": { "description": "Available metrics tables in the `system` database", "type": "string", @@ -628,6 +646,14 @@ "asynchronous_metric_log", "metric_log" ] + }, + "TimestampFormat": { + "description": "Which format should the timestamp be in.", + "type": "string", + "enum": [ + "utc", + "unix_epoch" + ] } }, "responses": { From 7c09457588902b26fa1c9a4d1f8e9cc7b54f0e42 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 25 Nov 2024 13:10:20 +1300 Subject: [PATCH 12/16] tidy up --- clickhouse-admin/types/src/lib.rs | 1 - oximeter/db/src/client/mod.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index b4f46457f4..5e2e63ed46 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1099,7 +1099,6 @@ pub struct MetricInfoPath { pub struct TimeSeriesSettingsQuery { /// The interval to collect monitoring metrics in seconds. /// Default is 60 seconds. - // TODO: How can I actually get the default in the API spec? #[serde(default = "default_interval")] pub interval: u64, /// Range of time to collect monitoring metrics in seconds. diff --git a/oximeter/db/src/client/mod.rs b/oximeter/db/src/client/mod.rs index b3a53a5cbc..e8db4a4b61 100644 --- a/oximeter/db/src/client/mod.rs +++ b/oximeter/db/src/client/mod.rs @@ -1156,7 +1156,7 @@ impl Client { // block. // // TODO-robustness This currently does no validation of the statement. - pub async fn execute_with_block( + async fn execute_with_block( &self, sql: &str, ) -> Result { From 98c73b6b30171d66468cd67290bc4858c6e156b4 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 25 Nov 2024 13:56:27 +1300 Subject: [PATCH 13/16] add endpoint to single node --- clickhouse-admin/api/src/lib.rs | 13 ++- clickhouse-admin/src/http_entrypoints.rs | 15 +++ clickhouse-admin/types/src/lib.rs | 43 +++++--- openapi/clickhouse-admin-single.json | 121 +++++++++++++++++++++++ 4 files changed, 178 insertions(+), 14 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index 540c3933be..3902de11d9 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -151,5 +151,16 @@ pub trait ClickhouseAdminSingleApi { rqctx: RequestContext, ) -> Result; - // TODO: Retrieve time series here too + /// Retrieve time series from the system database. The value the average of all + /// values within the interval. + /// These are internal ClickHouse metrics. + #[endpoint { + method = GET, + path = "/timeseries/{table}/{metric}/avg" + }] + async fn system_timeseries_avg( + rqctx: RequestContext, + path_params: Path, + query_params: Query, + ) -> Result>, HttpError>; } diff --git a/clickhouse-admin/src/http_entrypoints.rs b/clickhouse-admin/src/http_entrypoints.rs index 4cd7c1673b..9379e8102f 100644 --- a/clickhouse-admin/src/http_entrypoints.rs +++ b/clickhouse-admin/src/http_entrypoints.rs @@ -171,4 +171,19 @@ impl ClickhouseAdminSingleApi for ClickhouseAdminSingleImpl { Ok(HttpResponseUpdatedNoContent()) } + + async fn system_timeseries_avg( + rqctx: RequestContext, + path_params: Path, + query_params: Query, + ) -> Result>, HttpError> { + let ctx = rqctx.context(); + let retrieval_settings = query_params.into_inner(); + let metric_info = path_params.into_inner(); + let settings = + SystemTimeSeriesSettings { retrieval_settings, metric_info }; + let output = + ctx.clickhouse_cli().system_timeseries_avg(settings).await?; + Ok(HttpResponseOk(output)) + } } diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index 5e2e63ed46..ac86993d8b 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1049,7 +1049,7 @@ fn default_timestamp_format() -> TimestampFormat { TimestampFormat::Utc } -#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[derive(Clone, Copy, Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] /// Available metrics tables in the `system` database pub enum SystemTable { @@ -1067,7 +1067,7 @@ impl fmt::Display for SystemTable { } } -#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[derive(Clone, Copy, Debug, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] /// Which format should the timestamp be in. pub enum TimestampFormat { @@ -1078,8 +1078,8 @@ pub enum TimestampFormat { impl fmt::Display for TimestampFormat { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let table = match self { - TimestampFormat::Utc => "utc", - TimestampFormat::UnixEpoch => "unix_epoch", + TimestampFormat::Utc => "iso", + TimestampFormat::UnixEpoch => "unix_timestamp", }; write!(f, "{}", table) } @@ -1122,17 +1122,34 @@ pub struct SystemTimeSeriesSettings { } impl SystemTimeSeriesSettings { - // TODO: Use more aggregate functions than just avg? + fn interval(&self) -> u64 { + self.retrieval_settings.interval + } + + fn time_range(&self) -> u64 { + self.retrieval_settings.time_range + } + + fn timestamp_format(&self) -> TimestampFormat { + self.retrieval_settings.timestamp_format + } + fn metric_name(&self) -> &str { + &self.metric_info.metric + } + + fn table(&self) -> SystemTable { + self.metric_info.table + } + + // TODO: Use more aggregate functions than just avg? pub fn query_avg(&self) -> String { - let interval = self.retrieval_settings.interval; - let time_range = self.retrieval_settings.time_range; - let metric_name = &self.metric_info.metric; - let table = &self.metric_info.table; - let ts_fmt = match &self.retrieval_settings.timestamp_format { - TimestampFormat::Utc => "iso", - TimestampFormat::UnixEpoch => "unix_timestamp", - }; + let interval = self.interval(); + let time_range = self.time_range(); + let metric_name = self.metric_name(); + let table = self.table(); + let ts_fmt = self.timestamp_format(); + let avg_value = match table { SystemTable::MetricLog => metric_name, SystemTable::AsynchronousMetricLog => "value", diff --git a/openapi/clickhouse-admin-single.json b/openapi/clickhouse-admin-single.json index 74763957ca..c1b8bbf950 100644 --- a/openapi/clickhouse-admin-single.json +++ b/openapi/clickhouse-admin-single.json @@ -26,6 +26,83 @@ } } } + }, + "/timeseries/{table}/{metric}/avg": { + "get": { + "summary": "Retrieve time series from the system database. The value the average of all", + "description": "values within the interval. These are internal ClickHouse metrics.", + "operationId": "system_timeseries_avg", + "parameters": [ + { + "in": "path", + "name": "metric", + "description": "Name of the metric to retrieve.", + "required": true, + "schema": { + "type": "string" + } + }, + { + "in": "path", + "name": "table", + "description": "Table to query in the `system` database", + "required": true, + "schema": { + "$ref": "#/components/schemas/SystemTable" + } + }, + { + "in": "query", + "name": "interval", + "description": "The interval to collect monitoring metrics in seconds. Default is 60 seconds.", + "schema": { + "type": "integer", + "format": "uint64", + "minimum": 0 + } + }, + { + "in": "query", + "name": "time_range", + "description": "Range of time to collect monitoring metrics in seconds. Default is 86400 seconds (24 hrs).", + "schema": { + "type": "integer", + "format": "uint64", + "minimum": 0 + } + }, + { + "in": "query", + "name": "timestamp_format", + "description": "Format in which each timeseries timestamp will be in. Default is UTC", + "schema": { + "$ref": "#/components/schemas/TimestampFormat" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Array_of_SystemTimeSeries", + "type": "array", + "items": { + "$ref": "#/components/schemas/SystemTimeSeries" + } + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } } }, "components": { @@ -48,6 +125,50 @@ "message", "request_id" ] + }, + "SystemTimeSeries": { + "description": "Retrieved time series from the internal `system` database.", + "type": "object", + "properties": { + "time": { + "$ref": "#/components/schemas/Timestamp" + }, + "value": { + "type": "number", + "format": "double" + } + }, + "required": [ + "time", + "value" + ] + }, + "Timestamp": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "string", + "format": "date-time" + } + ] + }, + "SystemTable": { + "description": "Available metrics tables in the `system` database", + "type": "string", + "enum": [ + "asynchronous_metric_log", + "metric_log" + ] + }, + "TimestampFormat": { + "description": "Which format should the timestamp be in.", + "type": "string", + "enum": [ + "utc", + "unix_epoch" + ] } }, "responses": { From 5fa54f5b92d6b9b6b170099a65213c804dbbff06 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 25 Nov 2024 18:35:53 +1300 Subject: [PATCH 14/16] Add some tests --- Cargo.lock | 2 +- Cargo.toml | 2 +- clickhouse-admin/types/src/lib.rs | 123 ++++++++++++++++++++++++++- openapi/clickhouse-admin-server.json | 6 +- openapi/clickhouse-admin-single.json | 6 +- 5 files changed, 128 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a667cacef7..1138d6f3ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1440,7 +1440,7 @@ dependencies = [ [[package]] name = "clickward" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/clickward?rev=a1b342c2558e835d09e6e39a40d3de798a29c2f#a1b342c2558e835d09e6e39a40d3de798a29c2f5" +source = "git+https://github.com/oxidecomputer/clickward?rev=242fd812aaeafec99ba01b5505ffbb2bd2370917#242fd812aaeafec99ba01b5505ffbb2bd2370917" dependencies = [ "anyhow", "camino", diff --git a/Cargo.toml b/Cargo.toml index 8f4ba04ad1..4832a99e99 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -336,7 +336,7 @@ clickhouse-admin-server-client = { path = "clients/clickhouse-admin-server-clien clickhouse-admin-single-client = { path = "clients/clickhouse-admin-single-client" } clickhouse-admin-types = { path = "clickhouse-admin/types" } clickhouse-admin-test-utils = { path = "clickhouse-admin/test-utils" } -clickward = { git = "https://github.com/oxidecomputer/clickward", rev = "a1b342c2558e835d09e6e39a40d3de798a29c2f" } +clickward = { git = "https://github.com/oxidecomputer/clickward", rev = "242fd812aaeafec99ba01b5505ffbb2bd2370917" } cockroach-admin-api = { path = "cockroach-admin/api" } cockroach-admin-client = { path = "clients/cockroach-admin-client" } cockroach-admin-types = { path = "cockroach-admin/types" } diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index ac86993d8b..79b58ffef7 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1186,15 +1186,33 @@ impl SystemTimeSeriesSettings { } } -#[derive(Debug, Display, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Display, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(untagged)] pub enum Timestamp { - Unix(String), Utc(DateTime), + Unix(String), +} + +impl FromStr for Timestamp { + type Err = Error; + + fn from_str(s: &str) -> Result { + if let Ok(t) = s.parse() { + Ok(Timestamp::Utc(t)) + } else if let Some(_) = DateTime::from_timestamp( + s.parse() + .with_context(|| format!("{s} is not a valid time format"))?, + 0, + ) { + Ok(Timestamp::Unix(s.to_string())) + } else { + bail!("{s} is not a valid time format") + } + } } /// Retrieved time series from the internal `system` database. -#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(rename_all = "snake_case")] pub struct SystemTimeSeries { pub time: Timestamp, @@ -1216,6 +1234,12 @@ impl SystemTimeSeries { let mut m = vec![]; for line in s.lines() { + // serde_json deserialises f64 types with loss of precision at times. + // For example, in our tests some of the values to serialize have a + // fractional value of `.33333`, but once parsed, they become `.33331`. + // + // We do not require this level of precision, so we'll leave as is. + // Just noting that we are aware of this slight inaccuracy. let item: SystemTimeSeries = serde_json::from_str(line)?; m.push(item); } @@ -1239,6 +1263,7 @@ mod tests { ClickhouseHost, DistributedDdlQueue, KeeperConf, KeeperId, KeeperServerInfo, KeeperServerType, KeeperSettings, Lgif, LogLevel, RaftConfig, RaftServerSettings, ServerId, ServerSettings, + SystemTimeSeries, }; fn log() -> slog::Logger { @@ -2078,4 +2103,96 @@ snapshot_storage_disk=LocalSnapshotDisk "missing field `entry_version` at line 1 column 454", ); } + + #[test] + fn test_unix_epoch_system_timeseries_parse_success() { + let log = log(); + let data = "{\"time\":\"1732494720\",\"value\":110220450825.75238} +{\"time\":\"1732494840\",\"value\":110339992917.33333} +{\"time\":\"1732494960\",\"value\":110421854037.33333}\n" + .as_bytes(); + let timeseries = SystemTimeSeries::parse(&log, data).unwrap(); + + let expected = vec![ + SystemTimeSeries { + time: crate::Timestamp::Unix("1732494720".to_string()), + value: 110220450825.75238, + }, + SystemTimeSeries { + time: crate::Timestamp::Unix("1732494840".to_string()), + value: 110339992917.33331, + }, + SystemTimeSeries { + time: crate::Timestamp::Unix("1732494960".to_string()), + value: 110421854037.33331, + }, + ]; + + assert_eq!(timeseries, expected); + } + + #[test] + fn test_utc_system_timeseries_parse_success() { + let log = log(); + let data = + "{\"time\":\"2024-11-25T00:34:00Z\",\"value\":110220450825.75238} +{\"time\":\"2024-11-25T00:35:00Z\",\"value\":110339992917.33333} +{\"time\":\"2024-11-25T00:36:00Z\",\"value\":110421854037.33333}\n" + .as_bytes(); + let timeseries = SystemTimeSeries::parse(&log, data).unwrap(); + + let expected = vec![ + SystemTimeSeries { + time: crate::Timestamp::Utc( + "2024-11-25T00:34:00Z".parse::>().unwrap(), + ), + value: 110220450825.75238, + }, + SystemTimeSeries { + time: crate::Timestamp::Utc( + "2024-11-25T00:35:00Z".parse::>().unwrap(), + ), + value: 110339992917.33331, + }, + SystemTimeSeries { + time: crate::Timestamp::Utc( + "2024-11-25T00:36:00Z".parse::>().unwrap(), + ), + value: 110421854037.33331, + }, + ]; + + assert_eq!(timeseries, expected); + } + + #[test] + fn test_misshapen_system_timeseries_parse_fail() { + let log = log(); + let data = "{\"bob\":\"1732494720\",\"value\":110220450825.75238}\n" + .as_bytes(); + let result = SystemTimeSeries::parse(&log, data); + + let error = result.unwrap_err(); + let root_cause = error.root_cause(); + + assert_eq!( + format!("{}", root_cause), + "missing field `time` at line 1 column 47", + ); + } + + #[test] + fn test_time_format_system_timeseries_parse_fail() { + let log = log(); + let data = "{\"time\":2024,\"value\":110220450825.75238}\n".as_bytes(); + let result = SystemTimeSeries::parse(&log, data); + + let error = result.unwrap_err(); + let root_cause = error.root_cause(); + + assert_eq!( + format!("{}", root_cause), + "data did not match any variant of untagged enum Timestamp at line 1 column 12", + ); + } } diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index 05042fbc68..2f00a86564 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -630,12 +630,12 @@ }, "Timestamp": { "anyOf": [ - { - "type": "string" - }, { "type": "string", "format": "date-time" + }, + { + "type": "string" } ] }, diff --git a/openapi/clickhouse-admin-single.json b/openapi/clickhouse-admin-single.json index c1b8bbf950..f86a4956b7 100644 --- a/openapi/clickhouse-admin-single.json +++ b/openapi/clickhouse-admin-single.json @@ -145,12 +145,12 @@ }, "Timestamp": { "anyOf": [ - { - "type": "string" - }, { "type": "string", "format": "date-time" + }, + { + "type": "string" } ] }, From 6661776b8effdef6da5fdf66c55d84ddb9c192e1 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 25 Nov 2024 19:02:16 +1300 Subject: [PATCH 15/16] output unix time as an int rather than string --- clickhouse-admin/types/src/lib.rs | 46 ++++++++++++------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index 79b58ffef7..d4a8a31d51 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1155,8 +1155,14 @@ impl SystemTimeSeriesSettings { SystemTable::AsynchronousMetricLog => "value", }; + // This formats the unix timestamp as an integer rather than a string + let int_timestamp = match ts_fmt { + TimestampFormat::UnixEpoch => "::INT", + TimestampFormat::Utc => "", + }; + let mut query = format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({avg_value}) AS value + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND){int_timestamp} AS time, avg({avg_value}) AS value FROM system.{table} WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} " @@ -1190,25 +1196,7 @@ impl SystemTimeSeriesSettings { #[serde(untagged)] pub enum Timestamp { Utc(DateTime), - Unix(String), -} - -impl FromStr for Timestamp { - type Err = Error; - - fn from_str(s: &str) -> Result { - if let Ok(t) = s.parse() { - Ok(Timestamp::Utc(t)) - } else if let Some(_) = DateTime::from_timestamp( - s.parse() - .with_context(|| format!("{s} is not a valid time format"))?, - 0, - ) { - Ok(Timestamp::Unix(s.to_string())) - } else { - bail!("{s} is not a valid time format") - } - } + Unix(u64), } /// Retrieved time series from the internal `system` database. @@ -1227,7 +1215,6 @@ impl SystemTimeSeries { info!( log, "Retrieved data from `system` database"; - // TODO: Log output here? feels like a lot of noise? "output" => ?s ); @@ -2107,23 +2094,23 @@ snapshot_storage_disk=LocalSnapshotDisk #[test] fn test_unix_epoch_system_timeseries_parse_success() { let log = log(); - let data = "{\"time\":\"1732494720\",\"value\":110220450825.75238} -{\"time\":\"1732494840\",\"value\":110339992917.33333} -{\"time\":\"1732494960\",\"value\":110421854037.33333}\n" + let data = "{\"time\":1732494720,\"value\":110220450825.75238} +{\"time\":1732494840,\"value\":110339992917.33333} +{\"time\":1732494960,\"value\":110421854037.33333}\n" .as_bytes(); let timeseries = SystemTimeSeries::parse(&log, data).unwrap(); let expected = vec![ SystemTimeSeries { - time: crate::Timestamp::Unix("1732494720".to_string()), + time: crate::Timestamp::Unix(1732494720), value: 110220450825.75238, }, SystemTimeSeries { - time: crate::Timestamp::Unix("1732494840".to_string()), + time: crate::Timestamp::Unix(1732494840), value: 110339992917.33331, }, SystemTimeSeries { - time: crate::Timestamp::Unix("1732494960".to_string()), + time: crate::Timestamp::Unix(1732494960), value: 110421854037.33331, }, ]; @@ -2184,7 +2171,8 @@ snapshot_storage_disk=LocalSnapshotDisk #[test] fn test_time_format_system_timeseries_parse_fail() { let log = log(); - let data = "{\"time\":2024,\"value\":110220450825.75238}\n".as_bytes(); + let data = "{\"time\":\"2024-11-25\",\"value\":110220450825.75238}\n" + .as_bytes(); let result = SystemTimeSeries::parse(&log, data); let error = result.unwrap_err(); @@ -2192,7 +2180,7 @@ snapshot_storage_disk=LocalSnapshotDisk assert_eq!( format!("{}", root_cause), - "data did not match any variant of untagged enum Timestamp at line 1 column 12", + "data did not match any variant of untagged enum Timestamp at line 1 column 20", ); } } From 59b0f5773d87bd54d51241653a606cd25aafeabb Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 26 Nov 2024 12:04:27 +1300 Subject: [PATCH 16/16] address comments --- clickhouse-admin/api/src/lib.rs | 12 +++++++---- clickhouse-admin/src/clickhouse_cli.rs | 4 ++-- clickhouse-admin/types/src/lib.rs | 30 +++++++++++--------------- openapi/clickhouse-admin-server.json | 4 ++-- openapi/clickhouse-admin-single.json | 4 ++-- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/clickhouse-admin/api/src/lib.rs b/clickhouse-admin/api/src/lib.rs index 3902de11d9..d9e742d764 100644 --- a/clickhouse-admin/api/src/lib.rs +++ b/clickhouse-admin/api/src/lib.rs @@ -118,8 +118,10 @@ pub trait ClickhouseAdminServerApi { rqctx: RequestContext, ) -> Result>, HttpError>; - /// Retrieve time series from the system database. The value the average of all - /// values within the interval. + /// Retrieve time series from the system database. + /// + /// The value of each data point is the average of all stored data points + /// within the interval. /// These are internal ClickHouse metrics. #[endpoint { method = GET, @@ -151,8 +153,10 @@ pub trait ClickhouseAdminSingleApi { rqctx: RequestContext, ) -> Result; - /// Retrieve time series from the system database. The value the average of all - /// values within the interval. + /// Retrieve time series from the system database. + /// + /// The value of each data point is the average of all stored data points + /// within the interval. /// These are internal ClickHouse metrics. #[endpoint { method = GET, diff --git a/clickhouse-admin/src/clickhouse_cli.rs b/clickhouse-admin/src/clickhouse_cli.rs index c6557cfb30..3d8a1b076e 100644 --- a/clickhouse-admin/src/clickhouse_cli.rs +++ b/clickhouse-admin/src/clickhouse_cli.rs @@ -11,7 +11,7 @@ use clickhouse_admin_types::{ }; use dropshot::HttpError; use illumos_utils::{output_to_exec_error, ExecutionError}; -use slog::{info, Logger}; +use slog::{debug, Logger}; use slog_error_chain::{InlineErrorChain, SlogInlineError}; use std::collections::BTreeSet; use std::ffi::OsStr; @@ -175,7 +175,7 @@ impl ClickhouseCli { let log = self.log.clone().unwrap(); let query = settings.query_avg(); - info!(&log, "Querying system database"; "query" => &query); + debug!(&log, "Querying system database"; "query" => &query); self.client_non_interactive( ClickhouseClientType::Server, diff --git a/clickhouse-admin/types/src/lib.rs b/clickhouse-admin/types/src/lib.rs index d4a8a31d51..3b8696438c 100644 --- a/clickhouse-admin/types/src/lib.rs +++ b/clickhouse-admin/types/src/lib.rs @@ -1155,14 +1155,8 @@ impl SystemTimeSeriesSettings { SystemTable::AsynchronousMetricLog => "value", }; - // This formats the unix timestamp as an integer rather than a string - let int_timestamp = match ts_fmt { - TimestampFormat::UnixEpoch => "::INT", - TimestampFormat::Utc => "", - }; - let mut query = format!( - "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND){int_timestamp} AS time, avg({avg_value}) AS value + "SELECT toStartOfInterval(event_time, INTERVAL {interval} SECOND) AS time, avg({avg_value}) AS value FROM system.{table} WHERE event_date >= toDate(now() - {time_range}) AND event_time >= now() - {time_range} " @@ -1192,11 +1186,14 @@ impl SystemTimeSeriesSettings { } } +// Our OpenAPI generator does not allow for enums to be of different +// primitive types. Because Utc is a "string" in json, Unix cannot be an int. +// This is why we set it as a `String`. #[derive(Debug, Display, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(untagged)] pub enum Timestamp { Utc(DateTime), - Unix(u64), + Unix(String), } /// Retrieved time series from the internal `system` database. @@ -2094,23 +2091,23 @@ snapshot_storage_disk=LocalSnapshotDisk #[test] fn test_unix_epoch_system_timeseries_parse_success() { let log = log(); - let data = "{\"time\":1732494720,\"value\":110220450825.75238} -{\"time\":1732494840,\"value\":110339992917.33333} -{\"time\":1732494960,\"value\":110421854037.33333}\n" + let data = "{\"time\":\"1732494720\",\"value\":110220450825.75238} +{\"time\":\"1732494840\",\"value\":110339992917.33333} +{\"time\":\"1732494960\",\"value\":110421854037.33333}\n" .as_bytes(); let timeseries = SystemTimeSeries::parse(&log, data).unwrap(); let expected = vec![ SystemTimeSeries { - time: crate::Timestamp::Unix(1732494720), + time: crate::Timestamp::Unix("1732494720".to_string()), value: 110220450825.75238, }, SystemTimeSeries { - time: crate::Timestamp::Unix(1732494840), + time: crate::Timestamp::Unix("1732494840".to_string()), value: 110339992917.33331, }, SystemTimeSeries { - time: crate::Timestamp::Unix(1732494960), + time: crate::Timestamp::Unix("1732494960".to_string()), value: 110421854037.33331, }, ]; @@ -2171,8 +2168,7 @@ snapshot_storage_disk=LocalSnapshotDisk #[test] fn test_time_format_system_timeseries_parse_fail() { let log = log(); - let data = "{\"time\":\"2024-11-25\",\"value\":110220450825.75238}\n" - .as_bytes(); + let data = "{\"time\":2024,\"value\":110220450825.75238}\n".as_bytes(); let result = SystemTimeSeries::parse(&log, data); let error = result.unwrap_err(); @@ -2180,7 +2176,7 @@ snapshot_storage_disk=LocalSnapshotDisk assert_eq!( format!("{}", root_cause), - "data did not match any variant of untagged enum Timestamp at line 1 column 20", + "data did not match any variant of untagged enum Timestamp at line 1 column 12", ); } } diff --git a/openapi/clickhouse-admin-server.json b/openapi/clickhouse-admin-server.json index 2f00a86564..c82c7c0d8e 100644 --- a/openapi/clickhouse-admin-server.json +++ b/openapi/clickhouse-admin-server.json @@ -76,8 +76,8 @@ }, "/timeseries/{table}/{metric}/avg": { "get": { - "summary": "Retrieve time series from the system database. The value the average of all", - "description": "values within the interval. These are internal ClickHouse metrics.", + "summary": "Retrieve time series from the system database.", + "description": "The value of each data point is the average of all stored data points within the interval. These are internal ClickHouse metrics.", "operationId": "system_timeseries_avg", "parameters": [ { diff --git a/openapi/clickhouse-admin-single.json b/openapi/clickhouse-admin-single.json index f86a4956b7..b00bf56314 100644 --- a/openapi/clickhouse-admin-single.json +++ b/openapi/clickhouse-admin-single.json @@ -29,8 +29,8 @@ }, "/timeseries/{table}/{metric}/avg": { "get": { - "summary": "Retrieve time series from the system database. The value the average of all", - "description": "values within the interval. These are internal ClickHouse metrics.", + "summary": "Retrieve time series from the system database.", + "description": "The value of each data point is the average of all stored data points within the interval. These are internal ClickHouse metrics.", "operationId": "system_timeseries_avg", "parameters": [ {