Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
31 changes: 23 additions & 8 deletions rqe-optimizer/examples/autosketch_vs_asap.rs

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.

(line 519) as Millis truncates. Lookbacks are now fractional seconds (1/rate), so a value like 0.0049999 would become 4 ms, which isn't a multiple of the 5 ms scrape. Use .round() here (and for interval_ms).

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 460e290: lookback_ms and interval_ms are .round()ed. No rate that divides 1000 truncates wrongly today (I checked all of them), but with the new assert a mis-rounded value would now fail loudly anyway.

Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,19 @@ const TRACES_TABLE: &str = "rqe-optimizer/data/autosketch-eval/table.json";
/// Objective weights of #777 §4: CPU only, then AWS Fargate (us-east-1,
/// Linux/x86) $/vCPU-hour and $/GB-hour, so the objective reads in $/hour.
const WEIGHTS: [(&str, f64, f64); 2] = [("cpu", 1.0, 0.0), ("fargate", 0.0405, 0.00445)];
/// Every stream is scraped once a second: windows and slides are whole
/// seconds, and the stream's samples/s become its series count.
/// A stream's scrape interval when its table gives no per-series rate: windows
/// and slides are whole seconds, and the stream's samples/s become its series
/// count.
const SCRAPE_MS: Millis = 1_000;
/// `ms` in seconds: an integer when whole, so second-grained plans read as
/// before, else fractional (an instant RQE's one-scrape window).
fn json_secs(ms: Millis) -> Value {
if ms.is_multiple_of(1000) {
json!(ms / 1000)
} else {
json!(ms as f64 / 1000.0)
}
}
const BYTES_PER_GIB: f64 = (1u64 << 30) as f64;
const SEED: u64 = 7;
/// AutoSketch's benchmark time per evaluated configuration, at the paper's
Expand Down Expand Up @@ -387,6 +397,12 @@ fn from_table(
};
let capability = capability_of(r);
let rate = r["label_set"]["arrival_rate_per_sec"].as_f64().unwrap();
// A metric with a per-series rate (the synthetic gauge and counter) is
// scraped at it, so an instant RQE's one-scrape lookback holds one
// sample per series. Record metrics (http, flows) keep SCRAPE_MS.
let scrape_ms = workload["e2e"]["metrics"][metric]["samples_per_sec_per_series"]

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.

