PMM-14912 Dynamic thresholds - #5579
Conversation
cb7ad52 to
eab2635
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5579 +/- ##
==========================================
+ Coverage 43.59% 45.01% +1.42%
==========================================
Files 415 426 +11
Lines 43134 43885 +751
==========================================
+ Hits 18804 19757 +953
+ Misses 22454 22176 -278
- Partials 1876 1952 +76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This reverts commit b56bd03.
|
@CodeRabbit full review |
|
@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
| 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.
| // 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.
| // 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.
| // 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.
| 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.
| 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.
|
|
||
| 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.
| 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.
| 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.
| 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.
| 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.
| 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. |
2 participants
PMM-14912
Grafana: percona/grafana#912
FB: Percona-Lab/pmm-submodules#4449
Summary by CodeRabbit
New Features
Documentation