diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index f7e74f1730..f40904af02 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -6571,7 +6571,11 @@ impl NexusExternalApi for NexusExternalApiImpl { nexus .timeseries_query(&opctx, &query) .await - .map(|tables| HttpResponseOk(views::OxqlQueryResult { tables })) + .map(|tables| { + HttpResponseOk(views::OxqlQueryResult { + tables: tables.into_iter().map(Into::into).collect(), + }) + }) .map_err(HttpError::from) }; apictx @@ -6598,7 +6602,11 @@ impl NexusExternalApi for NexusExternalApiImpl { nexus .timeseries_query_project(&opctx, &project_lookup, &query) .await - .map(|tables| HttpResponseOk(views::OxqlQueryResult { tables })) + .map(|tables| { + HttpResponseOk(views::OxqlQueryResult { + tables: tables.into_iter().map(Into::into).collect(), + }) + }) .map_err(HttpError::from) }; apictx diff --git a/nexus/tests/integration_tests/metrics.rs b/nexus/tests/integration_tests/metrics.rs index 7b36814cbc..8ba283c13e 100644 --- a/nexus/tests/integration_tests/metrics.rs +++ b/nexus/tests/integration_tests/metrics.rs @@ -22,7 +22,7 @@ use nexus_test_utils::resource_helpers::{ use nexus_test_utils::wait_for_producer; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::shared::ProjectRole; -use nexus_types::external_api::views::OxqlQueryResult; +use nexus_types::external_api::views; use nexus_types::silo::DEFAULT_SILO_ID; use omicron_uuid_kinds::{GenericUuid, InstanceUuid}; use oximeter::TimeseriesSchema; @@ -270,17 +270,18 @@ async fn test_instance_watcher_metrics( #[track_caller] fn count_state( - table: &oxql_types::Table, + table: &views::OxqlTable, instance_id: InstanceUuid, state: &'static str, ) -> Result { use oxql_types::point::ValueArray; let uuid = FieldValue::Uuid(instance_id.into_untyped_uuid()); let state = FieldValue::String(state.into()); - let mut timeserieses = table.timeseries().filter(|ts| { - ts.fields.get(INSTANCE_ID_FIELD) == Some(&uuid) - && ts.fields.get(STATE_FIELD) == Some(&state) - }); + let mut timeserieses = + table.timeseries.clone().into_iter().filter(|ts| { + ts.fields.get(INSTANCE_ID_FIELD) == Some(&uuid) + && ts.fields.get(STATE_FIELD) == Some(&state) + }); let timeseries = timeserieses.next().ok_or_else(|| { MetricsNotYet::new(format!( "missing timeseries for instance {instance_id}, state {state}\n\ @@ -325,7 +326,7 @@ async fn test_instance_watcher_metrics( .system_timeseries_query_until(OXQL_QUERY, |metrics| { let checks = metrics .iter() - .find(|t| t.name() == "virtual_machine:check") + .find(|t| t.name == "virtual_machine:check") .ok_or_else(|| { MetricsNotYet::new("missing virtual_machine:check") })?; @@ -348,7 +349,7 @@ async fn test_instance_watcher_metrics( .system_timeseries_query_until(OXQL_QUERY, |metrics| { let checks = metrics .iter() - .find(|t| t.name() == "virtual_machine:check") + .find(|t| t.name == "virtual_machine:check") .expect("missing virtual_machine:check"); let ts1 = dbg!(count_state(&checks, instance1_uuid, STATE_STARTING)?); @@ -371,7 +372,7 @@ async fn test_instance_watcher_metrics( .system_timeseries_query_until(OXQL_QUERY, |metrics| { let checks = metrics .iter() - .find(|t| t.name() == "virtual_machine:check") + .find(|t| t.name == "virtual_machine:check") .expect("missing virtual_machine:check"); let ts1_starting = dbg!(count_state(&checks, instance1_uuid, STATE_STARTING)?); @@ -402,7 +403,7 @@ async fn test_instance_watcher_metrics( .system_timeseries_query_until(OXQL_QUERY, |metrics| { let checks = metrics .iter() - .find(|t| t.name() == "virtual_machine:check") + .find(|t| t.name == "virtual_machine:check") .expect("missing virtual_machine:check"); let ts1_starting = @@ -437,7 +438,7 @@ async fn test_instance_watcher_metrics( .system_timeseries_query_until(OXQL_QUERY, |metrics| { let checks = metrics .iter() - .find(|t| t.name() == "virtual_machine:check") + .find(|t| t.name == "virtual_machine:check") .expect("missing virtual_machine:check"); let ts1_starting = dbg!(count_state(&checks, instance1_uuid, STATE_STARTING)?); @@ -497,7 +498,7 @@ async fn test_project_timeseries_query( return Err(MetricsNotYet::new("waiting for table creation")); } assert_eq!(result.len(), 1); - if result[0].timeseries().len() == 0 { + if result[0].timeseries.is_empty() { return Err(MetricsNotYet::new( "waiting for timeseries population", )); @@ -511,11 +512,11 @@ async fn test_project_timeseries_query( .project_timeseries_query(&p1.identity.id.to_string(), q1) .await; assert_eq!(result.len(), 1); - assert!(result[0].timeseries().len() > 0); + assert!(!result[0].timeseries.is_empty()); let result = metrics_querier.project_timeseries_query("project2", q1).await; assert_eq!(result.len(), 1); - assert!(result[0].timeseries().len() > 0); + assert!(!result[0].timeseries.is_empty()); // with project specified let q2 = &format!("{} | filter project_id == \"{}\"", q1, p1.identity.id); @@ -523,11 +524,11 @@ async fn test_project_timeseries_query( let result = metrics_querier.project_timeseries_query("project1", q2).await; assert_eq!(result.len(), 1); // we get 2 timeseries because there are two instances - assert!(result[0].timeseries().len() == 2); + assert!(result[0].timeseries.len() == 2); let result = metrics_querier.project_timeseries_query("project2", q2).await; assert_eq!(result.len(), 1); - assert_eq!(result[0].timeseries().len(), 0); + assert_eq!(result[0].timeseries.len(), 0); // with instance specified let q3 = @@ -536,12 +537,12 @@ async fn test_project_timeseries_query( // project containing instance gives me something let result = metrics_querier.project_timeseries_query("project1", q3).await; assert_eq!(result.len(), 1); - assert_eq!(result[0].timeseries().len(), 1); + assert_eq!(result[0].timeseries.len(), 1); // should be empty or error let result = metrics_querier.project_timeseries_query("project2", q3).await; assert_eq!(result.len(), 1); - assert_eq!(result[0].timeseries().len(), 0); + assert_eq!(result[0].timeseries.len(), 0); // now let's test it with group_by let q4 = &format!( @@ -550,7 +551,7 @@ async fn test_project_timeseries_query( ); let result = metrics_querier.project_timeseries_query("project1", q4).await; assert_eq!(result.len(), 1); - assert_eq!(result[0].timeseries().len(), 2); + assert_eq!(result[0].timeseries.len(), 2); // test with a nested query let q5 = &format!( @@ -565,13 +566,13 @@ async fn test_project_timeseries_query( // we get two results, each contains one timeseries, and the instance ID // on each corresponds to the one we requested assert_eq!(result.len(), 2); - assert_eq!(result[0].timeseries().len(), 1); - let timeseries = result[0].timeseries().next().unwrap(); + assert_eq!(result[0].timeseries.len(), 1); + let timeseries = result[0].timeseries[0].clone(); let instance_id = timeseries.fields.get("instance_id").unwrap().to_string(); assert_eq!(instance_id, i1p1.identity.id.to_string()); - assert_eq!(result[1].timeseries().len(), 1); - let timeseries = result[1].timeseries().next().unwrap(); + assert_eq!(result[1].timeseries.len(), 1); + let timeseries = &result[1].timeseries[0]; let instance_id = timeseries.fields.get("instance_id").unwrap().to_string(); assert_eq!(instance_id, i2p1.identity.id.to_string()); @@ -639,10 +640,10 @@ async fn test_project_timeseries_query( .expect_status(Some(StatusCode::OK)); let result = NexusRequest::new(request) .authn_as(AuthnMode::UnprivilegedUser) - .execute_and_parse_unwrap::() + .execute_and_parse_unwrap::() .await; assert_eq!(result.tables.len(), 1); - assert_eq!(result.tables[0].timeseries().len(), 2); // two instances + assert_eq!(result.tables[0].timeseries.len(), 2); // two instances } #[nexus_test] @@ -780,7 +781,7 @@ async fn test_mgs_metrics( querier.system_timeseries_query_until(&query, |tables| { let table = tables .into_iter() - .find(|t| t.name() == name) + .find(|t| t.name == *name) .ok_or_else(|| { MetricsNotYet::new(format!( "failed to find table for {query}", @@ -791,7 +792,7 @@ async fn test_mgs_metrics( .keys() .map(|serial| (serial.clone(), 0)) .collect::>(); - for timeseries in table.timeseries() { + for timeseries in &table.timeseries { let fields = ×eries.fields; if timeseries.points.is_empty() { return Err(MetricsNotYet::new(format!( diff --git a/nexus/tests/integration_tests/metrics_querier.rs b/nexus/tests/integration_tests/metrics_querier.rs index 20bf10a18d..91c1eb5c7f 100644 --- a/nexus/tests/integration_tests/metrics_querier.rs +++ b/nexus/tests/integration_tests/metrics_querier.rs @@ -13,7 +13,7 @@ use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::resource_helpers::objects_list_page_authz; use nexus_types::external_api::params; -use nexus_types::external_api::views::OxqlQueryResult; +use nexus_types::external_api::views; use omicron_test_utils::dev::poll; use omicron_test_utils::dev::poll::CondCheckError; use omicron_test_utils::dev::poll::wait_for_condition; @@ -93,7 +93,7 @@ impl<'a, N> MetricsQuerier<'a, N> { cond: F, ) -> T where - F: Fn(Vec) -> Result, + F: Fn(Vec) -> Result, { self.timeseries_query_until("/v1/system/timeseries/query", query, cond) .await @@ -108,7 +108,7 @@ impl<'a, N> MetricsQuerier<'a, N> { cond: F, ) -> T where - F: Fn(Vec) -> Result, + F: Fn(Vec) -> Result, { self.timeseries_query_until( &format!("/v1/timeseries/query?project={project}"), @@ -128,7 +128,7 @@ impl<'a, N> MetricsQuerier<'a, N> { &self, project: &str, query: &str, - ) -> Vec { + ) -> Vec { self.project_timeseries_query_until(project, query, |tables| Ok(tables)) .await } @@ -270,7 +270,7 @@ impl<'a, N> MetricsQuerier<'a, N> { cond: F, ) -> T where - F: Fn(Vec) -> Result, + F: Fn(Vec) -> Result, { let result = wait_for_condition( || async { @@ -375,7 +375,7 @@ impl<'a, N> MetricsQuerier<'a, N> { // Try to parse the query as usual, which will fail on other kinds of // errors. TimeseriesQueryResult::Ok( - rsp.parsed_body::() + rsp.parsed_body::() .unwrap_or_else(|e| { panic!( "could not parse timeseries query response: {e:?}\n\ @@ -389,5 +389,5 @@ impl<'a, N> MetricsQuerier<'a, N> { enum TimeseriesQueryResult { TimeseriesNotFound, - Ok(Vec), + Ok(Vec), } diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index 2b8f828230..6cc509ebbb 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -1113,11 +1113,59 @@ pub struct AllowList { // OxQL QUERIES +/// A table represents one or more timeseries with the same schema. +/// +/// A table is the result of an OxQL query. It contains a name, usually the name +/// of the timeseries schema from which the data is derived, and any number of +/// timeseries, which contain the actual data. +// +// # Motivation +// +// This struct is derived from [`oxql_types::Table`] but presents timeseries data as a `Vec` +// rather than a map keyed by [`TimeseriesKey`]. This provides a cleaner JSON +// representation for external consumers, as these numeric keys are ephemeral +// identifiers that have no meaning to API consumers. Key ordering is retained +// as this is contructed from the already sorted values present in [`Table`]. +// +// When serializing a [`Table`] to JSON, the `BTreeMap` +// structure produces output with numeric keys like: +// ```json +// { +// "timeseries": { +// "2352746367989923131": { ... }, +// "3940108470521992408": { ... } +// } +// } +// ``` +// +// The `Table` view instead serializes timeseries as an array: +// ```json +// { +// "timeseries": [ { ... }, { ... } ] +// } +// ``` +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +pub struct OxqlTable { + /// The name of the table. + pub name: String, + /// The set of timeseries in the table, ordered by key. + pub timeseries: Vec, +} + +impl From for OxqlTable { + fn from(table: oxql_types::Table) -> Self { + OxqlTable { + name: table.name.clone(), + timeseries: table.into_iter().collect(), + } + } +} + /// The result of a successful OxQL query. #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] pub struct OxqlQueryResult { /// Tables resulting from the query, each containing timeseries. - pub tables: Vec, + pub tables: Vec, } // ALERTS diff --git a/openapi/nexus.json b/openapi/nexus.json index 7928ae4f55..3722de2144 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -22275,7 +22275,7 @@ "description": "Tables resulting from the query, each containing timeseries.", "type": "array", "items": { - "$ref": "#/components/schemas/Table" + "$ref": "#/components/schemas/OxqlTable" } } }, @@ -22283,6 +22283,27 @@ "tables" ] }, + "OxqlTable": { + "description": "A table represents one or more timeseries with the same schema.\n\nA table is the result of an OxQL query. It contains a name, usually the name of the timeseries schema from which the data is derived, and any number of timeseries, which contain the actual data.", + "type": "object", + "properties": { + "name": { + "description": "The name of the table.", + "type": "string" + }, + "timeseries": { + "description": "The set of timeseries in the table, ordered by key.", + "type": "array", + "items": { + "$ref": "#/components/schemas/Timeseries" + } + } + }, + "required": [ + "name", + "timeseries" + ] + }, "Password": { "title": "A password used to authenticate a user", "description": "Passwords may be subject to additional constraints.", @@ -25523,25 +25544,6 @@ "vlan_id" ] }, - "Table": { - "description": "A table represents one or more timeseries with the same schema.\n\nA table is the result of an OxQL query. It contains a name, usually the name of the timeseries schema from which the data is derived, and any number of timeseries, which contain the actual data.", - "type": "object", - "properties": { - "name": { - "type": "string" - }, - "timeseries": { - "type": "object", - "additionalProperties": { - "$ref": "#/components/schemas/Timeseries" - } - } - }, - "required": [ - "name", - "timeseries" - ] - }, "TargetRelease": { "description": "View of a system software target release.", "type": "object",