Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

179 changes: 161 additions & 18 deletions asap-planner-rs/src/bin/optimizer_cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
use std::path::PathBuf;

use asap_planner::optimizer::{
load_optional_selected_atomic_cost_table, run_greedy_pipeline, AtomicCostTable, LabelSetFacts,
build_milp_workload, load_flat_atomic_cost_table, load_optional_selected_atomic_cost_table,
load_workload_facts, run_greedy_pipeline, solve_milp, AtomicCostTable, LabelSetFacts,
LabelSetFactsError,
};
use asap_planner::ControllerConfig;
use clap::Parser;
use rqe_optimizer::milp::Objective;

#[derive(Parser, Debug)]
#[command(
Expand All @@ -27,26 +30,57 @@ struct Args {
#[arg(long = "data-ingestion-interval-ms", value_parser = clap::value_parser!(u64).range(1..))]
data_ingestion_interval_ms: u64,

/// YAML label-set facts: `series_count` per (metric, spatial filter) and
/// `cardinality` per (metric, spatial filter, grouping labels).
#[arg(long = "label-set-facts")]
label_set_facts: PathBuf,

/// Path to the versioned atomic-cost document sketch-bench's `atomic-costs`
/// subcommand exports. Requires --atomic-cost-workload to select exactly
/// one measured workload profile. Omitted: every
/// benchmarked-family candidate (CMS/HLL/KLL) is dropped, since there is
/// no data to cost it at — only trivial accumulators and EXACT remain
/// selectable.
#[arg(long = "atomic-costs")]
/// Greedy only. YAML label-set facts: `series_count` per (metric, spatial
/// filter) and `cardinality` per (metric, spatial filter, grouping labels).
#[arg(
long = "label-set-facts",
required_unless_present = "milp",
conflicts_with = "milp"
)]
label_set_facts: Option<PathBuf>,

/// Greedy: the versioned atomic-cost document sketch-bench's
/// `atomic-costs` subcommand exports; requires --atomic-cost-workload.
/// Omitted: every benchmarked-family candidate (CMS/HLL/KLL) is dropped,
/// leaving only trivial accumulators and EXACT.
/// MILP: the flat cost table `export_rqe_optimizer_costs.sh` writes
/// (`rqe_atomic_costs.json`); required.
#[arg(long = "atomic-costs", required_if_eq("milp", "true"))]
atomic_costs: Option<PathBuf>,

/// JSON `profiles[].workload` value copied from the sketch-bench atomic-cost
/// document. This makes the empirical workload profile explicit and avoids
/// mixing costs from different traces or time windows.
#[arg(long = "atomic-cost-workload", requires = "atomic_costs")]
/// Greedy only. JSON `profiles[].workload` value copied from the

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: --label-set-facts has no conflicts_with = "milp", while --atomic-cost-workload does, so it is silently ignored under --milp.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5561781. --label-set-facts now has conflicts_with = "milp".

/// sketch-bench atomic-cost document. This makes the empirical workload
/// profile explicit and avoids mixing costs from different traces or time
/// windows.
#[arg(
long = "atomic-cost-workload",
requires = "atomic_costs",
conflicts_with = "milp"
)]
atomic_cost_workload: Option<PathBuf>,

/// Plan with sketch-bench's rqe-optimizer MILP and print the plan; writes
/// no configs yet.
#[arg(long)]
milp: bool,

/// MILP only. YAML workload facts: per metric, `cardinality` per label
/// set, including the set of all its labels (the series count).
#[arg(
long = "workload-facts",
required_if_eq("milp", "true"),
requires = "milp"
)]
workload_facts: Option<PathBuf>,

/// MILP only. Objective weight on CPU-sec/sec. Default: rqe-optimizer's.
#[arg(long = "w-cpu", requires = "milp", value_parser = parse_weight)]
w_cpu: Option<f64>,

/// MILP only. Objective weight on memory GiB. Default: rqe-optimizer's.
#[arg(long = "w-mem", requires = "milp", value_parser = parse_weight)]
w_mem: Option<f64>,

