feat(observability): add sync pressure and source-cost flame graphs - #2005
feat(observability): add sync pressure and source-cost flame graphs#2005Bojan131 wants to merge 6 commits into
Conversation
Bojan131
changed the title
…odex/grafana-worker-queue-flamegraph-2003
Bojan131
changed the title
PR #2005 observability merge-readiness reviewPR: OriginTrail/dkg#2005 VerdictNot merge-ready: one introduced P1 remains. The worker-pressure flamegraph executes five instant Loki metric queries whose lookback is the entire Grafana-selected range. Against the repository's pinned Loki 3.3.2, one leaf query over only two representative log records expanded to 192 internal splits at 24 hours and reported 36–50 seconds of aggregate queue time. The panel repeats the expensive active and queued expressions three times each. That is an observability-induced backend load problem, and it directly contradicts this repository's runbook warning not to use long-range instant Loki metric queries. The actual short-window LogQL semantics are correct: the real Loki parser extracted moved array slots, preserved scheduler/lane/phase/operation labels, and returned the expected peaks. The W1 PromQL groupings are also correct at the reviewed head. The remaining two findings are regression-test gaps, not current production defects. Ground truth and scope
Findings[P1 / blocking / introduced by PR #2005] Selected-range instant LogQL makes the dashboard a source of Loki pressureThe flamegraph uses flowchart LR
R["Grafana selected range"] --> T["Five instant targets"]
T --> A["Active expression evaluated three times"]
T --> Q["Queued expression evaluated three times"]
A --> AS["Eight fixed-slot max_over_time branches"]
Q --> QS["Eight fixed-slot max_over_time branches"]
AS --> L["Loki splits the long lookback"]
QS --> L
This is the exact pattern the repository already warns about in I launched the repository's pinned A second cold-ish pass reported 36.54 seconds aggregate queue time for the same 24-hour query. These are Loki's aggregate query statistics, not claimed wall-clock latency; they demonstrate backend scheduling work. This was two log records and only one of the five Grafana targets. Recommended fix direction: make the flamegraph lookback explicitly short and bounded—one hour is consistent with the dashboard default and the runbook—and state that fixed scope in the panel title/description. Keep the heatmaps as the selected-range historical view. I verified the literal query change: the same 24-hour dashboard context with a fixed If full selected-range flame widths are a hard requirement, treat that as a query/telemetry redesign and benchmark it at 6/24 hours before merging. Do not apply “change instant to range and add a reduce transform” literally as the whole fix. I tested that shape with one-minute windows over 24 hours; it still produced 25 splits and 11–19 seconds aggregate queue time for one phase, before accounting for the repeated hierarchy targets. It also changes the returned frame from one value per series to many time samples; without a correctly ordered per-series Acceptance evidence should include the real Loki test described in the next finding, a 24-hour resource bound, and a rendered flamegraph check after any transform change. [P2 / non-blocking test hardening / introduced by PR #2005] CI proves generated query text, not executable LogQL resultsThe current verifier checks target count, substrings, eight array indexes, and transform IDs. It never asks Loki to parse/evaluate the query or verifies returned labels and values. I changed only the generated query source from The real pinned Loki execution then returned: The mutant reached the actual exported query builder and the generated dashboard, but every current check accepted it. This is especially relevant because the P1 fix must modify the same query mode/window/transform contract. Recommended fix direction: add a pinned Loki integration check to the observability workflow. Push two or three representative A parser-only request is not enough: the [P2 / non-blocking test hardening / introduced by PR #2005] Three decision panels lack result-level grouping testsThe reviewed W1 dashboard queries correctly retain:
However, Temporary result fixtures with two series differing only in the dropped label killed the mutants: Recommended fix direction: add those three two-series cases to the existing promtool dashboard-result fixture. Each pair must share all retained labels and differ only in the label being protected; otherwise dropping that label can still pass. This is small, local hardening and can land with the P1 fix, but the current production expressions are correct, so I would not block solely on it. Load-bearing code reviewed with no current defect found
Executed evidence at the restored headThe first dependency install and closure-build invocations exceeded their local command time caps without a compiler/test failure. Per the review rule, I did not trust those partial runs. Rerunning the same closure after dependencies were present completed cleanly: Mutation evidence: Immediately before writing this review, GitHub and the isolated checkout both reported head Merge readinessRequest one fix round. Bound or redesign the flamegraph query so a wider incident time range cannot multiply expensive instant Loki work, and prove the chosen shape against real Loki 3.3.2 at the maximum supported range. The real-LogQL result test should land with that fix because the current structural checks demonstrably accept an empty-result field mutation. The I5/I7/I8 promtool assertions are worthwhile low-cost hardening but are not independently blocking. No finding above is inherited from W1 or PR #2003. The Loki long-range behavior is an existing backend constraint already documented by this repository; the incompatible unbounded instant-query use is introduced by PR #2005. |
|
@Jurij89 addressed all three findings in
The panel/runbook documentation and PR description were updated to explain the fixed-snapshot versus selected-history split. Fresh GitHub CI is green, including the new real-Loki step and all generated PromQL result tests: https://github.com/OriginTrail/dkg/actions/runs/30808581609 |
| // with representative safe values. verify-profile-render.mjs independently | ||
| // performs the same substitution and compares the complete expression set, | ||
| // so this parser target cannot drift away from the dashboard it certifies. | ||
| const dashboardExprForPromtool = (expr) => expr |
There was a problem hiding this comment.
🟡 Issue: Use one Grafana-macro substitution contract for rendering and verification
What's wrong
The renderer and verifier are supposed to prove exact expression parity, but the normalization step that defines parity is duplicated. That weakens the abstraction: future dashboard variables or macro changes become a two-file coordination task in the most contract-sensitive part of the observability generator.
Example
Adding a new dashboard variable such as ${sync_transport:regex} would require updating both functions. If only the renderer is updated, the verifier compares against a differently substituted expression set; if only the verifier is updated, the promtool fixture still carries an unparsable Grafana token.
Suggested direction
Centralize the substitution list instead of copying it into both the renderer and verifier. This keeps the promtool fixture relationship explicit without relying on parallel hand-maintained replacement chains.
For Agents
Move the Grafana-to-promtool substitution map/function into a shared observability helper used by renderDashboardRules and verify-profile-render.mjs. Preserve the exact current substitutions for node/source/lane/outcome/rate_interval/range, and keep the expression-set comparison proving fixture parity.
3 participants
Dependency
This PR is temporarily based on
feat/w1-sync-measurementand depends on #2033. After #2033 merges, retarget this PR totestnet-canary; the resulting diff is only the observability extension.What changed
Loki worker-pressure diagnostics
[backpressure]records through the daemon's structured logger so they reach the existing redacted Loki pathW1 source-attributed sync cost
promtoolCI to parse both the fixed W1 packet and every dashboard expressionreason,include_shared_memory, oradmission, preserving their exact grouping and ratesWhy
The Loki views show where queue pressure accumulated from bounded transition/summary records without letting a long dashboard time range overload Loki. The fixed one-hour flamegraph is the recent pressure snapshot, while the heatmaps remain the selected-range historical view.
The new metrics dashboard complements that with #2033's source-attributed sync accounting, so operators can see what work was attempted, how much payload moved, how much active occupancy it consumed, and whether W1 has enough trustworthy evidence to draw a conclusion. A failed W1 gate is shown as inconclusive, not healthy; byte metrics are encoded application payload rather than wire bandwidth, and operations rejected before starting appear in I5 instead of receiving a fake zero-duration I4 sample.
Validation
missingAgeMsmutant with zero returned samplesreason,include_shared_memory, oradmissionfrom its respective grouping makes the matching test failgit diff --checkpassedNo scheduler admission policy, worker limit, queue behavior, or DKG runtime data path is changed by this PR.