The committed input tables (results/autosketch-vs-asap-hydra/inputs/tables/*.json, e2e-rate10/inputs/tables/*.json) still have the old 1 s instant lookback, but they already carry samples_per_sec_per_series (200/s and 10/s). This line now reads them with a 5 ms / 100 ms scrape.

Instant RQEs then get candidate windows of 5 ms, 10 ms, … up to 1 s instead of only 1 s, and temporal RQEs get sub-second windows too. So the candidate sets, MILP and plans no longer match the committed results built from these same inputs, and nothing flags the mismatch.

Two options: regenerate those tables, or keep SCRAPE_MS when the table's lookback is the old one.

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.

Measured: on the committed 200/s tables (classic, all, multigroup at shared = 1), main's runner and this PR's give byte-identical plans. The sub-second windows get enumerated, but the reported candidate count is unchanged (420 / 574 / 154) and so is the MILP. What doesn't reproduce is candidate build time (see the other thread). So I'm keeping the finer grain rather than special-casing old tables. Regenerating the committed tables with one-scrape instant RQEs goes with #213's §6.3 rerun. Numbers are in the PR description, "Candidate windows at 200/s".

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.

If workload.e2e.metrics[metric] is missing (no e2e block, or a key mismatch: e2e keys swap / for _), scrape_ms silently falls back to 1000. An instant RQE with a 5 ms lookback then gets no candidate windows, so evaluate drops it as unservable before validate_facts runs. Only the dropped count is logged. This should fail loudly when the lookback/interval aren't multiples of the scrape.

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 460e290. The runner now asserts that each RQE's lookback and interval are multiples of its stream's scrape, before it builds candidates. Checked by deleting e2e.metrics.data from a 10/s table: it now panics with …/sum_by_job/keys/1s: lookback 100 ms and interval 1000 ms must be multiples of …/job/value's 1000 ms scrape instead of dropping the RQE.

.as_u64()
.map_or(SCRAPE_MS, |per_sec| 1_000 / per_sec);

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.

1_000 / per_sec is integer division with no guard on the rate.

  • A rate that doesn't divide 1000 truncates. At --samples-per-sec 3, scrape_ms is 333. The temporal lookbacks (60000) and interval (1000) aren't multiples of 333, so lib.rs validation rejects every synthetic RQE. The same happens at 7, 30 and 300/s.
  • Above 1000/s, scrape_ms is 0 ("scrape interval is 0").
  • samples_per_sec_per_series: 0 panics with divide-by-zero.

This needs a check that the rate divides 1000, here and in the exporter.

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 813204b: the runner asserts that samples_per_sec_per_series is > 0 and divides 1000, so the scrape interval is whole ms. 3/s, 7/s, 0 and anything above 1000/s are rejected with a message naming the metric.

let groups = groups_of(r, capability);
// A schema metric's RQE names its grouping (its `groups` are that
// grouping's); otherwise the stream has one label `g`, the grouping.
Expand All @@ -406,11 +422,10 @@ fn from_table(
max_covered_share.insert(id.clone(), r["max_covered_share"].as_f64().unwrap());
}
// One metric per stream, labels {g, x} (or the schema's and x), and
// with one scrape a second the full label set has one series per
// sample/s. A stream's RQEs differ only in range and grouping, so
// the full label set has one series per sample per scrape interval. A stream's RQEs differ only in range and grouping, so
// take the largest per-RQE rate (conservative). Rates below `groups`
// samples/s are rounded up so every group has a series.
let series = (rate * SCRAPE_MS as f64 / 1000.0).round() as u64;
let series = (rate * scrape_ms as f64 / 1000.0).round() as u64;
let most = stream_series.entry(stream.clone()).or_default();
*most = (*most).max(series);
let most = *most;
Expand All @@ -436,7 +451,7 @@ fn from_table(
MetricFacts {
cardinality: [(labels.clone(), 0), (LabelSet::new(), 1)].into(),
labels,
scrape_interval_ms: SCRAPE_MS,
scrape_interval_ms: scrape_ms,

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.

With a sub-second scrape, temporal RQEs on synthetic streams also change, which contradicts the PR's claim that they don't. At 200/s, a 60 s lookback gives divisors(12000) = 48 candidate windows (5 ms … 60 s) instead of 12, and 24 h gives 220 instead of 96.

  • If the MILP picks, say, a 500 ms window/slide, the temporal plan emits a fractional window_secs, which ASAPQuery may not accept (design decision 4 is still unconfirmed).
  • The larger candidate set also inflates candidate_build_secs / milp_solve_secs / planning_secs, which are used to compare AutoSketch with ASAP.

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.

Agreed, my claim was wrong; the description now says temporal RQEs get scrape-grained candidates. Keeping them deliberately: the engine supports sub-second windows, so that's the real search space, and more candidates can only lower ASAP's cost. Measured at 200/s: the MILP picks no sub-second window for any temporal RQE, solve time is unchanged, and candidate build time grows ~3–4× (classic 0.18 → 0.77 s, all 0.30 → 0.94 s). ASAP's planning time stays under 1 s against AutoSketch's 600 s of benchmarking, so the planning-time comparison doesn't move. This is design decision 6 in the description.

value_range: None,
data_shape: BTreeMap::new(),
// A stream is one value of its metric, so one capability.
Expand Down Expand Up @@ -741,8 +756,8 @@ fn summarize(
"rqe": r.id,
"sketch": d.config.sketch,
"params": d.config.sketch_config["params"],
"window_secs": d.window_ms / 1000,
"slide_secs": d.slide_ms / 1000,
"window_secs": json_secs(d.window_ms),
"slide_secs": json_secs(d.slide_ms),
"merged_shards": r.lookback_ms / d.window_ms,
"ingest_workers": rqe_optimizer::analytical_cost_model::ingest_workers(d, &w.facts),
"latency_ms": latency,
Expand Down
12 changes: 7 additions & 5 deletions scripts/export_autosketch_eval_table.py

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.

(line 696) The docstring says the runner does not read the e2e block, but it now reads samples_per_sec_per_series from it for the scrape interval. test_classic_tables_are_main_s_all_tables_byte_for_byte deletes e2e before comparing (and its comment repeats the claim), so dropping or renaming that field wouldn't fail any test.

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 460e290. The e2e_block docstring and the test comment now say the runner reads samples_per_sec_per_series. Before the byte-for-byte test drops e2e, it asserts that every RQE metric has samples_per_sec_per_series == 200. Checked: renaming the field fails the test.

Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,9 @@ def synthetic_queries(windows=SYNTHETIC_WINDOWS, interval=SYNTHETIC_INTERVAL,
Streams: `series/*` hold one instance per series (C groups), `job/*` one per
job (J groups), and `label_0/*` one top-k sketch over all C series as keys.
`*/dist` streams feed quantile sketches, `*/value` sums, `*/increment`
increases. Spatial templates repeat every 1 s and read the last second
(S = T = 1 s); temporal ones repeat every `interval` seconds over each
increases. Spatial templates repeat every 1 s and, like a PromQL instant
query, read one sample per series: one scrape interval (S = 1 /
SYNTHETIC_SAMPLES_PER_SEC s, T = 1 s); temporal ones repeat every `interval` seconds over each
lookback in `windows`. RQEs that repeat another's query and range (template
10, D5) are kept once. Templates 7, `sum by (job) (rate(data_total[w]))`,
and 9, `topk(32, sum by (label_0) (rate(data_total[w])))`, are dropped
Expand All @@ -445,6 +446,7 @@ def synthetic_queries(windows=SYNTHETIC_WINDOWS, interval=SYNTHETIC_INTERVAL,
gauge `data`, increments from the counter `data_total`.
"""
rate = SYNTHETIC_SERIES * SYNTHETIC_SAMPLES_PER_SEC # samples/s over all series
scrape = 1 / SYNTHETIC_SAMPLES_PER_SEC # an instant query's lookback, in s

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.

1 / SYNTHETIC_SAMPLES_PER_SEC is float seconds, and nothing checks that it's a whole number of ms. The runner truncates it with (lookback_secs * 1000.0) as Millis, so at --samples-per-sec 3 or 7 the lookback becomes 333 / 142 ms. Neither lines up with the 1000 ms step or the temporal lookbacks, so the table can't be planned.

The exporter should reject rates that don't divide 1000.

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 813204b: the exporter rejects a --samples-per-sec that doesn't divide 1000 (parser.error). The new test test_samples_per_sec_must_divide_1000 covers 3, 0 and 2000.

dataset = dataset or "synthetic/" + point_id({"templates": templates, "shared": 1})
ranges = [(w, WINDOW_SECS[w]) for w in windows.split(",")]
seen = set() if seen is None else seen
Expand Down Expand Up @@ -487,7 +489,7 @@ def add(query_id, stream, capability, rng, s, t, groups, keys=None, promql=None)
add(f"quantile_over_time_p{q}", "series/dist", "quantile", label(rng), s,
interval, C)
for q in quantiles:
add(f"quantile_by_job_p{q}", "job/dist", "quantile", "1s", 1, 1, J)
add(f"quantile_by_job_p{q}", "job/dist", "quantile", "1s", scrape, 1, J)
for rng, s in [(w, WINDOW_SECS[w]) for w in DASHBOARD_FIXED_WINDOWS]:
if (rng, s) not in ranges:
continue
Expand All @@ -499,9 +501,9 @@ def add(query_id, stream, capability, rng, s, t, groups, keys=None, promql=None)
return schema_queries(interval, dataset, range_with_interval, seen, metric, e2e)
assert templates in ("classic", "all"), templates
quantiles = quantiles or SYNTHETIC_QUANTILES
add("sum_by_job", "job/value", "sum", "1s", 1, 1, J, promql=f"sum by (job) ({gauge})")
add("sum_by_job", "job/value", "sum", "1s", scrape, 1, J, promql=f"sum by (job) ({gauge})")
for q in quantiles:
add(f"quantile_by_job_p{q}", "job/dist", "quantile", "1s", 1, 1, J,
add(f"quantile_by_job_p{q}", "job/dist", "quantile", "1s", scrape, 1, J,
promql=f"quantile by (job) ({q}, {gauge})")
for rng, s in ranges:
lbl = label(rng)
Expand Down
13 changes: 9 additions & 4 deletions scripts/test_export_autosketch_eval_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,10 @@ def test_classic_templates_give_42_distinct_rqes(self):

def test_items_per_instance_follow_the_data_model(self):
qs = {(q["query_id"], q["range"]): q for q in synthetic_queries(templates="classic")}
# 1e4 series x 200 samples/s = 2e6 samples/s.
self.assertEqual(qs[("quantile_by_job_p0.5", "1s")]["max_N"], 2e5)
# 1e4 series x 200 samples/s = 2e6 samples/s; an instant query reads
# one scrape, one sample from each of a job's 1e3 series.
q = qs[("quantile_by_job_p0.5", "1s")]
self.assertEqual((q["range_s"], q["max_N"]), (0.005, 1e3))
self.assertEqual(qs[("quantile_over_time_p0.5", "15m")]["max_N"], 200 * 900)
top = qs[("topk32_sum_by_label0_sum_over_time", "24h")]
self.assertEqual((top["K"], top["groups"], top["max_N"]), (10_000, 1, 2e6 * 86400))
Expand Down Expand Up @@ -284,8 +286,8 @@ class SchemaTemplatesTest(unittest.TestCase):
def test_classic_tables_are_main_s_all_tables_byte_for_byte(self):
# The committed tables are the 10-template set's, from before "all"
# grew: renamed, today's "classic" tables match them byte for byte
# without templates 7 and 9's RQEs, apart from the `e2e` block, which
# the runner does not read.
# without templates 7 and 9's RQEs and with instant RQEs reading one
# scrape, apart from the `e2e` block, which the runner does not read.
with tempfile.TemporaryDirectory() as out:
main(["--synthetic", "--revision", "test", "--out", out])
classic = [n for n in sorted(os.listdir(out)) if "templatesclassic" in n]
Expand All @@ -298,6 +300,9 @@ def test_classic_tables_are_main_s_all_tables_byte_for_byte(self):
committed = json.load(f)
rqes = committed["workloads"][0]["rqes"]
rqes[:] = [r for r in rqes if not r["query_id"].startswith(DROPPED)]
for r in rqes: # instant RQEs now read one scrape (5 ms)
if r["range"] == "1s":
r.update(lookback_secs=0.005, min_N=1e3, max_N=1e3)
del ours["sketch_bench_revision"], committed["sketch_bench_revision"]
del ours["workloads"][0]["e2e"]
self.assertEqual(json.dumps(ours, sort_keys=True),
Expand Down
Loading