[bugfix] support no-whitespace alert expressions in instance filtering #1
Conversation
WalkthroughA new constructor overload has been added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as RealTimeAlertCalculatorTest
participant Calculator as RealTimeAlertCalculator
participant MockDeps as Mocked Dependencies
Test->>MockDeps: Set up mocks for dependencies
Test->>Calculator: Instantiate with start=false
Test->>Calculator: Call filterThresholdsByAppAndMetrics()
Calculator->>MockDeps: (Uses dependencies as needed)
Calculator-->>Test: Returns filtered alert definitions
Test->>Test: Assert filtering correctness
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/calculate/RealTimeAlertCalculator.java (1)
116-116: Consider adding thread safety to the now-public startCalculate method.Since this method has been made public for testing purposes, it could potentially be called multiple times, which might lead to redundant worker threads being created.
public void startCalculate() { + // Add a synchronization mechanism or flag to prevent multiple calls + // For example: use AtomicBoolean to track if threads are already started Runnable runnable = () -> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/calculate/RealTimeAlertCalculator.java(4 hunks)hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/calculate/RealTimeAlertCalculatorTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/calculate/RealTimeAlertCalculatorTest.java (1)
web-app/src/app/pojo/AlertDefine.ts (1)
AlertDefine(20-41)
🔇 Additional comments (7)
hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/calculate/RealTimeAlertCalculatorTest.java (3)
25-38: Setup looks good with appropriate mocks.The test setup correctly mocks all required dependencies and passes
falseto the constructor to prevent threads from starting automatically during tests, which is a good practice for unit testing.
40-60: Well-structured test for expressions with spaces.This test case correctly verifies that the filter method works properly when there are spaces in the instance expression, which is the established behavior.
62-82: Good test case for the bugfix - handling expressions without spaces.This test case directly addresses the bug described in the PR summary, verifying that alert expressions without spaces between the comma and quotes (e.g.,
equals(__instance__,"501045327364864")) are properly filtered. This ensures the fix works as expected.hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/calculate/RealTimeAlertCalculator.java (4)
76-76: Fixed regex pattern to handle expressions without spaces.The regular expression pattern has been updated to include
\\s*(zero or more whitespace characters), which correctly handles both cases with and without spaces after commas in instance expressions.
85-90: Good constructor refactoring to support testing.The original constructor now delegates to the new overloaded constructor, passing
trueto start calculation threads by default. This maintains backward compatibility while enabling better testability.
92-114: Well-documented constructor overload for testability.The new constructor with the
startparameter is well-documented and provides better testability by allowing tests to create instances without starting alert calculation threads.
276-276: Method made public for testing purposes.This method visibility change allows direct testing of the filtering logic, which is essential for this bugfix.
What's changed?
apache#3220
This PR fixes a bug in RealTimeAlertCalculator where instance filtering failed when alert expressions contained no whitespace (e.g.,
equals(__app__,"redis") && equals(__instance__,"501045327364864")).The backend regex has been updated to support such compact expressions. A unit test is also added to ensure proper filtering across different formatting styles.
Checklist
Add or update API
Summary by CodeRabbit