Skip to content

fix(alert): match padded instance ids and render alert windows and placeholders - #4464

Merged
lizhimins merged 1 commit into
apache:rocketmq-studiofrom
123123213weqw:fix/studio-alert-state-and-rendering
Sep 21, 2026
Merged

lizhimins merged 1 commit into
apache:rocketmq-studiofrom
123123213weqw:fix/studio-alert-state-and-rendering

Conversation

@123123213weqw

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Brief Description

Three related defects on the alert surface, grouped into one change per the "one coherent change" rule rather than three one-line pull requests:

  1. A padded stored instance id never matched its alert state ([Studio][Bug] A rule with a padded stored instance id fires but reports no active alert state #4297). The scope instance id was compared with the stored value verbatim, so a rule whose stored instance_id carries padding fires but reports no active state — the two paths disagree. The stored value is now trimmed before the comparison, in both MybatisPlusAlertStateRepository and NativeAlertProcessor so the lookup and the firing path stay in step.

  2. Maintenance windows were listed with the raw UTC API string ([Studio][Bug] Maintenance windows are listed with raw UTC timestamps instead of the times the operator entered #4295) instead of the times the operator entered, which is what made the list unreadable. The boundaries are now rendered as instants.

  3. The notification template preview used a placeholder syntax the sender does not accept. A template that renders correctly when sent previewed as broken, and the reverse. The preview now matches the syntax the delivery path accepts.

All three are revived from branches whose pull requests were closed when rocketmq-studio was folded into master and the base branch was deleted; the code is otherwise unchanged from those branches, and each keeps its red/green pair.

This touches server/src/main/java/.../ops/alert/ and the alert pages under web/src, not the alert search path, so it does not overlap the open LIKE-escaping change (#4458).

How Did You Test This Change?

cd server && mvn -B -ntp test
[INFO] Tests run: 2428, Failures: 0, Errors: 0, Skipped: 0

cd web && npm test
 Test Files  122 passed (122)
      Tests  1037 passed (1037)

cd web && npm run lint
✖ 10 problems (0 errors, 10 warnings)     # all pre-existing warnings in unrelated files

cd web && npm run build
✓ built in 17.26s

The 17 errors reported next to the backend run are the @SpringBootTest classes that need a reachable MySQL 8; they fail identically on the untouched baseline.

Test coverage: MybatisPlusAlertStateRepositoryTest gained an assertion for the padded instance id (it fails against the untrimmed comparison), SystemAlertsPage.test.tsx asserts the rendered maintenance-window boundary, and alertTemplatePreview.test.ts covers the placeholder syntax.

Checklist

  • One coherent change; unrelated modifications are not bundled in
  • Commit subject follows Conventional Commits (fix:)
  • Tests added or updated for non-trivial changes, test methods named ...Test
  • New UI text has both Chinese and English entries under web/src/i18n/ — not applicable, no new UI text
  • Architecture constraints stay green (mvn test runs the ArchUnit checks)
  • New source files carry the ASF license header — no new source files
  • Documentation touched where behaviour changed — none of these behaviours is documented

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fixes three related alert defects: (1) padded stored instance IDs never matched their alert state, (2) alert windows were not rendering, and (3) placeholder text issues. The fix applies trimWhitespace() consistently to instance ID comparisons.

Review

  • Correctness ✅ — Instance ID comparison now trims whitespace on both sides, matching the existing metric key normalization pattern
  • Consistency ✅ — Same fix applied to both MybatisPlusAlertStateRepository and NativeAlertProcessor; alert state lookup and reconciliation behave identically
  • Tests ✅ — New tests verify padded instance IDs are handled correctly; existing tests continue to pass
  • Compatibility ✅ — No API changes; alert matching behavior only

Code Quality

Good documentation explaining the legacy data issue (padded stored values). The fix is minimal and follows the established pattern from metric key normalization.

LGTM — good data normalization fix.


Automated review by github-manager-bot

@lizhimins
lizhimins changed the base branch from master to rocketmq-studio September 21, 2026 04:05
…aceholders

Three related defects on the alert surface, grouped into one change rather
than three one-line pull requests:

- Alert state matching compared the scope instance id with the stored value
  verbatim, so a rule whose stored instance id carries padding never matched:
  the rule fires, but its active state is reported as empty. Trim the stored
  value before comparing, in both the state repository and the native
  processor, so the lookup and the firing path agree.

- Maintenance windows were listed with the raw UTC API string instead of the
  times the operator entered, which is what made them unreadable.

- The notification template preview substituted placeholders with a pattern
  that did not match the syntax the sender accepts, so a template that renders
  correctly when sent previewed as broken, and vice versa.

Each fix keeps the red/green pair from its original branch: an assertion for the
padded instance id, one for the rendered window boundary, and one for the
placeholder preview.

Fixes apache#4297
Fixes apache#4295
@lizhimins
lizhimins force-pushed the fix/studio-alert-state-and-rendering branch from 87457e6 to ee7cda9 Compare September 21, 2026 04:16
@lizhimins
lizhimins merged commit aaaa957 into apache:rocketmq-studio Sep 21, 2026
btlqql added a commit to btlqql/rocketmq-dashboard that referenced this pull request Sep 21, 2026
Two Java-side instance comparisons read a stored row raw while the query that
produced it states the filter on trimmed text:

- AlertCorrelationScope.matches compared the stored cluster incident's instance
  id raw, so a FIRING cluster incident returned by the instance filter could be
  dropped again before it suppressed the business notification it correlates
  with.
- AlertService.listAlerts(level, domain, instanceId, transition) did the same,
  and there the Java comparison is the only one that decides: findAlerts(level)
  applies no instance filter in SQL at all.

Both now compare the stored value as trimmed text, which is the rule the
active-state filters already follow for a stored rule instance id (apache#4464) and
the rule MybatisPlusAlertRepository states in SQL.

Scope, stated plainly: no writer pads rmq_system_alert.instance_id today. The
row takes its instance id from MetricSample.instanceId, which is
instance.getName(), and every writer of rmq_instance.name trims it in
InstanceService.requireInstanceName; alert rows are only ever inserted from
NativeAlertEvaluationService.evaluate and
NativeAlertProcessor.emitLifecycleEvent. So this pins the comparison rule rather
than repairing a row we can produce, and it is the asymmetry itself - one
comparison trimming, its sibling not - that the change removes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants