fix(alert): consolidate rule authoring correctness - #2965
Merged
lizhimins merged 3 commits intoSep 2, 2026
Merged
Conversation
This was referenced Sep 2, 2026
RockteMQ-AI
approved these changes
Sep 2, 2026
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Fixes alert rule authoring correctness: switches changes() to delta() for the client connection drop rule (the old expression could never trigger since changes() counts transitions, not signed deltas), normalizes legacy Chinese display labels to machine-readable metric names before backend submission, and supports composite Prometheus durations like 1h30m. Good test coverage across all three fixes.
LGTM 👍
Automated review by github-manager-bot
Use a signed gauge delta for the bundled producer connection-drop rule so a drop of more than five producers can satisfy the PromQL expression. Keep the generated YAML and frontend mock aligned with the generator and lock the expression in backend and frontend regression tests. Constraint: changes() reports transition count and is non-negative, so changes(...) < -5 can never fire. Rejected: keep changes() and invert threshold | PromQL changes() cannot represent signed drops. Directive: keep server/scripts/gen_alert_rule_yaml.py, generated alert YAML, and web mock expression synchronized for bundled rules. Confidence: high Scope-risk: narrow Tested: JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/openjdk-21.0.2/Contents/Home mvn -Dtest=AlertRuleAssetServiceTest,AlertRuleRequestDTOTest,AlertServiceTest test; npm test -- alertRuleAssetService.test.ts alerts.test.ts AlertsPage.test.tsx; npm run build; python3 -m py_compile server/scripts/gen_alert_rule_yaml.py; git diff --check Signed-off-by: liuhy <liuhongyu@apache.org>
Normalize legacy display metric and duration values before submitting alert rules so old saved rules and copied rules keep the backend API contract. Keep threshold unit derivation keyed by canonical metric names and cover legacy conversion in the page tests. Constraint: existing stored rules may still contain older display labels, but new submissions should use canonical metric and Prometheus duration values. Rejected: only changing select option values | edit and duplicate flows can still replay legacy stored labels without explicit normalization. Confidence: high Scope-risk: narrow Tested: JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/openjdk-21.0.2/Contents/Home mvn -Dtest=AlertRuleAssetServiceTest,AlertRuleRequestDTOTest,AlertServiceTest test; npm test -- alertRuleAssetService.test.ts alerts.test.ts AlertsPage.test.tsx; npm run build; git diff --check Signed-off-by: liuhy <liuhongyu@apache.org>
Share the alert rule request Prometheus duration grammar with export validation so composite durations such as 1h30m round-trip into generated YAML instead of falling back to 5m. Constraint: keep existing fallback behavior for invalid or missing durations. Rejected: duplicating a wider regex in AlertService | would let DTO validation and export validation drift again. Confidence: high Scope-risk: narrow Tested: JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/openjdk-21.0.2/Contents/Home mvn -Dtest=AlertRuleAssetServiceTest,AlertRuleRequestDTOTest,AlertServiceTest test; JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/openjdk-21.0.2/Contents/Home mvn -DskipTests package; git diff --check Signed-off-by: liuhy <liuhongyu@apache.org>
lizhimins
force-pushed
the
consolidate/studio-alert-rule-correctness
branch
from
September 2, 2026 07:08
1d528e5 to
83ac164
Compare
RockteMQ-AI
approved these changes
Sep 2, 2026
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
New commits address alert rule correctness and duration validation. Changes look good.
Review of New Changes
Correctness ✅
changes()→delta()fix is correct:changes()counts value transitions and cannot produce negative values, making the original alert rule non-triggerable.delta()computes the actual signed difference.- Duration regex update to accept composite Prometheus durations (e.g.,
1h30m) aligns with Prometheus spec. - Extracting
PROMETHEUS_DURATION_REGEXPto a shared constant avoids divergence between DTO validation and service logic.
Tests ✅
- Good coverage: tests verify both the bundled YAML rule and the generator script use the correct expression.
- Composite duration validation test ensures
1h30mis accepted.
Compatibility ✅
- No breaking changes to public APIs.
LGTM.
Automated review by github-manager-bot
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
3 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.
Summary
Consolidates the alert rule authoring correctness fixes from the existing focused PRs:
This keeps the fixes together because they all affect alert rule authoring/export correctness:
delta()expression for the bundled producer connection-drop rule in the generator, generated YAML, and frontend mock data1h30mround-trip correctlyCloses #2614
Closes #2603
Closes #2604
Verification
JAVA_HOME=$(/usr/libexec/java_home -v 21) mvn -Dtest=AlertRuleAssetServiceTest,AlertRuleRequestDTOTest,AlertServiceTest test-> 87 tests passednpm test -- alertRuleAssetService.test.ts alerts.test.ts AlertsPage.test.tsx-> 4 files / 43 tests passednpm run build-> passed (tsc -b && vite build)JAVA_HOME=$(/usr/libexec/java_home -v 21) mvn -DskipTests package-> passedpython3 -m py_compile server/scripts/gen_alert_rule_yaml.py-> passedgit diff --check-> passedweb/-> 0 errors / 0 warningsKnown baseline note
JAVA_HOME=$(/usr/libexec/java_home -v 21) mvn -Dtest='org.apache.rocketmq.studio.ops.alert.*Test' testcurrently fails on the latest cleanorigin/rocketmq-studioas well:AlertSchemaMigrationTesttries to alterrmq_instance_message.result_snapshotwhen the table is absent. This PR does not modify that migration path; the targeted rule-authoring tests pass.