PMM-14912 Dynamic thresholds - #5579
Closed
matejkubinec wants to merge 13 commits into
Closed
Conversation
matejkubinec
force-pushed
the
PMM-14912-dynamic-thresholds
branch
from
July 22, 2026 12:11
cb7ad52 to
eab2635
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5579 +/- ##
==========================================
+ Coverage 43.59% 46.55% +2.95%
==========================================
Files 415 427 +12
Lines 43134 44750 +1616
==========================================
+ Hits 18804 20833 +2029
+ Misses 22454 21841 -613
- Partials 1876 2076 +200 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
@CodeRabbit full review |
Contributor
|
@coderabbitai full review |
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (42)
WalkthroughThe change adds per-node dynamic alert thresholds. It updates alert contracts and validation, persists rules and overrides, injects dynamic Grafana queries, exposes gRPC and Prometheus functionality, and adds PMM UI controls. ChangesDynamic alert thresholds
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ui/apps/pmm/src/contexts/grafana/grafana.provider.tsx (1)
66-167: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe
setFromGrafanaRefaddition has no effect whilesetFromGrafanastays in the dependency array.Lines 67-68 add
setFromGrafanaRefso the theme-change listener (Line 94-99) can call the latestsetFromGrafanawithout needing that value in the effect's dependency list. Line 167's dependency array is still[isLoaded, setFromGrafana, navigate]. BecausesetFromGrafanaremains a dependency, the effect still fully re-runs (messenger.unregister()then re-registers every listener in this effect) wheneversetFromGrafana's identity changes — the exact listener-churn behavior this ref was presumably meant to avoid, matching the commit history's note about a "workaround... to address messenger listener clearing issues".As written, the ref is redundant: with
setFromGrafanastill in the deps array, the effect already gets a fresh closure with the currentsetFromGrafanaon every re-run, so reading it through a ref changes nothing. DropsetFromGrafanafrom the dependency array to make the ref serve its intended purpose.🔧 Proposed fix
// eslint-disable-next-line react-hooks/exhaustive-deps - }, [isLoaded, setFromGrafana, navigate]); + }, [isLoaded, navigate]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/contexts/grafana/grafana.provider.tsx` around lines 66 - 167, Remove setFromGrafana from the dependency array of the effect that registers messenger listeners, while retaining setFromGrafanaRef.current for theme updates. Keep the effect dependent on isLoaded and navigate so the ref supplies the latest callback without causing listener unregister/re-register churn.managed/services/alerting/rule_builder.go (1)
136-176: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
buildMultiExpressionRuleDataagainst an emptyruleID, like the single-expression path does.
buildSingleExpressionRuleDatadisables threshold injection whenruleID == ""(falls back to the baked default), confirmed byTestBuildGrafanaRuleDataSingleExprEmptyRuleIDUnchanged.buildMultiExpressionRuleDatahas no such guard:allocateThresholdRefIDs(template)always allocates threshold refs, and the code always injectspmm_alert_threshold{rule_id="", param="..."}queries.With
ruleID == "", the injected query can never match a realpmm_alert_thresholdseries (real rule IDs are UUIDs), so the resulting math expression compares against a metric that is never emitted, instead of falling back to the template default. This is untested: rule_builder_dynamic_test.go has no multi-expression counterpart to the single-expression empty-ruleIDtest.🐛 Proposed fix to mirror the single-expression fallback
// Assign each overridable parameter a dedicated ref ID whose query resolves // the per-node threshold from the pmm_alert_threshold custom metric. The // expression steps then reference $<refID> instead of the baked-in literal. overridableRefs := allocateThresholdRefIDs(template) + if ruleID == "" { + // Mirror buildSingleExpressionRuleData: without a rule ID there is no + // pmm_alert_threshold series to join against, so fall back to baking + // the default value into the expression instead of injecting a query + // that can never match (rule_id=""). + overridableRefs = nil + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed/services/alerting/rule_builder.go` around lines 136 - 176, Update buildMultiExpressionRuleData to skip threshold-ref allocation and threshold query injection when ruleID is empty, matching buildSingleExpressionRuleData so expressions retain the template’s baked-in defaults. Only allocate overridableRefs and append threshold queries for non-empty ruleID, while preserving the existing query and expression construction paths.managed/data/alerting-templates/mysql_too_many_connections.yml (1)
18-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender the effective threshold in the alert description.
transformMapsreplaces[[ .threshold ]]with the rule-creation default, while the alert expression uses the per-nodeT_thresholdquery. Use{{ $values.T_threshold.Value }}so notifications show the effective threshold.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed/data/alerting-templates/mysql_too_many_connections.yml` around lines 18 - 25, Update the alert description annotation in the MySQL too-many-connections template to display the effective per-node threshold from the T_threshold query using the alert value reference, replacing the transformed [[ .threshold ]] placeholder while preserving the surrounding wording.
🧹 Nitpick comments (6)
ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.constants.tsx (2)
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated head-cell style.
The same
muiTableHeadCellProps.sxobject appears in three columns. Declare it once and reuse it.♻️ Proposed refactor
+const HEAD_CELL_PROPS = { + sx: { + '.Mui-TableHeadCell-Content': { + height: 40, + }, + }, +}; + export const ALERT_THRESHOLDS_COLUMNS: MRT_ColumnDef<AlertThresholdRow>[] = [Then set
muiTableHeadCellProps: HEAD_CELL_PROPSin each column.Also applies to: 44-50, 69-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.constants.tsx` around lines 30 - 36, Extract the repeated muiTableHeadCellProps.sx configuration into a shared HEAD_CELL_PROPS constant, then replace the duplicated inline objects in all three column definitions with muiTableHeadCellProps: HEAD_CELL_PROPS.
19-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the accessor with the rendered value in the "Parameter" column.
The column reads
summarybut rendersparamName. Sorting, filtering, and global search then act on text that the user does not see. This column also keeps column actions and filtering enabled, so the mismatch is reachable. UseparamNameas the accessor, or rendersummaryin the cell.♻️ Proposed fix
{ - accessorKey: 'summary', + accessorKey: 'paramName', header: 'Parameter', - Cell: ({ row: { original } }) => original.paramName, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.constants.tsx` around lines 19 - 23, Update the “Parameter” column definition to align its accessor with the value rendered by its Cell callback: use paramName consistently instead of summary, while preserving the existing displayed value and column filtering, sorting, and search behavior.ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.types.ts (1)
10-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the form value type match the real runtime value.
An MUI
TextInputwithtype: 'number'stores a string in react-hook-form state unless the field registersvalueAsNumber. The comment says "string while editing", but the type declaresnumber | undefined.AlertThresholds.tsxLine 82 then needs theraw as unknown === ''cast to work around the mismatch. Declare the union here and remove the cast downstream.♻️ Proposed type change
-// Form values: composite row id -> override value (string while editing). -export type AlertThresholdsFormValues = Record<string, number | undefined>; +// Form values: composite row id -> override value. +// The number input yields a string while editing, so accept both. +export type AlertThresholdsFormValues = Record< + string, + number | string | undefined +>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.types.ts` around lines 10 - 11, Update AlertThresholdsFormValues to allow string, number, or undefined values, matching the runtime state of the numeric TextInput during editing. In AlertThresholds.tsx, update the handling near the raw form value check to remove the raw-as-unknown cast and compare the value directly against the empty string.managed/pi/alert/overridable_test.go (1)
99-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd token whitespace variants to the valid table.
paramTokenRegexpand the final-comparison regexp both accept flexible whitespace inside the token, for example[[.threshold]]and[[ .threshold ]]. No case covers that behavior.♻️ Suggested additional cases
{name: "gt no bool", expr: "a > [[ .threshold ]]"}, + {name: "no inner spaces", expr: "a > [[.threshold]]"}, + {name: "extra inner spaces", expr: "a > [[ .threshold ]]"}, + {name: "trailing newline", expr: "a > bool [[ .threshold ]]\n"}, {name: "multiline", expr: "a\n> bool [[ .threshold ]]"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed/pi/alert/overridable_test.go` around lines 99 - 121, Add table cases in the overridable template validation test around singleExprOverridableTemplate to cover token whitespace variants, including compact [[.threshold]] and padded [[ .threshold ]] forms. Keep the existing comparison and multiline cases unchanged, and ensure both paramTokenRegexp and the final-comparison regexp whitespace behavior is exercised.managed/data/alerting-templates/postgresql_table_bloat_dual_threshold.yml (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm this template is intended to ship as a built-in.
The name
pmm_postgresql_table_bloat_dual_thresholdand the summary suffix "(dual threshold)" read like a fixture for exercising two overridable parameters. Built-in templates inmanaged/data/alerting-templatesare visible to all users. If this template exists only to test the dual-threshold path, move it to test data. If it is a product template, rename it so the title does not expose the implementation detail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed/data/alerting-templates/postgresql_table_bloat_dual_threshold.yml` around lines 3 - 5, Confirm whether pmm_postgresql_table_bloat_dual_threshold is intended as a user-visible built-in: if it only exercises overridable dual-threshold parameters, move the template to test data; otherwise rename the template and update its summary to describe the product-facing alert without exposing the implementation detail.managed/models/database.go (1)
1205-1215: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider an index on
node_id.The
UNIQUE (rule_id, param_name, node_id)constraint indexes lookups whose leading column isrule_id.models.FindThresholdOverridesByNodefilters only onnode_id, so it performs a sequential scan. That helper runs on theListNodeThresholdsrequest path. The row count grows with rules × overridable parameters × nodes.♻️ Proposed addition
PRIMARY KEY (id), UNIQUE (rule_id, param_name, node_id) )`, + `CREATE INDEX alert_rule_threshold_overrides_node_id_idx ON alert_rule_threshold_overrides (node_id)`, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed/models/database.go` around lines 1205 - 1215, Add a dedicated index on node_id to the alert_rule_threshold_overrides table definition alongside the existing UNIQUE constraint. Ensure models.FindThresholdOverridesByNode can use this index for node_id-only lookups without changing the existing uniqueness constraint or query behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@managed/data/alerting-templates/postgresql_table_bloat_dual_threshold.yml`:
- Around line 6-12: Update the alert template’s queries for refs A and B to use
custom exporter query definitions that collect the table bloat percentage and
real-size metrics, rather than directly referencing unavailable native series.
Preserve the existing metric mapping so the alert continues evaluating both
bloat thresholds.
In `@managed/models/alert_rule_helpers.go`:
- Around line 145-172: Replace the read-then-write logic in
UpsertThresholdOverride with one atomic INSERT ... ON CONFLICT (rule_id,
param_name, node_id) DO UPDATE statement using q.QueryRow and RETURNING to
populate AlertRuleThresholdOverride. Preserve the existing inputs, update Value
on conflicts, return the resulting row, and wrap query errors with the
threshold-override context.
In `@managed/services/alerting/service.go`:
- Around line 811-842: Prevent phantom alert-rule registry rows when Grafana
creation fails. In the CreateAlertRule rollback path, execute DeleteAlertRule
through a context independent of the cancelled request context, and add startup
or collector reconciliation to remove registry rows whose Grafana rules no
longer exist. Update the nearby comment to accurately describe orphan-row
behavior.
In `@managed/services/alerting/threshold_metrics.go`:
- Around line 108-119: Replace the per-node emission in the DefaultParams loop
with one unlabeled default metric per (rule, param) and additional node-labeled
metrics only for entries present in byParamNode overrides; update the metric
descriptor and rule-expression join as needed so node-specific values override
the default via group_left. If retaining the current shape instead, introduce a
documented product cap and warning when the series count exceeds it.
- Around line 70-95: Update AlertThresholdMetricsCollector.Collect to derive a
bounded-timeout context instead of using context.Background, load all overrides
once with models.FindAllThresholdOverrides, group them by RuleID before
processing rules, and reuse the grouped values rather than querying per rule.
Accumulate generated metrics in a slice during the transaction, then send them
to ch only after InTransactionContext succeeds; preserve the documented
no-metrics-on-failure behavior and handle the transaction error accordingly.
In `@managed/services/alerting/threshold_overrides.go`:
- Around line 90-125: The SetNodeThreshold transaction must handle concurrent
first-time writes without propagating a unique-constraint error. Update
models.UpsertThresholdOverride, used within SetNodeThreshold, to use INSERT ...
ON CONFLICT DO UPDATE for the existing (rule_id, param_name, node_id) uniqueness
constraint, or retry the operation in a fresh transaction while preserving the
current validation and result-building flow.
In `@ui/apps/pmm-compat/src/lib/events.ts`:
- Around line 27-30: Update OpenAlertThresholdsModalEvent to extend
BusEventWithPayload with the payload shape { nodeId: string; nodeName: string },
importing BusEventWithPayload from `@grafana/data`. Preserve the existing event
type value so compat.ts can forward e.payload containing the required node
identity.
In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.tsx`:
- Around line 76-113: Update handleSubmit to catch failures from
Promise.all(operations), report the mutation error through the existing
user-facing error notification mechanism, and return without calling handleClose
so the modal remains open for retry. Keep the success snackbar and close
behavior only on successful completion.
- Around line 57-68: Update GrafanaProvider cleanup so it removes only listeners
owned by that provider instead of clearing the shared CrossFrameMessenger
listener list via unregister(). Then update the AlertThresholds listener effect
to register OPEN_ALERT_THRESHOLDS_MODAL once with an empty dependency array and
retain its own handler cleanup, avoiding render-time re-registration and
listener gaps.
In
`@ui/apps/pmm/src/components/alert-thresholds/reset-value-cell/ResetValueCell.tsx`:
- Around line 17-21: Update the IconButton in ResetValueCell to include an
accessible aria-label and a matching title describing that it resets the value,
while preserving the existing onClick behavior and icon.
---
Outside diff comments:
In `@managed/data/alerting-templates/mysql_too_many_connections.yml`:
- Around line 18-25: Update the alert description annotation in the MySQL
too-many-connections template to display the effective per-node threshold from
the T_threshold query using the alert value reference, replacing the transformed
[[ .threshold ]] placeholder while preserving the surrounding wording.
In `@managed/services/alerting/rule_builder.go`:
- Around line 136-176: Update buildMultiExpressionRuleData to skip threshold-ref
allocation and threshold query injection when ruleID is empty, matching
buildSingleExpressionRuleData so expressions retain the template’s baked-in
defaults. Only allocate overridableRefs and append threshold queries for
non-empty ruleID, while preserving the existing query and expression
construction paths.
In `@ui/apps/pmm/src/contexts/grafana/grafana.provider.tsx`:
- Around line 66-167: Remove setFromGrafana from the dependency array of the
effect that registers messenger listeners, while retaining
setFromGrafanaRef.current for theme updates. Keep the effect dependent on
isLoaded and navigate so the ref supplies the latest callback without causing
listener unregister/re-register churn.
---
Nitpick comments:
In `@managed/data/alerting-templates/postgresql_table_bloat_dual_threshold.yml`:
- Around line 3-5: Confirm whether pmm_postgresql_table_bloat_dual_threshold is
intended as a user-visible built-in: if it only exercises overridable
dual-threshold parameters, move the template to test data; otherwise rename the
template and update its summary to describe the product-facing alert without
exposing the implementation detail.
In `@managed/models/database.go`:
- Around line 1205-1215: Add a dedicated index on node_id to the
alert_rule_threshold_overrides table definition alongside the existing UNIQUE
constraint. Ensure models.FindThresholdOverridesByNode can use this index for
node_id-only lookups without changing the existing uniqueness constraint or
query behavior.
In `@managed/pi/alert/overridable_test.go`:
- Around line 99-121: Add table cases in the overridable template validation
test around singleExprOverridableTemplate to cover token whitespace variants,
including compact [[.threshold]] and padded [[ .threshold ]] forms. Keep the
existing comparison and multiline cases unchanged, and ensure both
paramTokenRegexp and the final-comparison regexp whitespace behavior is
exercised.
In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.constants.tsx`:
- Around line 30-36: Extract the repeated muiTableHeadCellProps.sx configuration
into a shared HEAD_CELL_PROPS constant, then replace the duplicated inline
objects in all three column definitions with muiTableHeadCellProps:
HEAD_CELL_PROPS.
- Around line 19-23: Update the “Parameter” column definition to align its
accessor with the value rendered by its Cell callback: use paramName
consistently instead of summary, while preserving the existing displayed value
and column filtering, sorting, and search behavior.
In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.types.ts`:
- Around line 10-11: Update AlertThresholdsFormValues to allow string, number,
or undefined values, matching the runtime state of the numeric TextInput during
editing. In AlertThresholds.tsx, update the handling near the raw form value
check to remove the raw-as-unknown cast and compare the value directly against
the empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be36beaf-d5f7-46f4-8fb9-c7b3c69951b9
⛔ Files ignored due to path filters (3)
api/alerting/v1/alerting.pb.gois excluded by!**/*.pb.goapi/alerting/v1/alerting.pb.gw.gois excluded by!**/*.pb.gw.goapi/alerting/v1/alerting_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (42)
api/alerting/v1/alerting.pb.validate.goapi/alerting/v1/alerting.protodynamic-alert-thresholds-plan.mdmanaged/cmd/pmm-managed/main.gomanaged/data/alerting-templates/mysql_too_many_connections.ymlmanaged/data/alerting-templates/node_high_cpu_load.ymlmanaged/data/alerting-templates/postgresql_table_bloat_dual_threshold.ymlmanaged/models/alert_rule_helpers.gomanaged/models/alert_rule_model.gomanaged/models/alert_rule_model_reform.gomanaged/models/alert_rule_threshold_override_model.gomanaged/models/alert_rule_threshold_override_model_reform.gomanaged/models/database.gomanaged/models/template_helpers.gomanaged/models/template_model.gomanaged/pi/alert/overridable.gomanaged/pi/alert/overridable_test.gomanaged/pi/alert/parameter.gomanaged/pi/alert/template.gomanaged/services/alerting/rule_builder.gomanaged/services/alerting/rule_builder_dynamic_test.gomanaged/services/alerting/rule_builder_test.gomanaged/services/alerting/service.gomanaged/services/alerting/service_test.gomanaged/services/alerting/threshold_metrics.gomanaged/services/alerting/threshold_overrides.goui/apps/pmm-compat/src/compat.tsui/apps/pmm-compat/src/lib/events.tsui/apps/pmm/src/api/alerting.tsui/apps/pmm/src/components/alert-thresholds/AlertThresholds.constants.tsxui/apps/pmm/src/components/alert-thresholds/AlertThresholds.tsxui/apps/pmm/src/components/alert-thresholds/AlertThresholds.types.tsui/apps/pmm/src/components/alert-thresholds/index.tsui/apps/pmm/src/components/alert-thresholds/reset-value-cell/ResetValueCell.tsxui/apps/pmm/src/components/alert-thresholds/reset-value-cell/index.tsui/apps/pmm/src/components/main/MainWithNav.tsxui/apps/pmm/src/components/modal/Modal.tsxui/apps/pmm/src/contexts/grafana/grafana.provider.tsxui/apps/pmm/src/hooks/api/useNodeThresholds.tsui/apps/pmm/src/types/alerting.types.tsui/packages/shared/src/messenger.tsui/packages/shared/src/types.ts
Comment on lines
+145
to
+172
| func UpsertThresholdOverride(q *reform.Querier, ruleID, paramName, nodeID string, value float64) (*AlertRuleThresholdOverride, error) { | ||
| existing, err := q.SelectOneFrom(AlertRuleThresholdOverrideTable, | ||
| fmt.Sprintf("WHERE rule_id = %s AND param_name = %s AND node_id = %s", q.Placeholder(1), q.Placeholder(2), q.Placeholder(3)), | ||
| ruleID, paramName, nodeID) | ||
| switch { | ||
| case errors.Is(err, reform.ErrNoRows): | ||
| row := &AlertRuleThresholdOverride{ | ||
| ID: uuid.New().String(), | ||
| RuleID: ruleID, | ||
| ParamName: paramName, | ||
| NodeID: nodeID, | ||
| Value: value, | ||
| } | ||
| if err := q.Insert(row); err != nil { | ||
| return nil, fmt.Errorf("failed to create threshold override: %w", err) | ||
| } | ||
| return row, nil | ||
| case err != nil: | ||
| return nil, fmt.Errorf("failed to look up threshold override: %w", err) | ||
| default: | ||
| row := existing.(*AlertRuleThresholdOverride) //nolint:forcetypeassert | ||
| row.Value = value | ||
| if err := q.Update(row); err != nil { | ||
| return nil, fmt.Errorf("failed to update threshold override: %w", err) | ||
| } | ||
| return row, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify reform Querier query/exec surface and existing raw-SQL usage patterns in the repo.
fd -t f 'querier.go' --full-path -- "$(go env GOPATH 2>/dev/null)/pkg/mod" 2>/dev/null | head -5
# Find existing raw SQL patterns using the reform querier in this repository.
rg -nP --type=go -C3 '\bq\.(QueryRow|Query|Exec)\(' managed/models | head -60
# Look for any existing ON CONFLICT usage to match conventions.
rg -nP --type=go -C3 'ON CONFLICT' managed | head -40Repository: percona/pmm
Length of output: 3108
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- reform dependency and Querier API ---'
rg -n 'reform' go.mod go.sum managed/models | head -40
fd -t f 'querier.go' . "$(go env GOPATH 2>/dev/null)/pkg/mod" 2>/dev/null | head -20
printf '%s\n' '--- target helper and related model definitions ---'
cat -n managed/models/alert_rule_helpers.go | sed -n '1,210p'
rg -n -C4 'type AlertRuleThresholdOverride|AlertRuleThresholdOverrideTable|threshold_overrides|UNIQUE.*rule_id|rule_id.*param_name.*node_id' managed | head -160
printf '%s\n' '--- callers and transaction wrapper ---'
rg -n -C5 'UpsertThresholdOverride|SetNodeThreshold|InTransactionContext' managed | head -180Repository: percona/pmm
Length of output: 38604
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- reform v1.5.1 source ---'
curl -fsSL https://raw.githubusercontent.com/go-reform/reform/v1.5.1/reform/querier.go | sed -n '1,240p'
printf '%s\n' '--- reform v1.5.1 repository references ---'
curl -fsSL https://api.github.com/repos/go-reform/reform/git/trees/v1.5.1?recursive=1 |
jq -r '.tree[].path' | rg 'querier|db|tx|reform.go' | head -80
printf '%s\n' '--- complete override model ---'
cat -n managed/models/alert_rule_threshold_override_model.go
cat -n managed/models/alert_rule_threshold_override_model_reform.go | sed -n '1,125p'
printf '%s\n' '--- all threshold setter callers ---'
rg -n -C8 'UpsertThresholdOverride|SetNodeThreshold|threshold override|threshold_override' . --glob '*.go' | head -240Repository: percona/pmm
Length of output: 28009
🌐 Web query:
gopkg.in/reform.v1 v1.5.1 Querier QueryRow source github
💡 Result:
The source code for the QueryRow method in gopkg.in/reform.v1 (version 1.5.1) is located in the querier.go file within the go-reform/reform GitHub repository [1]. In version 1.5.1, the implementation of QueryRow on the Querier struct is as follows: // QueryRow executes a query that is expected to return at most one row. // QueryRow always returns a non-nil value. Errors are deferred until Row's Scan method is called. func (q *Querier) QueryRow(query string, args...interface{}) *sql.Row { q.logBefore(query, args) start:= time.Now row:= q.dbtxCtx.QueryRowContext(q.ctx, query, args...) q.logAfter(query, args, time.Since(start), nil) return row } This method executes a query intended to return at most one row, logs the execution, and returns a standard sql.Row object, deferring any errors until the Scan method is invoked [1]. The Querier also provides QueryRowContext, which acts as a compatibility wrapper to satisfy standard interfaces [1][2].
Citations:
- 1: https://github.com/go-reform/reform/blob/v1.5.1/querier.go
- 2: https://github.com/go-reform/reform/blob/v1.5.1/doc.go
Use an atomic upsert for threshold overrides.
Concurrent calls can both observe no row, causing one insert to fail on the UNIQUE (rule_id, param_name, node_id) constraint. Replace the read-then-write sequence with INSERT ... ON CONFLICT ... DO UPDATE and RETURNING. reform.Querier exposes QueryRow in v1.5.1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@managed/models/alert_rule_helpers.go` around lines 145 - 172, Replace the
read-then-write logic in UpsertThresholdOverride with one atomic INSERT ... ON
CONFLICT (rule_id, param_name, node_id) DO UPDATE statement using q.QueryRow and
RETURNING to populate AlertRuleThresholdOverride. Preserve the existing inputs,
update Value on conflicts, return the resulting row, and wrap query errors with
the threshold-override context.
Comment on lines
811
to
842
| // Persist the registry row before creating the Grafana rule so the collector | ||
| // can emit pmm_alert_threshold as soon as the rule exists. If the Grafana | ||
| // call fails we best-effort roll the row back (an orphan row only emits | ||
| // unused series, which is harmless). | ||
| if ruleID != "" { | ||
| err = s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { | ||
| _, txErr := models.CreateAlertRule(tx.Querier, &models.CreateAlertRuleParams{ | ||
| RuleID: ruleID, | ||
| TemplateName: req.TemplateName, | ||
| FolderUID: req.FolderUid, | ||
| RuleGroup: req.Group, | ||
| RuleTitle: req.Name, | ||
| DefaultParams: defaultParams, | ||
| }) | ||
| return txErr | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| err = s.grafanaClient.CreateAlertRule(ctx, req.FolderUid, req.Group, interval, &rule) | ||
| if err != nil { | ||
| if ruleID != "" { | ||
| if delErr := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { | ||
| return models.DeleteAlertRule(tx.Querier, ruleID) | ||
| }); delErr != nil { | ||
| s.l.Warnf("failed to roll back alert rule registry row %s: %v", ruleID, delErr) | ||
| } | ||
| } | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A failed rollback leaves a permanent phantom rule.
The code commits the registry row, then creates the Grafana rule. If CreateAlertRule fails, the rollback delete is best-effort and only logged. Two cases make the delete fail:
- The database rejects the delete.
ctxis already cancelled, which is likely when the Grafana call failed because of cancellation. The rollback reuses the samectx.
The comment states that an orphan row "only emits unused series". That is not accurate. ListNodeThresholds in managed/services/alerting/threshold_overrides.go iterates every row from models.FindAlertRules and returns a threshold entry for each DefaultParams key. An orphan row therefore appears in the PMM threshold UI as an editable threshold for a rule that does not exist in Grafana, and it persists until someone deletes the row manually.
Use a context that is not tied to the failed request for the rollback, and reconcile orphan rows on startup or in the collector.
🛠️ Minimal fix for the cancelled-context case
err = s.grafanaClient.CreateAlertRule(ctx, req.FolderUid, req.Group, interval, &rule)
if err != nil {
if ruleID != "" {
- if delErr := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error {
+ rollbackCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second)
+ defer cancel()
+ if delErr := s.db.InTransactionContext(rollbackCtx, nil, func(tx *reform.TX) error {
return models.DeleteAlertRule(tx.Querier, ruleID)
}); delErr != nil {
s.l.Warnf("failed to roll back alert rule registry row %s: %v", ruleID, delErr)
}
}
return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Persist the registry row before creating the Grafana rule so the collector | |
| // can emit pmm_alert_threshold as soon as the rule exists. If the Grafana | |
| // call fails we best-effort roll the row back (an orphan row only emits | |
| // unused series, which is harmless). | |
| if ruleID != "" { | |
| err = s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { | |
| _, txErr := models.CreateAlertRule(tx.Querier, &models.CreateAlertRuleParams{ | |
| RuleID: ruleID, | |
| TemplateName: req.TemplateName, | |
| FolderUID: req.FolderUid, | |
| RuleGroup: req.Group, | |
| RuleTitle: req.Name, | |
| DefaultParams: defaultParams, | |
| }) | |
| return txErr | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| err = s.grafanaClient.CreateAlertRule(ctx, req.FolderUid, req.Group, interval, &rule) | |
| if err != nil { | |
| if ruleID != "" { | |
| if delErr := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { | |
| return models.DeleteAlertRule(tx.Querier, ruleID) | |
| }); delErr != nil { | |
| s.l.Warnf("failed to roll back alert rule registry row %s: %v", ruleID, delErr) | |
| } | |
| } | |
| return nil, err | |
| } | |
| // Persist the registry row before creating the Grafana rule so the collector | |
| // can emit pmm_alert_threshold as soon as the rule exists. If the Grafana | |
| // call fails we best-effort roll the row back (an orphan row only emits | |
| // unused series, which is harmless). | |
| if ruleID != "" { | |
| err = s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { | |
| _, txErr := models.CreateAlertRule(tx.Querier, &models.CreateAlertRuleParams{ | |
| RuleID: ruleID, | |
| TemplateName: req.TemplateName, | |
| FolderUID: req.FolderUid, | |
| RuleGroup: req.Group, | |
| RuleTitle: req.Name, | |
| DefaultParams: defaultParams, | |
| }) | |
| return txErr | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| err = s.grafanaClient.CreateAlertRule(ctx, req.FolderUid, req.Group, interval, &rule) | |
| if err != nil { | |
| if ruleID != "" { | |
| rollbackCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) | |
| defer cancel() | |
| if delErr := s.db.InTransactionContext(rollbackCtx, nil, func(tx *reform.TX) error { | |
| return models.DeleteAlertRule(tx.Querier, ruleID) | |
| }); delErr != nil { | |
| s.l.Warnf("failed to roll back alert rule registry row %s: %v", ruleID, delErr) | |
| } | |
| } | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@managed/services/alerting/service.go` around lines 811 - 842, Prevent phantom
alert-rule registry rows when Grafana creation fails. In the CreateAlertRule
rollback path, execute DeleteAlertRule through a context independent of the
cancelled request context, and add startup or collector reconciliation to remove
registry rows whose Grafana rules no longer exist. Update the nearby comment to
accurately describe orphan-row behavior.
Comment on lines
+70
to
+95
| // Collect implements prom.Collector. Failures are logged and yield no metrics | ||
| // for the affected scrape rather than aborting the whole /metrics response. | ||
| func (c *AlertThresholdMetricsCollector) Collect(ch chan<- prom.Metric) { | ||
| err := c.db.InTransactionContext(context.Background(), nil, func(tx *reform.TX) error { | ||
| rules, err := models.FindAlertRules(tx.Querier) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(rules) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| nodes, err := models.FindNodes(tx.Querier, models.NodeFilters{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, rule := range rules { | ||
| if len(rule.DefaultParams) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| overrides, err := models.FindThresholdOverridesByRule(tx.Querier, rule.RuleID) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the database work in Collect, and remove the per-rule query.
Three points about this block:
context.Background()gives the transaction no deadline. Prometheus scrapes callCollectsynchronously. A slow or blocked database holds the transaction and stalls the whole/debug/metricsresponse with no upper bound. Derive a context with a timeout instead.models.FindThresholdOverridesByRuleruns once per rule inside the loop at line 92.models.FindAllThresholdOverridesalready exists and returns every row in one query. Group the result byRuleIDbefore the loop.- The doc comment states that failures "yield no metrics for the affected scrape". That is not accurate. The code sends metrics to
chinside the transaction, so an error at line 94 leaves the already-sent series in the scrape. The result is a partial series set, and rules whose threshold series is missing evaluate against no data. Collect the metrics into a slice inside the transaction and send them only after the transaction succeeds.
♻️ Proposed restructure
func (c *AlertThresholdMetricsCollector) Collect(ch chan<- prom.Metric) {
- err := c.db.InTransactionContext(context.Background(), nil, func(tx *reform.TX) error {
+ ctx, cancel := context.WithTimeout(context.Background(), collectTimeout)
+ defer cancel()
+
+ var metrics []prom.Metric
+ err := c.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error {
rules, err := models.FindAlertRules(tx.Querier)
if err != nil {
return err
}
if len(rules) == 0 {
return nil
}
nodes, err := models.FindNodes(tx.Querier, models.NodeFilters{})
if err != nil {
return err
}
+ allOverrides, err := models.FindAllThresholdOverrides(tx.Querier)
+ if err != nil {
+ return err
+ }
+
+ // rule_id -> param -> node_id -> override value
+ byRuleParamNode := make(map[string]map[string]map[string]float64, len(rules))
+ for _, o := range allOverrides {
+ params, ok := byRuleParamNode[o.RuleID]
+ if !ok {
+ params = make(map[string]map[string]float64)
+ byRuleParamNode[o.RuleID] = params
+ }
+ nodeValues, ok := params[o.ParamName]
+ if !ok {
+ nodeValues = make(map[string]float64)
+ params[o.ParamName] = nodeValues
+ }
+ nodeValues[o.NodeID] = o.Value
+ }
+
for _, rule := range rules {
if len(rule.DefaultParams) == 0 {
continue
}
-
- overrides, err := models.FindThresholdOverridesByRule(tx.Querier, rule.RuleID)
- if err != nil {
- return err
- }
-
- // param -> node_id -> override value
- byParamNode := make(map[string]map[string]float64, len(rule.DefaultParams))
- for _, o := range overrides {
- m, ok := byParamNode[o.ParamName]
- if !ok {
- m = make(map[string]float64)
- byParamNode[o.ParamName] = m
- }
- m[o.NodeID] = o.Value
- }
+ byParamNode := byRuleParamNode[rule.RuleID]Send metrics to ch after InTransactionContext returns without an error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@managed/services/alerting/threshold_metrics.go` around lines 70 - 95, Update
AlertThresholdMetricsCollector.Collect to derive a bounded-timeout context
instead of using context.Background, load all overrides once with
models.FindAllThresholdOverrides, group them by RuleID before processing rules,
and reuse the grouped values rather than querying per rule. Accumulate generated
metrics in a slice during the transaction, then send them to ch only after
InTransactionContext succeeds; preserve the documented no-metrics-on-failure
behavior and handle the transaction error accordingly.
Comment on lines
+108
to
+119
| for paramName, defaultValue := range rule.DefaultParams { | ||
| for _, node := range nodes { | ||
| value := defaultValue | ||
| if nodeOverrides, ok := byParamNode[paramName]; ok { | ||
| if ov, ok := nodeOverrides[node.NodeID]; ok { | ||
| value = ov | ||
| } | ||
| } | ||
|
|
||
| ch <- prom.MustNewConstMetric(c.desc, prom.GaugeValue, value, rule.RuleID, paramName, node.NodeName) | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Series count multiplies rules, parameters, and nodes.
The nested loops emit one series per (rule, param, node) on every scrape. The count is Σ(rules) × overridable params × nodes. With 100 rules, 2 overridable parameters each, and 500 nodes, the collector emits 100,000 series from a single endpoint on each scrape. Nothing in this code bounds the product.
Most of those series carry the rule default, because only overridden nodes differ. Consider emitting one default series per (rule, param) without the node label, plus one series per actual override, and let the rule expression fall back with group_left when no per-node series exists. That keeps the emitted set proportional to the number of real overrides.
If you keep the current shape, add a documented cap and a warning log when the product exceeds it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@managed/services/alerting/threshold_metrics.go` around lines 108 - 119,
Replace the per-node emission in the DefaultParams loop with one unlabeled
default metric per (rule, param) and additional node-labeled metrics only for
entries present in byParamNode overrides; update the metric descriptor and
rule-expression join as needed so node-specific values override the default via
group_left. If retaining the current shape instead, introduce a documented
product cap and warning when the series count exceeds it.
Comment on lines
+90
to
+125
| func (s *Service) SetNodeThreshold(ctx context.Context, req *alerting.SetNodeThresholdRequest) (*alerting.SetNodeThresholdResponse, error) { | ||
| var result *alerting.NodeThreshold | ||
|
|
||
| err := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { | ||
| if _, err := models.FindNodeByID(tx.Querier, req.NodeId); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| rule, err := models.FindAlertRuleByID(tx.Querier, req.RuleId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| defaultValue, ok := rule.DefaultParams[req.ParamName] | ||
| if !ok { | ||
| return status.Errorf(codes.InvalidArgument, "Parameter %q is not an overridable threshold of rule %q.", req.ParamName, req.RuleId) | ||
| } | ||
|
|
||
| if err := s.validateThresholdRange(rule, req.ParamName, req.Value); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if _, err := models.UpsertThresholdOverride(tx.Querier, req.RuleId, req.ParamName, req.NodeId, req.Value); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| result = s.buildNodeThreshold(rule, req.ParamName, defaultValue, req.Value, true) | ||
|
|
||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &alerting.SetNodeThresholdResponse{Threshold: result}, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the threshold-override table enforces uniqueness on (rule_id, param_name, node_id)
rg -n -i -C6 'threshold_override' --glob '*.sql' --glob '*.go'Repository: percona/pmm
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg -i 'alert|threshold|override|migration|schema' | head -200
printf '%s\n' '--- related identifiers ---'
rg -n -i -C5 'UpsertThresholdOverride|FindThresholdOverridesByNode|NodeThreshold|threshold.?override|threshold_override|rule_id.*param_name.*node_id' --glob '*.go' --glob '*.sql' --glob '*.yaml' --glob '*.yml' . | head -500Repository: percona/pmm
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- schema ---'
sed -n '1188,1220p' managed/models/database.go
printf '%s\n' '--- upsert and select helpers ---'
sed -n '114,172p' managed/models/alert_rule_helpers.go
printf '%s\n' '--- service transaction and list path ---'
sed -n '1,155p' managed/services/alerting/threshold_overrides.go
printf '%s\n' '--- transaction implementation references ---'
rg -n -C4 'func .*InTransactionContext|InTransactionContext\\(' managed --glob '*.go' | head -160
printf '%s\n' '--- concurrent/upsert tests ---'
rg -n -i -C4 'UpsertThresholdOverride|threshold override|threshold_override|unique|duplicate' managed --glob '*_test.go' | head -300Repository: percona/pmm
Length of output: 29813
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- transaction implementation ---'
rg -n -F -C8 'InTransactionContext' managed --glob '*.go' | head -240
printf '%s\n' '--- database schema initialization ---'
rg -n -F -C8 'DatabaseSchema|SchemaStatements|CREATE TABLE alert_rule_threshold_overrides|databaseSchema' managed/models managed --glob '*.go' | head -240
printf '%s\n' '--- focused helper and service call graph ---'
rg -n -F -C5 'UpsertThresholdOverride' managed --glob '*.go'
rg -n -F -C5 'SetNodeThreshold' managed/services/alerting --glob '*.go'
printf '%s\n' '--- read-only source invariant check ---'
python3 - <<'PY'
from pathlib import Path
schema = Path("managed/models/database.go").read_text()
helper = Path("managed/models/alert_rule_helpers.go").read_text()
service = Path("managed/services/alerting/threshold_overrides.go").read_text()
assert "UNIQUE (rule_id, param_name, node_id)" in schema
start = helper.index("func UpsertThresholdOverride")
end = helper.index("\n}", start) + 2
upsert = helper[start:end]
assert "q.SelectOneFrom" in upsert
assert "q.Insert(row)" in upsert
assert "failed to create threshold override" in upsert
assert "ON CONFLICT" not in upsert
assert "retry" not in upsert.lower()
assert "models.UpsertThresholdOverride" in service
print("schema_unique_constraint=present")
print("upsert_strategy=select_then_insert_or_update")
print("insert_conflict_retry=absent")
print("service_propagates_upsert_error=present")
PYRepository: percona/pmm
Length of output: 24353
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all transaction wrapper definitions ---'
rg -n -F 'func ' --glob '*.go' . | rg 'InTransactionContext|TransactionContext' | head -120
printf '%s\n' '--- all transaction wrapper references and retry logic ---'
rg -n -i -C6 'InTransactionContext|serialization failure|deadlock|retry.*transaction|transaction.*retry' --glob '*.go' . | head -300
printf '%s\n' '--- database wrapper files ---'
git ls-files '*.go' | rg -i 'database|db|storage|reform' | head -200Repository: percona/pmm
Length of output: 30159
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- alerting service database field and constructors ---'
sed -n '1,180p' managed/services/alerting/deps.go
sed -n '1,130p' managed/services/alerting/service.go
rg -n -F -C5 'InTransactionContext' managed/services/alerting managed/services --glob '*.go' | head -180
printf '%s\n' '--- database type/import declarations ---'
rg -n -i -C5 'type .*DB|reform\\.DB|postgresql\\.Dialect|InTransactionContext' managed --glob '*.go' | head -300
printf '%s\n' '--- module dependency candidates ---'
rg -n -i 'reform|database|db' go.mod managed/go.mod go.work 2>/dev/null | head -120Repository: percona/pmm
Length of output: 42431
🌐 Web query:
gopkg.in/reform.v1 reform.DB InTransactionContext implementation retry rollback
💡 Result:
The reform.DB.InTransactionContext method in the gopkg.in/reform.v1 library is designed to wrap a function execution within a database transaction [1][2]. It handles transaction lifecycle management (Begin, Commit, and Rollback) automatically but does not include built-in retry logic [1][2]. Implementation Details: - The method starts a transaction using BeginTx with the provided context and transaction options [1][2]. - It uses a deferred function to ensure that a Rollback is executed if the provided function f returns an error or panics [1][2]. - If the function f executes successfully (returns nil), the method calls tx.Commit [1][2]. - A flag (committed) is used to track whether the commit was successful; if so, the deferred rollback becomes a no-op [1][2]. Retry and Rollback: - Rollback: The rollback mechanism is fully implemented for any error or panic that occurs within the provided closure [1][2]. - Retry: There is no native support for retrying transactions within reform.DB.InTransactionContext [1][2]. If your application requires retries (e.g., to handle deadlocks or transient network failures), you must implement a wrapper around InTransactionContext that loops and executes the transaction logic multiple times until it succeeds or reaches a retry limit [3]. External libraries or manual implementations are required for this functionality [4][3]. Example of manual retry logic: for attempt:= 0; attempt < maxRetries; attempt++ { err:= db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { //... business logic... return nil }) if err == nil { return nil // Success } // Check if the error is retryable (e.g., deadlock) if!isRetryable(err) { return err } // Optional: add backoff/delay }
Citations:
- 1: https://github.com/go-reform/reform/blob/main/db.go
- 2: https://github.com/go-reform/reform/blob/v1.5.1/db.go
- 3: https://kindatechnical.com/golang/lesson-75-database-transactions.html
- 4: https://pkg.go.dev/github.com/Jille/trxwrap
Handle concurrent threshold upserts atomically.
alert_rule_threshold_overrides already has UNIQUE (rule_id, param_name, node_id), so duplicate rows cannot occur. However, concurrent first writes can race in UpsertThresholdOverride; one insert can return a unique-constraint error, which SetNodeThreshold propagates. Use INSERT ... ON CONFLICT DO UPDATE or retry in a new transaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@managed/services/alerting/threshold_overrides.go` around lines 90 - 125, The
SetNodeThreshold transaction must handle concurrent first-time writes without
propagating a unique-constraint error. Update models.UpsertThresholdOverride,
used within SetNodeThreshold, to use INSERT ... ON CONFLICT DO UPDATE for the
existing (rule_id, param_name, node_id) uniqueness constraint, or retry the
operation in a fresh transaction while preserving the current validation and
result-building flow.
Comment on lines
+27
to
+30
|
|
||
| export class OpenAlertThresholdsModalEvent extends BusEventBase { | ||
| static type = 'open-alert-thresholds-modal-event'; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm where this event is published within this repo (the actual publisher may live in the
# separate Percona Grafana repository referenced by the PR, and is not visible here).
rg -n -C5 'OpenAlertThresholdsModalEvent' --type=tsRepository: percona/pmm
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- events.ts ---'
cat -n ui/apps/pmm-compat/src/lib/events.ts
printf '%s\n' '--- compat.ts around the subscriber ---'
sed -n '180,220p' ui/apps/pmm-compat/src/compat.ts
printf '%s\n' '--- shared types around the message ---'
sed -n '55,85p' ui/packages/shared/src/types.ts
printf '%s\n' '--- local event definitions and usages ---'
rg -n -C3 'BusEvent(Base|WithPayload)|OpenAlertThresholdsModal|OPEN_ALERT_THRESHOLDS_MODAL' ui --glob '*.{ts,tsx}'Repository: percona/pmm
Length of output: 8514
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
events = Path("ui/apps/pmm-compat/src/lib/events.ts").read_text()
compat = Path("ui/apps/pmm-compat/src/compat.ts").read_text()
types = Path("ui/packages/shared/src/types.ts").read_text()
assert re.search(
r"class\s+OpenAlertThresholdsModalEvent\s+extends\s+BusEventBase\b",
events,
)
assert "payload: e.payload" in compat
assert re.search(
r"Message<\s*'OPEN_ALERT_THRESHOLDS_MODAL',\s*\{\s*nodeId:\s*string;\s*nodeName:\s*string\s*\}\s*>",
types,
)
print("event base: BusEventBase")
print("forwarded property: e.payload")
print("required payload fields: nodeId, nodeName")
PY
printf '%s\n' '--- Grafana dependency references ---'
rg -n -C2 '"`@grafana/data`"|BusEventWithPayload|BusEventBase' \
package.json package-lock.json yarn.lock pnpm-lock.yaml ui 2>/dev/null || trueRepository: percona/pmm
Length of output: 3359
🌐 Web query:
Grafana @grafana/data BusEventBase BusEventWithPayload payload API
💡 Result:
In Grafana plugin development, BusEventBase and BusEventWithPayload are base classes located in the @grafana/data package used to define events for the Grafana EventBus [1][2][3]. BusEventBase is the foundational abstract class for all events [1]. It provides the essential structure, including a mandatory static type property used to identify the event, and optional metadata such as tags for advanced filtering [1]. BusEventWithPayload is an abstract class that extends BusEventBase, designed for events that need to carry data [1]. By using a generic type parameter (T), it defines a strongly typed payload property that is required to be provided during the event's instantiation [1][2]. To implement a custom event, you typically extend one of these classes and define a static type string [2]: class MyCustomEvent extends BusEventWithPayload { static type = 'my-custom-event'; } When using the EventBus (often accessed via panel props), you can subscribe to these events by passing the event class to the getStream method [2]: eventBus.getStream(MyCustomEvent).subscribe((event) => { console.log(event.payload); }); The payload property allows for structured, type-safe communication between different components of a Grafana plugin or across the wider Grafana application [2][4]. Existing events like RefreshEvent extend BusEventBase (for events without payload) [5], while events like DataHoverEvent extend BusEventWithPayload (to carry data such as panel coordinates or data frames) [3][6].
Citations:
- 1: https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/events/types.ts
- 2: https://grafana.com/developers/plugin-tools/how-to-guides/panel-plugins/subscribe-events
- 3: https://github.com/grafana/grafana/blob/01dd623daa804d2f09a71a075dd4df398361cce7/packages/grafana-data/src/events/common.ts
- 4: https://grafana.com/docs/plugins/marcusolsson-dynamictext-panel/latest/exploring-eventbus/
- 5: https://github.com/grafana/grafana/blob/main/packages/grafana-runtime/src/services/appEvents.ts
- 6: https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/events/common.ts
Make OpenAlertThresholdsModalEvent extend BusEventWithPayload<{ nodeId: string; nodeName: string }> and import it from @grafana/data.
compat.ts forwards e.payload, but BusEventBase does not provide a payload. The OPEN_ALERT_THRESHOLDS_MODAL message therefore lacks the node identity required by the PMM UI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/apps/pmm-compat/src/lib/events.ts` around lines 27 - 30, Update
OpenAlertThresholdsModalEvent to extend BusEventWithPayload with the payload
shape { nodeId: string; nodeName: string }, importing BusEventWithPayload from
`@grafana/data`. Preserve the existing event type value so compat.ts can forward
e.payload containing the required node identity.
Comment on lines
+57
to
+68
| useEffect(() => { | ||
| const handler = messenger.addListener({ | ||
| type: 'OPEN_ALERT_THRESHOLDS_MODAL', | ||
| onMessage: (msg: OpenAlertThresholdsModalMessage) => { | ||
| setNodeId(msg.payload?.nodeId); | ||
| setNodeName(msg.payload?.nodeName); | ||
| setIsOpen(true); | ||
| }, | ||
| }); | ||
|
|
||
| return () => messenger.removeListener(handler); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect messenger listener registration semantics and other listener call sites.
set -euo pipefail
fd -t f 'messenger' ui -E node_modules
fd -t f 'messenger*' ui -E node_modules --exec sh -c 'ast-grep outline "$1" --items all' sh {}
rg -n -C4 'addListener\(|removeListener\(' ui --glob '!**/node_modules/**'Repository: percona/pmm
Length of output: 14806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- shared messenger outline ---'
ast-grep outline ui/packages/shared/src/messenger.ts --items all
printf '%s\n' '--- shared messenger implementation ---'
cat -n ui/packages/shared/src/messenger.ts
printf '%s\n' '--- component context ---'
cat -n ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.tsx
printf '%s\n' '--- messenger singleton ---'
cat -n ui/apps/pmm/src/lib/messenger.ts
printf '%s\n' '--- relevant tests ---'
cat -n ui/packages/shared/src/messenger.test.tsRepository: percona/pmm
Length of output: 16412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all references to the component and messenger singleton ---'
rg -n -C3 'AlertThresholds|from .*\blib/messenger|from .*\bmessenger' ui/apps/pmm/src --glob '!**/node_modules/**'
printf '%s\n' '--- effect dependency conventions in nearby components ---'
rg -n -C2 'useEffect\(\(\) => \{|messenger\.addListener' ui/apps/pmm/src/components ui/apps/pmm/src/contexts --glob '*.{ts,tsx}'
printf '%s\n' '--- repository lint configuration ---'
rg -n -C3 'react-hooks/exhaustive-deps|eslint-plugin-react-hooks' . --glob '!**/node_modules/**' --glob '!**/dist/**'Repository: percona/pmm
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- messenger registration and component lifetime ---'
rg -n -C5 'messenger\.(register|unregister)|<AlertThresholds|MainWithNav' ui/apps/pmm/src --glob '*.{ts,tsx}'
printf '%s\n' '--- exact dependency-bearing messenger effects ---'
sed -n '70,170p' ui/apps/pmm/src/contexts/grafana/grafana.provider.tsx
printf '%s\n' '--- lint rule configuration in the PMM app ---'
fd -H -t f 'eslint*' ui/apps/pmm
fd -H -t f 'package.json' ui/apps/pmm --exec sh -c 'printf "%s\n" "--- $1"; rg -n -C3 "lint|react-hooks" "$1"' sh {}Repository: percona/pmm
Length of output: 14105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
'use strict';
// Read-only model of CrossFrameMessenger.addListener/removeListener and the
// effect lifecycle shown in AlertThresholds.tsx.
const listeners = [];
const addListener = (listener) => {
listeners.push(listener);
return listener;
};
const removeListener = (listener) => {
const remaining = listeners.filter((candidate) => candidate !== listener);
listeners.length = 0;
listeners.push(...remaining);
};
const dispatch = (message) => {
for (const listener of [...listeners]) {
if (listener.type === message.type) listener.onMessage(message);
}
};
let opened = 0;
const createEffect = () => {
const handler = addListener({
type: 'OPEN_ALERT_THRESHOLDS_MODAL',
onMessage: () => opened += 1,
});
return () => removeListener(handler);
};
// Two renders without a dependency array: cleanup from render 1 runs before
// registration from render 2. A dispatch in that gap has no matching listener.
const cleanup1 = createEffect();
cleanup1();
dispatch({ type: 'OPEN_ALERT_THRESHOLDS_MODAL' });
const cleanup2 = createEffect();
console.log(JSON.stringify({
listenersAfterRerender: listeners.length,
messagesHandledDuringGap: opened,
messageHandledAfterRerender: (() => {
dispatch({ type: 'OPEN_ALERT_THRESHOLDS_MODAL' });
return opened;
})(),
}));
JSRepository: percona/pmm
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
'use strict';
const listeners = [];
const addListener = (listener) => {
listeners.push(listener);
return listener;
};
const removeListener = (listener) => {
const remaining = listeners.filter((candidate) => candidate !== listener);
listeners.length = 0;
listeners.push(...remaining);
};
const dispatch = (message) => {
for (const listener of [...listeners]) {
if (listener.type === message.type) listener.onMessage(message);
}
};
let opened = 0;
const createEffect = () => {
const handler = addListener({
type: 'OPEN_ALERT_THRESHOLDS_MODAL',
onMessage: () => opened += 1,
});
return () => removeListener(handler);
};
const cleanup1 = createEffect();
cleanup1();
dispatch({ type: 'OPEN_ALERT_THRESHOLDS_MODAL' });
const cleanup2 = createEffect();
console.log(JSON.stringify({
listenersAfterRerender: listeners.length,
messagesHandledDuringGap: opened,
messageHandledAfterRerender: (() => {
dispatch({ type: 'OPEN_ALERT_THRESHOLDS_MODAL' });
return opened;
})(),
}));
JSRepository: percona/pmm
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- GrafanaProvider definition and provider hierarchy ---'
ast-grep outline ui/apps/pmm/src/contexts/grafana/grafana.provider.tsx --items all
cat -n ui/apps/pmm/src/contexts/grafana/grafana.provider.tsx | sed -n '1,90p'
rg -n -C8 'GrafanaProvider|GrafanaPage|Providers' ui/apps/pmm/src/Providers.tsx ui/apps/pmm/src --glob '*.{ts,tsx}' --glob '!**/*.test.*'
printf '%s\n' '--- all unregister implementations and callers ---'
rg -n -C5 'unregister\(\)|this\.listeners\s*=' ui/packages/shared/src ui/apps/pmm/src --glob '*.{ts,tsx}'Repository: percona/pmm
Length of output: 38740
Fix shared messenger cleanup before adding [].
CrossFrameMessenger.unregister() clears the shared listener list. GrafanaProvider calls it while AlertThresholds can remain mounted, so [] can leave OPEN_ALERT_THRESHOLDS_MODAL unregistered. Scope cleanup to the listeners owned by GrafanaProvider, then register this listener once. The current effect also re-registers after every render and creates unnecessary listener gaps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.tsx` around lines
57 - 68, Update GrafanaProvider cleanup so it removes only listeners owned by
that provider instead of clearing the shared CrossFrameMessenger listener list
via unregister(). Then update the AlertThresholds listener effect to register
OPEN_ALERT_THRESHOLDS_MODAL once with an empty dependency array and retain its
own handler cleanup, avoiding render-time re-registration and listener gaps.
Comment on lines
+76
to
+113
| const handleSubmit = async (values: AlertThresholdsFormValues) => { | ||
| const operations: Promise<unknown>[] = []; | ||
|
|
||
| for (const row of rows) { | ||
| const raw = values[row.id]; | ||
| const parsed = | ||
| raw === undefined || (raw as unknown) === '' ? undefined : Number(raw); | ||
| const cleared = parsed === undefined || Number.isNaN(parsed); | ||
|
|
||
| // Clearing the field or setting it to the default reverts the node to the | ||
| // template default (delete the override); only needed if one exists. | ||
| if (cleared || parsed === row.defaultValue) { | ||
| if (row.isOverridden) { | ||
| operations.push( | ||
| deleteThreshold({ ruleId: row.ruleId, paramName: row.paramName }) | ||
| ); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (parsed !== row.effectiveValue) { | ||
| operations.push( | ||
| setThreshold({ | ||
| ruleId: row.ruleId, | ||
| paramName: row.paramName, | ||
| value: parsed, | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (operations.length > 0) { | ||
| await Promise.all(operations); | ||
| enqueueSnackbar('Alert thresholds updated', { variant: 'success' }); | ||
| } | ||
|
|
||
| handleClose(); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle mutation failures in the submit handler.
Promise.all rejects if any threshold request fails. The handler does not catch the rejection, and react-hook-form re-throws it from handleSubmit. The result is an unhandled promise rejection, no error message for the user, and no success snackbar even though earlier requests may have applied. Report the failure and keep the modal open so the user can retry.
🐛 Proposed fix
if (operations.length > 0) {
- await Promise.all(operations);
- enqueueSnackbar('Alert thresholds updated', { variant: 'success' });
+ const results = await Promise.allSettled(operations);
+ const failed = results.filter((r) => r.status === 'rejected').length;
+
+ if (failed > 0) {
+ enqueueSnackbar(
+ `Failed to update ${failed} of ${operations.length} alert thresholds`,
+ { variant: 'error' }
+ );
+ return;
+ }
+
+ enqueueSnackbar('Alert thresholds updated', { variant: 'success' });
}
handleClose();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleSubmit = async (values: AlertThresholdsFormValues) => { | |
| const operations: Promise<unknown>[] = []; | |
| for (const row of rows) { | |
| const raw = values[row.id]; | |
| const parsed = | |
| raw === undefined || (raw as unknown) === '' ? undefined : Number(raw); | |
| const cleared = parsed === undefined || Number.isNaN(parsed); | |
| // Clearing the field or setting it to the default reverts the node to the | |
| // template default (delete the override); only needed if one exists. | |
| if (cleared || parsed === row.defaultValue) { | |
| if (row.isOverridden) { | |
| operations.push( | |
| deleteThreshold({ ruleId: row.ruleId, paramName: row.paramName }) | |
| ); | |
| } | |
| continue; | |
| } | |
| if (parsed !== row.effectiveValue) { | |
| operations.push( | |
| setThreshold({ | |
| ruleId: row.ruleId, | |
| paramName: row.paramName, | |
| value: parsed, | |
| }) | |
| ); | |
| } | |
| } | |
| if (operations.length > 0) { | |
| await Promise.all(operations); | |
| enqueueSnackbar('Alert thresholds updated', { variant: 'success' }); | |
| } | |
| handleClose(); | |
| }; | |
| const handleSubmit = async (values: AlertThresholdsFormValues) => { | |
| const operations: Promise<unknown>[] = []; | |
| for (const row of rows) { | |
| const raw = values[row.id]; | |
| const parsed = | |
| raw === undefined || (raw as unknown) === '' ? undefined : Number(raw); | |
| const cleared = parsed === undefined || Number.isNaN(parsed); | |
| // Clearing the field or setting it to the default reverts the node to the | |
| // template default (delete the override); only needed if one exists. | |
| if (cleared || parsed === row.defaultValue) { | |
| if (row.isOverridden) { | |
| operations.push( | |
| deleteThreshold({ ruleId: row.ruleId, paramName: row.paramName }) | |
| ); | |
| } | |
| continue; | |
| } | |
| if (parsed !== row.effectiveValue) { | |
| operations.push( | |
| setThreshold({ | |
| ruleId: row.ruleId, | |
| paramName: row.paramName, | |
| value: parsed, | |
| }) | |
| ); | |
| } | |
| } | |
| if (operations.length > 0) { | |
| const results = await Promise.allSettled(operations); | |
| const failed = results.filter((r) => r.status === 'rejected').length; | |
| if (failed > 0) { | |
| enqueueSnackbar( | |
| `Failed to update ${failed} of ${operations.length} alert thresholds`, | |
| { variant: 'error' } | |
| ); | |
| return; | |
| } | |
| enqueueSnackbar('Alert thresholds updated', { variant: 'success' }); | |
| } | |
| handleClose(); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/apps/pmm/src/components/alert-thresholds/AlertThresholds.tsx` around lines
76 - 113, Update handleSubmit to catch failures from Promise.all(operations),
report the mutation error through the existing user-facing error notification
mechanism, and return without calling handleClose so the modal remains open for
retry. Keep the success snackbar and close behavior only on successful
completion.
Comment on lines
+17
to
+21
| return ( | ||
| <IconButton onClick={() => setValue(row.id, row.defaultValue)}> | ||
| <RestartAltIcon /> | ||
| </IconButton> | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Give the icon-only button an accessible name.
The IconButton contains only an icon. Screen readers announce no name for it. Add an aria-label, and a title for a hover hint.
♿ Proposed fix
- <IconButton onClick={() => setValue(row.id, row.defaultValue)}>
+ <IconButton
+ aria-label={`Reset ${row.paramName} to default`}
+ title="Reset to default"
+ onClick={() => setValue(row.id, row.defaultValue)}
+ >
<RestartAltIcon />
</IconButton>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <IconButton onClick={() => setValue(row.id, row.defaultValue)}> | |
| <RestartAltIcon /> | |
| </IconButton> | |
| ); | |
| return ( | |
| <IconButton | |
| aria-label={`Reset ${row.paramName} to default`} | |
| title="Reset to default" | |
| onClick={() => setValue(row.id, row.defaultValue)} | |
| > | |
| <RestartAltIcon /> | |
| </IconButton> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ui/apps/pmm/src/components/alert-thresholds/reset-value-cell/ResetValueCell.tsx`
around lines 17 - 21, Update the IconButton in ResetValueCell to include an
accessible aria-label and a matching title describing that it resets the value,
while preserving the existing onClick behavior and icon.
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 33 minutes. |
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.
matejkubinec
force-pushed
the
PMM-14912-dynamic-thresholds
branch
from
September 2, 2026 14:22
8e4a3c7 to
ca1ece4
Compare
Contributor
Author
|
Superseded by a fresh PR from the same branch. This one reviewed the POC; the branch has since been reimplemented on top of current main with a scope-based API, so the review history here no longer matches the code. |
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
2 participants
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
Summary by CodeRabbit
New Features
Documentation