Worktree simplify sla verdicts - #39
Merged
Merged
Conversation
The read path answers two separate requirements, so the benchmark now judges and reports two verdicts instead of one. Neither is folded into the other. The SLA family covers all four endpoints. Each floor is that endpoint's share of the 500 rps Standard-tier watermark from the team RPC SLA doc section 3.4, with the mix getTransaction 0.60, getEvents 0.20, getTransactions 0.15, getLedgers 0.05. Each endpoint answers for the p99 of section 4.1, one per storage tier: hot is the Live window, cold is Recent on frozen NVMe. Historical on frozen EBS is out of scope, because the benchmark boxes carry no EBS tier. The floors hold in every phase and every dataset profile, so targets.json stores them once. The end-to-end-budget probe covers getTransaction alone, at the demand-derived floors of work item 856, and answers for one number: an in-RPC p99 of 10 ms, its slice of the transaction-lifecycle budget. The scheduled p99 of that cell is reported and not judged. getTransaction carries both families in one leg. Its rate list is the union of the two ladders, deduplicated and sorted, so no new leg machinery is needed. Where the two floors coincide the shared cell carries both verdicts. getEvents' mean-page budget is deleted. The SLA states a p99 per endpoint and nothing else, so the mean page is reported and judged by nothing. docs/sla-derivation.md records where every number comes from, including the Little's-law derivation that was rejected once section 3.4 became available. Published runs under docs/runs keep the verdicts they were converted with, and the viewers still render them.
Viewer: the SLA check is looked up by kind, as SCHEMA.md now tells readers to, with applies_to kept only as the legacy fallback. One queryTargets(D) resolver replaces the thrFor / perEndpoint / qThr triad both renderers spelled out, so the 500 ms legacy default has one definition. The SLA and E2E-budget tables share cellTd / noteEl / tierTag / underTarget instead of each carrying its own copies. The SLA table decides per row whether the in-RPC time is judged elsewhere, from that row's own verdict_e2e, rather than from a run-level flag. The E2E table renders whatever entry carries a verdict_e2e instead of reading .txhash by name, and takes its header budget from the verdicts. Query figures are numbered by a counter in emission order rather than a hand-kept slot map. Converter: the query checks are appended once after the keep-up branch instead of in every arm. A test pins each SLA floor to aggregate_rps x mix. targets.json: the derivation block keeps only the numeric inputs; the prose lives in docs/sla-derivation.md and no longer rides into every run JSON. Runner: sortedKeys is slices.Sorted(maps.Keys); the one-use extraKeys helper is inlined; Rates reads the SLA floor with a map lookup instead of a separate membership test. Smoke: measuredGet is computed once (the second copy was not e2e-aware, so the footnote check never ran on split runs; it now does and allows the probe-leg suffix). hasE2EFamily and wantE2E share one selector; the ladder-row filter reuses the qout it already had. gen-fixtures picks the latency level as the nearest ladder rung in log space instead of hardcoded cutoffs, and reuses the converter's rps_cells.
Contributor
|
There was a problem hiding this comment.
🟡 Changes recommended
The current queryCheck fallback can select the wrong "queries" check after the split (picking the E2E probe instead of the SLA), leading to incorrect threshold resolution and test/viewer behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the benchmarks’ “open-loop queries” model to support two independent verdict families (SLA vs E2E-budget probe), flowing the new semantics consistently through target derivation (docs/targets.json), conversion, rendering, runner rate planning, and smoke/fixture tests.
Changes:
- Split query checks/verdicts into
query_sla+verdict_sla(per endpoint+tier targets) andquery_e2e_probe+verdict_e2e(txhash-only, demand-rate, in-RPC budget). - Update runner target loading/rate-ladder planning to union SLA + probe ladders for
txhash, while other endpoints use SLA ladder only. - Refresh fixtures/smoke tests and documentation to reflect per-row SLA thresholds and a separate E2E table.
File summaries
| File | Description |
|---|---|
| tests/smoke/smoke.mjs | Smoke assertions updated for split verdict families, per-row targets, and separate E2E table. |
| tests/smoke/gen-fixtures.py | Fixture generator updated to produce new SLA/probe shapes and unioned txhash ladders. |
| SCHEMA.md | Schema docs updated for new check/verdict fields and legacy compatibility notes. |
| runner/README.md | Runner documentation updated for SLA vs E2E-probe floors and txhash ladder union. |
| runner/internal/targets/targets.go | Target model refactored to SLA + E2E probe; Rates() updated for ladder union. |
| runner/internal/targets/targets_test.go | Tests updated to validate new targets schema and expected ladders. |
| runner/internal/plan/plan_test.go | Clarifies golden plan rates are pinned inputs (not derived from current SLA floors). |
| runner/cmd/campaign/run_test.go | Expected CLI --target-rps output updated for new SLA floors. |
| runner/cmd/campaign/main_test.go | Plan command output updated for unioned txhash ladder and SLA-based rates. |
| docs/targets.json | Replaces legacy single query target with two-family query_load model and derivation metadata. |
| docs/summary.js | Summary extraction updated to read `verdict_sla |
| docs/styles.css | Adds styling for “alternate requirement” figures (fig-alt). |
| docs/sla-derivation.md | New doc recording how SLA and probe floors/targets are derived. |
| docs/app.js | Viewer updated for per-endpoint thresholds, SLA vs E2E tables, and legacy behavior. |
| converter/tests/test_queries_rps.py | Converter tests updated for new verdict objects, tier-specific thresholds, and probe behavior. |
| converter/tests/test_phase.py | Phase/check emission tests updated for query_sla + query_e2e_probe. |
| converter/tests/fixtures.py | Default RPS fixtures updated to SLA floors and unioned txhash ladder. |
| converter/convert.py | Implements verdict_sla/verdict_e2e, emits two checks, and embeds updated query_load. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Match the read-path check on kind only (query_sla, then the legacy query_p99_threshold) in the viewer and the smoke mirror; the applies_to fallback could have picked the E2E probe check if query_sla were absent. - Rates sorts and deduplicates every endpoint's ladder, not only txhash, so the doc comment's 'ascending' holds regardless of ladder order in the file. - The missing-profile message names query_load.e2e_probe, the block it actually looks in, in both the runner and the converter.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.