btlqql opened a new pull request, #4710:
URL: https://github.com/apache/rocketmq-dashboard/pull/4710
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. -->
### Related issue
- No open issue covers this filter; the closest is #4297 (rules whose stored
`instanceId` carries surrounding whitespace), whose fix #4372 covers the
active-state filters in `MybatisPlusAlertStateRepository` and
`NativeAlertProcessor`.
- This PR covers the cross-domain notification-suppression correlation,
which compares the stored cluster incident rather than the stored rule, so the
two changes are independent.
### Brief Description
`AlertCorrelationScope.matches` (line 32) trimmed the business event's
instance id but compared the stored cluster incident's instance id raw, so a
FIRING cluster incident and the business notification it is meant to suppress
were not correlated and the operator got a second page for the same failure.
The filter runs in two steps with different string semantics:
`AlertNotificationSuppressionService` looks the incidents up through
`MybatisPlusAlertRepository.findAlertsPage`, which trims the *query* value and
compares in SQL (`MybatisPlusAlertRepository.java:198`), where MySQL's default
collation ignores trailing spaces. A row written as `' local '` by an earlier
build is therefore returned by that query, while the Java-side filter here
compares `'local'` with `' local '` and drops it again. The stored value is now
trimmed the same way the active-state filters do it
(`StringUtils.trimWhitespace`, which also tolerates a null label-free row), so
an incident the database considers in scope stays in scope.
### How Did You Test This Change?
Test written first and run against the unmodified tree (`red`, base commit
`7ce9a682`):
```
$ cd server && mvn -B -ntp test
-Dtest=AlertNotificationSuppressionServiceTest
[ERROR] Tests run: 8, Failures: 1, Errors: 0, Skipped: 0
[ERROR]
AlertNotificationSuppressionServiceTest.suppressesWhileTheClusterIncidentKeepsAPaddedStoredInstanceIdTest:163
Expecting Optional to contain:
SystemAlertVO(id=5, ..., transition=FIRING, instanceId= local ,
labels={brokerName=broker-1})
but was empty.
[INFO] BUILD FAILURE
```
After the fix (`green`), together with the alert correlation/state
neighbours:
```
$ cd server && mvn -B -ntp test
-Dtest=AlertNotificationSuppressionServiceTest,AlertFingerprintTest,AlertStateMachineTest,NativeAlertRuleScopeMatcherTest
[INFO] You have 0 Checkstyle violations.
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
```
Test coverage:
- `suppressesWhileTheClusterIncidentKeepsAPaddedStoredInstanceIdTest` - the
repository returns a cluster incident stored with a padded instance id (what
the instance filter yields for such a row) and the business event must still be
suppressed by it, proving the incident is not silently dropped a second time
after the query.
- The test mocks `AlertRepository`, the same way the other cases of the
class do, so it pins the filter behaviour without a database.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`fix:`)
- [x] Tests added or updated for non-trivial changes, test methods named
`...Test`
- [x] New UI text has both Chinese and English entries under `web/src/i18n/`
- not applicable, no UI text added
- [x] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks)
- [x] New source files carry the ASF license header - no new source files
- [x] Documentation touched where behaviour changed - not applicable, the
correlation contract is unchanged for unpadded values
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