#[arg(short, long, action = clap::ArgAction::Count)]
verbose: u8,
}
Expand All @@ -64,7 +98,14 @@ fn main() -> anyhow::Result<()> {

let yaml_str = std::fs::read_to_string(&args.input_config)?;
let config: ControllerConfig = serde_yaml::from_str(&yaml_str)?;
let facts = LabelSetFacts::from_path(&args.label_set_facts)?;
if args.milp {
return run_milp(&args, &config);
}
let facts = LabelSetFacts::from_path(
args.label_set_facts
.as_deref()
.expect("clap requires --label-set-facts without --milp"),
)?;

let atomic_cost_table = match load_optional_selected_atomic_cost_table(
args.atomic_costs.as_deref(),
Expand Down Expand Up @@ -108,3 +149,105 @@ fn main() -> anyhow::Result<()> {

Ok(())
}

/// Objective weights must be finite and non-negative: a negative weight
/// rewards cost, and NaN poisons every coefficient.
fn parse_weight(s: &str) -> Result<f64, String> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An all-zero objective is accepted. 0 passes this check for both weights, and the default w_mem is already 0, so --w-cpu 0 makes every cost coefficient 0. HiGHS then returns an arbitrary feasible plan, which the CLI prints as optimal. Reject the case where w_cpu == 0 && w_mem == 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a7515f6. The CLI rejects --w-cpu and --w-mem both being 0.

match s.parse::<f64>() {
Ok(w) if w.is_finite() && w >= 0.0 => Ok(w),
Ok(w) => Err(format!("must be finite and >= 0, got {w}")),
Err(e) => Err(e.to_string()),
}
}

fn run_milp(args: &Args, config: &ControllerConfig) -> anyhow::Result<()> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: workload facts load against config.metrics.unwrap_or_default() before the MissingMetricHints check runs. A config with no metrics: section fails with "metric X has facts but no metrics: hint" instead of the intended MissingMetricHints error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5561781. A missing metrics: section now returns MissingMetricHints before facts are loaded, and the MILP path calls config.warn_default_slas().

config.warn_default_slas();
let Some(hints) = config.metrics.as_deref() else {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this repeats the MissingMetricHints check that extract_hinted_items already does inside build_milp_workload. Having one guard (e.g. let load_workload_facts take the config) would keep the two from diverging.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving as is. The CLI guard has to run before load_workload_facts, which needs the hints, and that happens before build_milp_workload runs. Moving the check into a facts loader that takes the config would just move the duplicate.

return Err(LabelSetFactsError::MissingMetricHints.into());
};
let facts = load_workload_facts(
args.workload_facts
.as_deref()
.expect("clap requires --workload-facts with --milp"),
hints,
args.data_ingestion_interval_ms,
)?;
let costs = load_flat_atomic_cost_table(
args.atomic_costs
.as_deref()
.expect("clap requires --atomic-costs with --milp"),
)?;
let Objective::AUCCost { w_cpu, w_mem } = Objective::default();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--w-cpu and --w-mem accept any f64. A negative value (for example --w-mem -1) rewards memory or makes the problem unbounded, and NaN makes every objective coefficient NaN. Suggest rejecting values that aren't finite and >= 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5561781. --w-cpu/--w-mem go through parse_weight: finite and >= 0 only (weights_must_be_finite_and_non_negative).

let (w_cpu, w_mem) = (args.w_cpu.unwrap_or(w_cpu), args.w_mem.unwrap_or(w_mem));
// All-zero weights make every plan cost 0, so the solver's pick is arbitrary.
anyhow::ensure!(
w_cpu > 0.0 || w_mem > 0.0,
"--w-cpu and --w-mem are both 0; at least one must be positive"
);
let objective = Objective::AUCCost { w_cpu, w_mem };
tracing::debug!(?objective, cost_rows = costs.len(), "milp: inputs loaded");

let workload = build_milp_workload(config, &facts, args.data_ingestion_interval_ms)?;
let (deployments, solution) = solve_milp(&workload, &facts, &costs, objective)?;

let mut active: Vec<usize> = solution.mapping.clone();
active.sort();
active.dedup();
println!("=== Deployments: {} ===", active.len());
for &d in &active {
let dep = &deployments[d];
println!(
" [{d}] {:?} {} config={} metric={} grouping={:?} window={}ms slide={}ms",
dep.capability,
dep.config.sketch,
dep.config.sketch_config,
dep.metric,
dep.grouping_labels,
dep.window_ms,
dep.slide_ms,
);
}
println!("\n=== Raqes: {} ===", workload.raqes.len());
for ((raqe, &d), latency_ms) in workload
.raqes
.iter()
.zip(&solution.mapping)
.zip(&solution.plan_cost.query_latency_ms)
{
println!(" {} -> [{d}] latency={latency_ms:.3e}ms", raqe.id);
}
let cost = &solution.plan_cost;
println!(
"\nobjective={:.6e} cpu={:.6e} cpu-sec/sec memory={:.3} MB",
objective.value(cost),
cost.cpu_secs_per_sec(),
cost.memory_bytes() / 1e6,
);
for (phase, c) in [
("ingest", &cost.ingest),
("merge", &cost.merge),
("query", &cost.query),
("storage", &cost.storage),
] {
println!(
" {phase}: cpu={:.6e} cpu-sec/sec memory={:.3} MB",
c.cpu_secs_per_sec,
c.memory_bytes / 1e6
);
}
Ok(())
}

