Repository navigation
refactor(planner): rename latency_sla to optional latency_sla_ms #797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
69204f3
0d3f0a8
bc80c47
8d79801
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,16 +32,16 @@ pub struct ControllerConfig { | |
| } | ||
|
|
||
| impl ControllerConfig { | ||
| /// Warn if any query group has both SLAs at 0.0 (the serde Default), | ||
| /// which indicates `controller_options` was omitted from the config. | ||
| /// Warn if any query group still has the serde-default SLAs, which | ||
| /// indicates `controller_options` was omitted from the config. | ||
| pub fn warn_default_slas(&self) { | ||
| for qg in &self.query_groups { | ||
| let opts = &qg.controller_options; | ||
| if opts.accuracy_sla == 0.0 && opts.latency_sla == 0.0 { | ||
| if opts.accuracy_sla == 0.0 && opts.latency_sla_ms.is_none() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With omitted
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the earlier thread on this line: leaving it. A false positive needs an explicit |
||
| warn!( | ||
| query_group_id = ?qg.id, | ||
| "controller_options not set in query group; \ | ||
| accuracy_sla=0.0 and latency_sla=0.0 will be used — \ | ||
| accuracy_sla=0.0 and no latency_sla_ms limit will be used — \ | ||
| add controller_options to your config" | ||
| ); | ||
| } | ||
|
|
@@ -63,6 +63,7 @@ impl ControllerConfig { | |
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 8d79801. |
||
| pub struct QueryGroup { | ||
| pub id: Option<u32>, | ||
| pub queries: Vec<String>, | ||
|
|
@@ -79,11 +80,13 @@ pub struct QueryGroup { | |
| } | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 0d3f0a8. |
||
| #[derive(Debug, Clone, Deserialize, Default)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct ControllerOptions { | ||
| #[serde(deserialize_with = "deserialize_finite_f64")] | ||
| pub accuracy_sla: f64, | ||
| #[serde(deserialize_with = "deserialize_finite_f64")] | ||
| pub latency_sla: f64, | ||
| /// Maximum modeled query latency in milliseconds; `None` means no limit. | ||
| #[serde(default, deserialize_with = "deserialize_optional_positive_f64")] | ||
| pub latency_sla_ms: Option<f64>, | ||
| } | ||
|
|
||
| fn deserialize_finite_f64<'de, D>(deserializer: D) -> Result<f64, D::Error> | ||
|
|
@@ -98,6 +101,18 @@ where | |
| } | ||
| } | ||
|
|
||
| fn deserialize_optional_positive_f64<'de, D>(deserializer: D) -> Result<Option<f64>, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| match Option::<f64>::deserialize(deserializer)? { | ||
| Some(value) if !(value.is_finite() && value > 0.0) => Err(serde::de::Error::custom( | ||
| "must be a finite number greater than zero", | ||
| )), | ||
| value => Ok(value), | ||
| } | ||
| } | ||
|
|
||
| fn deserialize_positive_u64<'de, D>(deserializer: D) -> Result<u64, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
|
|
@@ -211,6 +226,7 @@ pub struct HllParams { | |
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct SQLControllerConfig { | ||
| pub query_groups: Vec<SQLQueryGroup>, | ||
| pub tables: Vec<TableDefinition>, | ||
|
|
@@ -220,6 +236,7 @@ pub struct SQLControllerConfig { | |
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct SQLQueryGroup { | ||
| pub id: Option<u32>, | ||
| pub queries: Vec<String>, | ||
|
|
@@ -237,13 +254,15 @@ pub struct TableDefinition { | |
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct ElasticDSLControllerConfig { | ||
| pub query_groups: Vec<ElasticDSLQueryGroup>, | ||
| pub sketch_parameters: Option<SketchParameterOverrides>, | ||
| pub aggregate_cleanup: Option<AggregateCleanupConfig>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct ElasticDSLQueryGroup { | ||
| pub id: Option<u32>, | ||
| pub queries: Vec<String>, | ||
|
|
@@ -265,7 +284,6 @@ query_groups: | |
| repetition_delay_ms: 60000 | ||
| controller_options: | ||
| accuracy_sla: .nan | ||
| latency_sla: .inf | ||
| "#; | ||
|
|
||
| let error = serde_yaml::from_str::<ControllerConfig>(yaml) | ||
|
|
@@ -283,13 +301,126 @@ query_groups: | |
| repetition_delay_ms: 60000 | ||
| controller_options: | ||
| accuracy_sla: 0.99 | ||
| latency_sla: 1.0 | ||
| latency_sla_ms: 250.0 | ||
| "#; | ||
|
|
||
| let config: ControllerConfig = serde_yaml::from_str(yaml).unwrap(); | ||
| let options = &config.query_groups[0].controller_options; | ||
| assert_eq!(options.accuracy_sla, 0.99); | ||
| assert_eq!(options.latency_sla, 1.0); | ||
| assert_eq!(options.latency_sla_ms, Some(250.0)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn omitted_latency_sla_ms_means_no_limit() { | ||
| let yaml = r#" | ||
| query_groups: | ||
| - queries: [sum(metric)] | ||
| repetition_delay_ms: 60000 | ||
| controller_options: | ||
| accuracy_sla: 0.99 | ||
| "#; | ||
|
|
||
| let config: ControllerConfig = serde_yaml::from_str(yaml).unwrap(); | ||
| assert_eq!( | ||
| config.query_groups[0].controller_options.latency_sla_ms, | ||
| None | ||
| ); | ||
| } | ||
|
|
||
| // Reject the unitless `latency_sla` key so it can't be silently ignored. | ||
| #[test] | ||
| fn rejects_legacy_latency_sla_key() { | ||
| let yaml = r#" | ||
| query_groups: | ||
| - queries: [sum(metric)] | ||
| repetition_delay_ms: 60000 | ||
| controller_options: | ||
| accuracy_sla: 0.99 | ||
| latency_sla: 1.0 | ||
| "#; | ||
|
|
||
| let error = serde_yaml::from_str::<ControllerConfig>(yaml) | ||
| .expect_err("legacy latency_sla key must be rejected") | ||
| .to_string(); | ||
| assert!(error.contains("unknown field `latency_sla`"), "{error}"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_misspelled_controller_options() { | ||
| // A typo would otherwise drop the whole SLA block to its defaults. | ||
| let yaml = r#" | ||
| query_groups: | ||
| - queries: [sum(metric)] | ||
| repetition_delay_ms: 60000 | ||
| controler_options: | ||
| accuracy_sla: 0.99 | ||
| "#; | ||
|
|
||
| let error = serde_yaml::from_str::<ControllerConfig>(yaml) | ||
| .expect_err("unknown query group key must be rejected") | ||
| .to_string(); | ||
| assert!( | ||
| error.contains("unknown field `controler_options`"), | ||
| "{error}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn sql_and_elastic_configs_reject_unknown_group_keys() { | ||
| let sql = r#" | ||
| tables: [] | ||
| query_groups: | ||
| - queries: ["SELECT 1"] | ||
| repetition_delay_ms: 60000 | ||
| latency_sla: 1.0 | ||
| controller_options: | ||
| accuracy_sla: 0.99 | ||
| "#; | ||
| let error = serde_yaml::from_str::<SQLControllerConfig>(sql) | ||
| .expect_err("unknown SQL group key must be rejected") | ||
| .to_string(); | ||
| assert!(error.contains("unknown field `latency_sla`"), "{error}"); | ||
|
|
||
| let elastic = r#" | ||
| query_groups: | ||
| - queries: ["{}"] | ||
| repetition_delay_ms: 60000 | ||
| index: i | ||
| time_field: t | ||
| controler_options: | ||
| accuracy_sla: 0.99 | ||
| "#; | ||
| let error = serde_yaml::from_str::<ElasticDSLControllerConfig>(elastic) | ||
| .expect_err("unknown Elastic group key must be rejected") | ||
| .to_string(); | ||
| assert!( | ||
| error.contains("unknown field `controler_options`"), | ||
| "{error}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_non_positive_or_non_finite_latency_sla_ms() { | ||
| for bad in ["0.0", "-5.0", ".inf", ".nan"] { | ||
| let yaml = format!( | ||
| r#" | ||
| query_groups: | ||
| - queries: [sum(metric)] | ||
| repetition_delay_ms: 60000 | ||
| controller_options: | ||
| accuracy_sla: 0.99 | ||
| latency_sla_ms: {bad} | ||
| "# | ||
| ); | ||
|
|
||
| let error = serde_yaml::from_str::<ControllerConfig>(&yaml) | ||
| .expect_err("invalid latency_sla_ms must be rejected") | ||
| .to_string(); | ||
| assert!( | ||
| error.contains("must be a finite number greater than zero"), | ||
| "{bad}: {error}" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ pub struct RQE { | |
| pub query_string: String, | ||
| pub t_repeat_ms: u64, | ||
| pub accuracy_sla: f64, | ||
| pub latency_sla: f64, | ||
| pub latency_sla_ms: Option<f64>, | ||
| } | ||
|
|
||
| /// Stable key for merging identical optimizer demand. | ||
|
|
@@ -36,7 +36,7 @@ struct OptimizerItemKey { | |
| topk_by_labels: Option<KeyByLabelNames>, | ||
| t_repeat_ms: u64, | ||
| accuracy_sla_bits: u64, | ||
| latency_sla_bits: u64, | ||
| latency_sla_ms_bits: Option<u64>, | ||
| } | ||
|
|
||
| impl OptimizerItemKey { | ||
|
|
@@ -51,7 +51,7 @@ impl OptimizerItemKey { | |
| topk_by_labels: req.topk_by_labels.clone(), | ||
| t_repeat_ms: rqe.t_repeat_ms, | ||
| accuracy_sla_bits: normalized_f64_bits(rqe.accuracy_sla), | ||
| latency_sla_bits: normalized_f64_bits(rqe.latency_sla), | ||
| latency_sla_ms_bits: rqe.latency_sla_ms.map(normalized_f64_bits), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The positive and finite rule for Also, no test checks that items with different
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -110,7 +110,7 @@ pub fn extract_aqes( | |
| query_frequency_hz, | ||
| t_repeat_ms: key.t_repeat_ms, | ||
| accuracy_sla: f64::from_bits(key.accuracy_sla_bits), | ||
| latency_sla: f64::from_bits(key.latency_sla_bits), | ||
| latency_sla_ms: key.latency_sla_ms_bits.map(f64::from_bits), | ||
| }, | ||
| ) | ||
| .collect()) | ||
|
|
@@ -236,7 +236,7 @@ mod tests { | |
| query_string: query.to_string(), | ||
| t_repeat_ms: t_ms, | ||
| accuracy_sla: 0.0, | ||
| latency_sla: 0.0, | ||
| latency_sla_ms: None, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -309,10 +309,24 @@ mod tests { | |
| assert_eq!(items.len(), 2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn different_latency_slas_become_distinct_items() { | ||
| let unlimited = rqe("sum_over_time(metric[5m])", 60_000); | ||
| let mut fast = rqe("sum_over_time(metric[5m])", 60_000); | ||
| fast.latency_sla_ms = Some(100.0); | ||
| let mut slow = rqe("sum_over_time(metric[5m])", 60_000); | ||
| slow.latency_sla_ms = Some(1_000.0); | ||
|
|
||
| let items = extract_aqes(&[unlimited, fast, slow], &empty_schema(), 15_000).unwrap(); | ||
| let mut slas: Vec<_> = items.iter().map(|item| item.latency_sla_ms).collect(); | ||
| slas.sort_by(|a, b| a.partial_cmp(b).unwrap()); | ||
| assert_eq!(slas, vec![None, Some(100.0), Some(1_000.0)]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn signed_zero_slas_merge_into_one_item() { | ||
| let mut negative_zero = rqe("sum_over_time(metric[5m])", 60_000); | ||
| negative_zero.latency_sla = -0.0; | ||
| negative_zero.accuracy_sla = -0.0; | ||
| let items = extract_aqes( | ||
| &[negative_zero, rqe("sum_over_time(metric[5m])", 60_000)], | ||
| &empty_schema(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warn_default_slastreatsaccuracy_sla == 0.0 && latency_sla_ms.is_none()as "controller_options missing". Omittinglatency_sla_msis now normal, so a valid explicit{accuracy_sla: 0.0}triggers a false warning. MakingQueryGroup.controller_optionsanOption<ControllerOptions>would make this exact instead of a guess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this as is. The false positive needs an explicit
accuracy_sla: 0.0, which means "accept any error" and is almost never intended, so a warning there is arguably still useful. Makingcontroller_optionsanOptionwould touch every caller for a log line. Happy to revisit if the warning becomes noisy.