PMM-14912 Dynamic thresholds - #5878
Draft
matejkubinec wants to merge 16 commits into
Draft
Conversation
Carry the design of record and the performance-testing results onto a branch based on main, so the backend can be implemented clean rather than on top of the proof-of-concept. The decision document records the design and the live measurements behind it; the performance document records the collector benchmarks that settled override-only emission over emitting a threshold for every target. Full authoring history for both remains on PMM-14912-dynamic-thresholds, along with the proof-of-concept they were written against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matej Kubinec <matej.kubinec@3pillarglobal.com>
Introduce the storage and metric layers for per-target alert threshold overrides, so tuning a threshold for one target is a data change rather than a rule change. Editing a rule resets alert state for every instance on it, which under `for: 5m` blinds every other target for five minutes when one is tuned. Migration 119 adds alert_rules, a registry of PMM-created rules holding the parameter snapshot Grafana cannot supply, and alert_rule_threshold_overrides. Its target column is polymorphic - a node id, a service id, or a cluster label value - so it carries no foreign key: cluster is a label value with no referent table. A CHECK rejects NaN and both infinities, this being the schema's first float column. Clearing an override tombstones the row instead of deleting it. A deleted row stops being emitted, and a series that stops being emitted keeps resolving for a full VictoriaMetrics lookbehind, so the clear takes minutes to land; changing the value of a live series takes one scrape. Measured end to end through a Grafana rule: 309s by absence, 14-21s by value change. ResolveThresholds is the single implementation of scope precedence, shared by the collector and later the API. Precedence cannot be expressed in PromQL - reducing operands to a common label set is what makes `or` prefer the left, and that reduction destroys the scope information needed to rank by - so nothing in the query backstops it. The order is service, node, cluster: a service runs on exactly one node and belongs to at most one cluster, so only that half is derivable; node over cluster is a convention, since the two cross-cut. The collector emits only overridden and tombstoned targets. Emitting a series per target instead scales with inventory rather than with what was actually tuned: measured at 1,000 nodes and 140 rule/parameter groups it consumed 77-82% of the 9s scrape budget, against 1-7% for this shape. Collect is bounded by a 3s timeout re-checked inside the emission loop, not only around the queries. The metric name and label set are shared constants rather than restated in the rule builder, so the generated query and the emitted series cannot drift - a rule pointing at a metric nobody emits fails silently, it simply never fires. Only node scope is reachable so far; service and cluster exist in the schema and resolver only. Templates gain an overridable flag, rejected on single-expression templates, which have no injection point yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Generate a threshold query step for every overridable parameter and point
the rule's expression at it, so the effective threshold is read from data
at evaluation time instead of being baked into the rule definition.
Tuning a threshold then never rewrites the rule, and so never resets
alert state for the other targets evaluated by it.
The injected step is:
max by (<join>) (label_replace(pmm_alert_threshold_override{...},
"<join>", "$1", "target", "(.*)"))
or (max by (<join>) (<the rule's own observed query>) * 0 + <default>)
Each clause earns its place. label_replace maps the collector's generic
target label onto whichever label this rule joins on; without it the two
operands carry different label sets and `or` returns both instead of
preferring the left. max by strips instance and job - the threshold is
scraped from pmm-managed, which can never match an observed series -
reduces both operands to one label set, and collapses the duplicate
series an HA cluster emits. The second clause manufactures the default
for every target the observed query reports, by reusing that query and
discarding its value, so the threshold shares the observed data's fate
and cannot go missing while the data it guards is still arriving.
The join label is derived from the parameter's scopes rather than
declared separately: a node override identifies its target by node name,
while service and cluster overrides both identify theirs by service name.
Mixing node with either of the other two is therefore incoherent, since a
rule joins on one label, and is now rejected when the template is parsed.
A parameter is paired with the query it is actually compared against -
the nearest query reference to its left - so a template comparing two
queries in one expression fans each default out over the right one.
Alert filters narrow the observed query but not the threshold, since a
filtered threshold would leave the targets the filter excludes with no
threshold at all.
The generated selector is asserted against the collector's own
descriptor. That pairing is the one failure mode with no runtime signal:
a rule selecting a metric nobody emits matches nothing and silently never
fires.
Rules with no PMM-minted ID generate exactly what they did before, so
none of this takes effect until the rule registry supplies one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mint a PMM identity for every rule created from a template with an overridable parameter, and persist what the threshold resolves against. This is what switches the feature on: until a rule has an ID to key overrides against, the builder emits no threshold step and the rule is generated exactly as before. CreateRule now mints the ID first, because it is the idempotency key for everything that follows, then stamps it on the Grafana rule as pmm_rule_id and stores a registry row keyed on it. The identity travels on the rule itself rather than being its Grafana UID, so a rule copied or renamed in Grafana can still be matched back to its overrides; the UID is only a cache of where the rule currently lives. The row carries a snapshot of each overridable parameter - default, join label, scopes, unit and range. The template it came from can be edited or deleted afterwards, so this is the only durable record of what the rule actually evaluates against, and of the range an override is validated within. The default stored is the value supplied at creation, not the template's, so a rule created with 42 has 42 as its baseline. The row is written before the rule reaches Grafana. The other order would leave a live rule whose thresholds can never be overridden and whose row may never arrive; this order can only leave an orphaned row, which the reconciler reaps, and a Grafana failure deletes it immediately. CreateRuleResponse gains rule_id so callers can address the rule's thresholds without looking it up. Regenerating for that field also picked up two formatting fixes in code from the previous commit that make format would otherwise have flagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Expose per-target threshold overrides so a threshold can be retuned without touching the rule. Four RPCs under /v1/alerting/thresholds: list, set, clear, and a batch verb applying several changes at once. The routes are generic rather than node-centric - scope and target travel as fields, not path segments. HTTP routes are additive-only, so shipping node-centric paths now would mean carrying a second route family forever once service and cluster scope land. A target cannot be a path segment in any case: a cluster target is an arbitrary label value, and one containing a slash would break gateway path matching outright. Threshold APIs require admin, matching Inventory where they are managed. Without an explicit rule they would inherit viewer from the "/v1/alerting" prefix, which would both let a read-only user retune every alert and expose thresholds outside the screen that gates them. This is a path rule rather than a method rule, so reads, writes and the batch verb are covered alike - prefix resolution stops at the colon, which the tests assert rather than assume. Batch updates apply in one transaction. A client editing several rows at once otherwise has no way to report which ones took effect after a partial failure, which is the whole reason the endpoint exists. The API needs to know which override is winning, not only its value, and deriving that in the service would have made precedence exist in two places. ResolveThresholds now delegates to ResolveThresholdsDetailed, which reports the winning row alongside the value, so what this API says and what the collector emits cannot drift. Two behaviours worth knowing rather than discovering: - Service and cluster scope return Unimplemented, not InvalidArgument. Both are already carried by the schema, the resolver and the proto, so enabling them is a validation change rather than an API change. - ListThresholds reports every overridable parameter when given a target, including untouched ones, and only actual overrides when not. Without a target there is no bounded set of targets to enumerate. Values are validated at the API as well as by the column's CHECK: a non-finite or out-of-range threshold reaching the database would surface as an opaque internal error rather than a bad request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Threshold overrides have no cascade to rely on. Their target column is polymorphic - a node id, a service id, or a cluster label value - so it carries no foreign key, and rows would otherwise outlive whatever they point at. Two paths close that: entity removal, and a sweep for rules deleted in Grafana. RemoveNode and RemoveService now delete a target's overrides in the same transaction as the entity itself, so an override can never survive its target even briefly. Removing a node reaches its services' overrides through the existing cascade into RemoveService, so there is no second cleanup path to keep in step. Cluster-scoped rows are deliberately never removed: a cluster with no services yet is dormant rather than stale, and should apply again when services join it. The reconciler reaps registry rows whose Grafana rule is gone, taking their overrides with them through the rule foreign key. It runs leader-only, since every replica shares one database and parallel sweeps would duplicate the same deletions. Rules are matched by the identity label PMM stamps on them rather than by Grafana UID, so a rule that was copied or renamed still counts as present. Two guards, both protecting configuration a user set by hand: - Rows younger than the grace period are spared. CreateRule writes the registry row before the rule reaches Grafana, so a sweep landing in that window would otherwise delete the row of a rule being created successfully. - A failed lookup aborts the sweep rather than being read as an empty Grafana, which would reap the entire registry. ListPMMRuleIDs is the only Grafana client surface this needs; the label constant moves to the services package so both sides reference one definition rather than repeating the string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turn the feature on for the first shipped template and let clients discover which parameters it applies to. pmm_node_high_cpu_load qualifies because it is multi-expression and aggregates by node_name, so the injected threshold query has an unambiguous label to join on. A golden-list test pins exactly which built-in templates are overridable, because marking one is not a free change: a template whose observed query does not carry the join label would generate a rule that silently never matches. Of the other three multi-expression templates, postgresql_high_transaction_rollbacks has no parameter at all, and the remaining two aggregate by service_name, so they belong with the service-scope increment. ParamDefinition gains an overridable field. Without it a client listing templates cannot tell which parameters are tunable, which is exactly what a UI needs in order to know what to render as editable. The models layer already carried the flag; only the API surface was missing it, so no unit test noticed. Verified end to end on a live server: creating a rule from this template injects the threshold step and registers the rule, an override reaches the rule's query through the collector, and clearing it returns the target to the default in 12 seconds rather than the five minutes that signalling a clear by absence would cost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add the Inventory modal for viewing and editing per-node alert thresholds, wired to the generic thresholds API. Scope and target travel as fields rather than path segments, matching the backend: a cluster target is an arbitrary label value, so it cannot be carried safely in a URL path. This screen only ever addresses nodes, so it pins the scope and leaves the rest of the API's reach unused. The form submits as one batch call rather than a request per row. Firing N requests would leave the modal half-applied after a partial failure, with no way to report which rows took effect; the batch endpoint applies every change in one transaction. Emptying a field, or typing the default back in, clears the override rather than writing the default as a new one, so the target falls back to the rule default or to a broader override still covering it. Rule titles are joined from the Grafana rules API on the pmm_rule_id label. They are deliberately absent from the thresholds response: rule metadata churns on rename, so Grafana stays authoritative for it. Rows whose rule has since been deleted keep an empty title rather than disappearing. Two details that are easy to get wrong and would fail quietly: - Numeric and boolean fields are optional because proto3 JSON omits zero values. A threshold of 0, or a row that is not overridden, arrives with the field absent rather than 0/false, so the row mapping coerces it. - A row is identified by rule, parameter and index. Two rules duplicated in Grafana share a rule id, which the API explicitly permits, so the first two parts alone would collapse two legitimate rows into one. The modal's message listener re-subscribes on every render, which is documented in place: GrafanaProvider's cleanup empties the shared listener array rather than removing only its own, so an ordinary navigation discards this subscription. Making the messenger's register/unregister symmetric is tracked separately. Content strings live in AlertThresholds.messages per convention. The reset button gains an accessible name, which it previously lacked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Move the submit diff out of the component and cover it, along with the row mapping, with unit tests. Deciding whether a row is a set, a clear or no change was inline in AlertThresholds, reachable only by rendering the modal. buildThresholdUpdates makes it a pure function alongside the existing row helpers, so the part most likely to be wrong is also the part that can be verified without a browser. The tests target the failure modes that would be silent rather than loud: - proto3 JSON omits zero values, so an absent default_value means 0 and not "unknown". Left uncoerced the table renders blanks where numbers belong. - Two rules duplicated in Grafana share a rule id, which the API explicitly permits, so rule and parameter together do not identify a row. Colliding ids would let one form field drive two rows. - Clearing omits `value` rather than sending the default. Sending it would pin the target to today's default and stop it following a later change to the rule. - An emptied input arrives as '' and must read as a clear, not as 0. - A title for a rule that no longer exists resolves to empty rather than breaking the row. Also normalises quote style in two files carried over from the previous commit, which make format rewrites and format-check would otherwise reject. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cover the threshold endpoints through the generated client, against a running server, so the HTTP surface is exercised rather than the service layer beneath it. The service-level tests already assert behaviour; these assert that it survives routing, JSON conversion and gRPC status mapping. Four tests, each pinning something that would fail quietly rather than loudly: - The lifecycle: an untouched target reports the rule default, setting an override reports it as effective, and clearing returns the target to the default while reporting it as no longer overridden. A tombstoned row reading as overridden would make every target ever tuned look tuned forever. - Validation, including that service and cluster scope answer Unimplemented rather than InvalidArgument. Both are already carried by the schema, the resolver and the proto, so the distinction is the difference between "not yet" and "malformed". - Batch: one invalid update rolls back the valid one alongside it. That transactional guarantee is the whole reason the endpoint exists, and it cannot be observed from a single-row test. - Removing a node takes its overrides with it. Those rows have no foreign key to cascade from - the target column is polymorphic - so this is the only thing proving the removal hook is wired. The fixture also asserts CreateRule returns a rule_id for an overridable template. Without one the feature is unreachable, and nothing else would notice if the field stopped being populated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Split a single-expression template apart at build time into the three steps a multi-expression template already produces - observed query, injected threshold, math comparison - so its threshold can be overridden per target. The template file is untouched; only the generated rule changes shape. This is what stood between 16 of the 43 shipped templates and an overridable threshold. Every one declares exactly one parameter, and it is always the threshold. The split is done on the PromQL AST rather than with a regexp. The parameter token is not valid PromQL, so it is replaced by a numeric sentinel padded to the token's exact byte length; AST positions then map 1:1 onto the original text, which lets the left-hand side be sliced out of the original string with the author's line breaks intact rather than reprinted from the AST. A regexp would have to special-case parentheses, the bool modifier and vector matching, and would fail silently by producing a plausible but wrong left-hand side. mysql_too_many_connections is the case that proves it: its `/ ignoring (job)` sits on a division inside the left-hand side, so any string search for vector matching rejects a template that is perfectly splittable. Two details the corpus forced: - The bool modifier is optional - 13 of the 16 use it, 3 do not - and is dropped either way, since Grafana math comparisons already yield 0/1. - The operator is carried across rather than assumed. Eight of the 16 compare with `<`, so assuming `>` would invert half of them. Grafana's `$value` is only a single scalar when a rule has one step, so a desugared rule would ship broken alert text. Annotations are repointed at the observed query: a bare reference also gains formatting, while one that pipes the value keeps its pipeline and is only renamed. The rename deliberately does not match `$values`, which shares its prefix and is already used by one template. Two things this surfaced rather than introduced: - The vector-matching guard the design sketch called for is unreachable. PromQL permits vector matching only between two instant vectors, and a threshold is a scalar, so such a template fails to parse whatever the threshold is - it could never have existed. Removed, with the reason recorded where it stood. - mongodb_replication_lag does not parse: `max(<range vector>[1m])` should be max_over_time. A pre-existing defect the splitter reports instead of hiding. The corpus test logs it rather than failing. No template is marked overridable yet, so nothing changes for anyone: desugaring only runs once a parameter opts in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Run golangci-lint under the toolchain go.mod pins. The binary in bin/ is a linux build and the host Go is newer than 1.26.6, which made the linter fail to typecheck the stdlib and silently suppress every other check. Extract thresholdsForRule from ListThresholds, which was over the cognitive complexity limit, and move the scope conversion ahead of the transaction so a bad scope is rejected without taking a connection. Replace hand-numbered query placeholders with whereAllEqual, which derives the numbering from column order. The numbers previously sat in a format string while the values they referred to were passed positionally to a separate call, so inserting a column meant renumbering by hand and a mistake matched on the wrong columns rather than erroring. Mark the validation subtests parallel. TestThresholdBatchUpdate stays sequential at both levels: its subtests drive one override row through set, clear and rollback in order, and the rollback case asserts against the state the clear left behind. Reword the doc comments godot flagged. Its autofix capitalises the first word, which renamed unexported identifiers in their own comments.
The design and performance-testing notes were working documents for building the feature, not reference material for maintaining it. The reasoning they carried that still matters lives as comments on the code it explains; the rest stays in git history.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5878 +/- ##
==========================================
+ Coverage 43.59% 46.70% +3.10%
==========================================
Files 415 427 +12
Lines 43134 44822 +1688
==========================================
+ Hits 18804 20933 +2129
+ Misses 22454 21815 -639
- Partials 1876 2074 +198 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add tests for the branches that carry logic rather than error plumbing: the minimum bound in checkThresholdValue, which was enforced but never asserted; the rule-id filter in thresholdRules; the comparator behind the table's row order; and both scope conversions, which only had their node arm exercised. Also cover targetNames for a target that has outlived its inventory entry, and IsOverridden, which reads as uncovered because it is called only from another package.
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
1 participant
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.
PMM-14912
Grafana: percona/grafana#912
FB: Percona-Lab/pmm-submodules#4449
Replaces #5579, which reviewed the POC. This branch reimplements the feature
on top of current
main, with a scope-based API rather than a node-centric one.What
Per-target alert threshold overrides. An operator can raise or lower the
threshold of an existing alert rule for one node, without editing the template
or duplicating the rule.
How
PostgreSQL is authoritative. Two tables back the feature: a registry row per
generated rule, and override rows keyed by
(rule_id, param_name, scope, target).VictoriaMetrics carries only what has been overridden, as the gauge
pmm_alert_threshold_override.The Grafana rule is written once at creation and never rewritten. An overridable
parameter becomes its own query step,
T_<param>, which reads the gauge andfalls back to the rule's own observed query for the default:
Reusing the rule's own observed expression for the fallback is deliberate - it
makes the threshold share the rule's fate. If the scrape target disappears, the
threshold series disappears with it rather than outliving the data it guards.
Clearing an override writes a tombstone instead of deleting the row, so the
emitted series changes value rather than vanishing. Measured on a live server,
this takes a clear from ~5 minutes down to ~12 seconds.
Precedence is
service->node->cluster, resolved in Go. It cannot beexpressed in PromQL: reducing both sides of an
orto a common label set is whatmakes
orprefer the left operand, and that reduction destroys the scopeinformation needed to rank by.
Scope of this PR
Node scope only. The schema, resolver and proto already carry service and
cluster, but those RPCs return
Unimplemented- service scope needs aservices-by-cluster lookup that is not in place yet.
Single-expression templates are desugared into the same three-step shape at build
time, so they can carry an overridable parameter too. The path is inert until a
template opts in; only
node_high_cpu_loaddoes so far.The threshold RPCs are admin-gated. The UI lives under inventory, where editors
have no access, so inheriting
viewerfrom/v1/alertingwould have been wrong.Testing
managed/models,managed/services/alerting,managed/pi/alertapi-tests/alerting/thresholds_test.go21-minute simulated scrape outage
make prepare-prcleanUI browser verification is still outstanding.