#[cfg(test)]
mod tests {
use super::parse_weight;

#[test]
fn weights_must_be_finite_and_non_negative() {
assert_eq!(parse_weight("0.5"), Ok(0.5));
assert_eq!(parse_weight("0"), Ok(0.0));
for bad in ["-1", "NaN", "inf", "x"] {
assert!(parse_weight(bad).is_err(), "{bad}");
}
}
}
15 changes: 8 additions & 7 deletions asap-planner-rs/src/optimizer/aqe_extractor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ pub fn extract_aqes(
metric_schema: &PromQLSchema,
scrape_interval_ms: u64,
) -> Result<Vec<OptimizerItem>, OptimizerError> {
let mut acc: HashMap<OptimizerItemKey, (QueryRequirements, Vec<String>, f64)> = HashMap::new();
let mut acc: HashMap<OptimizerItemKey, (QueryRequirements, Vec<String>, usize)> =
HashMap::new();

for rqe in rqes {
if rqe.t_repeat_ms == 0 {
Expand All @@ -82,13 +83,11 @@ pub fn extract_aqes(
match extract_requirements(&leaf, metric_schema, scrape_interval_ms) {
Ok(req) => {
let key = OptimizerItemKey::from_rqe(&req, rqe);
let entry = acc.entry(key).or_insert_with(|| (req, Vec::new(), 0.0));
let entry = acc.entry(key).or_insert_with(|| (req, Vec::new(), 0));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated leaves within one query are counted twice. occurrences goes up once per matching leaf, so sum by (job)(x) / sum by (job)(x) gives occurrences = 2. build_milp_workload then emits two Raqes and charges query and merge CPU twice per interval, even though the query runs once. Should identical leaves within a single RQE be deduplicated?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping two Raqes: the engine evaluates each arm separately. handle_binary_expr_promql calls evaluate_binary_arm for lhs and rhs (asap-query-engine/src/engines/simple_engine/promql.rs:562,565), each running the full fetch and merge pipeline with no memo. Range queries build and run one context per arm (promql.rs:803-828), and there is no result cache. So sum(x) / sum(x) really pays query and merge CPU twice.

if !entry.1.contains(&leaf) {
entry.1.push(leaf);
}
// query_frequency_hz must stay in Hz (queries per real second)
// regardless of t_repeat_ms's internal unit — 1000.0 / ms, not 1.0 / ms.
entry.2 += 1000.0 / rqe.t_repeat_ms as f64;
entry.2 += 1;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: occurrences is incremented per leaf even when the same leaf repeats inside one query (the contains check dedups only query_strings). rate(x[5m]) / rate(x[5m]) * 100 at T=60s gives occurrences=2, so two Raqes (#0, #1) are emitted and query cost is charged twice per interval for one panel.

}
Err(reason) => {
return Err(OptimizerError::UnsupportedLeaf {
Expand All @@ -104,10 +103,12 @@ pub fn extract_aqes(
Ok(acc
.into_iter()
.map(
|(key, (requirements, query_strings, query_frequency_hz))| OptimizerItem {
|(key, (requirements, query_strings, occurrences))| OptimizerItem {
requirements,
query_strings,
query_frequency_hz,
// Hz (queries per real second): 1000.0 / ms, not 1.0 / ms.
query_frequency_hz: occurrences as f64 * 1000.0 / key.t_repeat_ms as f64,
occurrences,
t_repeat_ms: key.t_repeat_ms,
accuracy_sla: f64::from_bits(key.accuracy_sla_bits),
latency_sla_ms: key.latency_sla_ms_bits.map(f64::from_bits),
Expand Down
Loading
Loading