feat: change relation between alerts and incidents to work with fingerprints instead of alert ids #2473
Conversation
dosubot
bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
VladimirFilonov
changed the title
VladimirFilonov
force-pushed
the
feature/2398-change-relation-between-alerts-and-incidents-to-work-with-fingerprints-instead-of-alert_ids
branch
from
234ac7f
to
e34fe00
Compare
VladimirFilonov
force-pushed
the
feature/2398-change-relation-between-alerts-and-incidents-to-work-with-fingerprints-instead-of-alert_ids
branch
5 times, most recently
from
6e8318d
to
05c057c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on transitioning the alert management system from using alert IDs to fingerprints for associating alerts with incidents. This involves modifications across various components, including the UI, database models, API routes, and tests. New database tables are introduced to store the latest alerts and their relationships with incidents, enhancing the system's efficiency and reducing the size of related tables. The overall control flow and logic remain intact, but the underlying data structures and identifiers have been updated to reflect this new approach. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (20)
tests/test_metrics.py (1)
Line range hint
13-31: Consider adding test cases for fingerprint-specific scenarios.While the basic functionality is tested, consider adding test cases for:
- Duplicate fingerprints to ensure metrics are counted correctly
- Empty fingerprints handling
- Updating existing incident-fingerprint relationships
Here's a suggested addition:
def test_add_alerts_with_duplicate_fingerprints( db_session, client, test_app, setup_stress_alerts_no_elastic ): # Create alerts with duplicate fingerprints alerts = setup_stress_alerts_no_elastic(3) alerts[1].fingerprint = alerts[0].fingerprint # Create duplicate fingerprint incident = create_incident_from_dict( "keep", {"user_generated_name": "test", "description": "test"} ) valid_api_key = "valid_api_key" setup_api_key(db_session, valid_api_key) # Add alerts with duplicate fingerprints add_alerts_to_incident_by_incident_id( "keep", incident.id, [a.fingerprint for a in alerts] ) response = client.get("/metrics?labels=a.b", headers={"X-API-KEY": "valid_api_key"}) # Should count unique fingerprints (2) instead of total alerts (3) assert ( f'alerts_total{{incident_name="test" incident_id="{incident.id}" a_b=""}} 2' in response.text.split("\n") )keep-ui/entities/incidents/model/models.ts (1)
34-34: Add JSDoc documentation for the new fingerprint property.The addition of the
fingerprintproperty aligns well with the PR objectives of transitioning from alert IDs to fingerprints. However, it would be beneficial to add documentation explaining its purpose and relationship with alerts.Consider adding JSDoc documentation:
+ /** Unique fingerprint identifying related alerts in this incident */ fingerprint: string;keep-ui/app/alerts/alert-associate-incident-modal.tsx (1)
Line range hint
1-190: Consider architectural implications of the fingerprint-based system.The component handles both incident creation and alert association. With the transition to fingerprint-based relationships, consider:
- Whether the
CreateOrUpdateIncidentFormcomponent needs updates to handle fingerprint-related data- If the incident creation flow should be aware of fingerprints
- Whether the polling mechanism (
usePollIncidents) needs adjustments for the new data structureConsider splitting this component into smaller, more focused components:
- One for incident selection
- One for incident creation
- One for alert association
This would make it easier to maintain and adapt to future changes in the fingerprint-based system.
keep/api/bl/incidents_bl.py (2)
Line range hint
134-147: Consider pagination for large fingerprint setsUsing
len(alert_fingerprints)as the limit parameter might lead to memory issues with large datasets. Consider implementing pagination or batch processing for better performance.db_alerts, _ = get_incident_alerts_by_incident_id( tenant_id=self.tenant_id, incident_id=incident_id, - limit=len(alert_fingerprints), + limit=min(len(alert_fingerprints), MAX_BATCH_SIZE), ) + # Process remaining alerts in batches if needed
Line range hint
208-221: Add error handling and logging for delete operationThe delete operation lacks proper error handling and logging compared to the add operation. Consider adding:
- Error handling for the deletion operation
- Logging after successful deletion
- Client notification of changes
Apply this improvement:
def delete_alerts_from_incident( self, incident_id: UUID, alert_fingerprints: List[str] ) -> None: + if not alert_fingerprints: + return self.logger.info( "Fetching incident", extra={ "incident_id": incident_id, + "alert_fingerprints": alert_fingerprints, "tenant_id": self.tenant_id, }, ) incident = get_incident_by_id(tenant_id=self.tenant_id, incident_id=incident_id) if not incident: raise HTTPException(status_code=404, detail="Incident not found") - remove_alerts_to_incident_by_incident_id(self.tenant_id, incident_id, alert_fingerprints) + try: + remove_alerts_to_incident_by_incident_id(self.tenant_id, incident_id, alert_fingerprints) + self.logger.info( + "Alerts removed from incident", + extra={ + "incident_id": incident_id, + "alert_fingerprints": alert_fingerprints, + "tenant_id": self.tenant_id, + }, + ) + self.__update_client_on_incident_change(incident_id) + except Exception as e: + self.logger.exception( + "Failed to remove alerts from incident", + extra={ + "incident_id": incident_id, + "alert_fingerprints": alert_fingerprints, + "tenant_id": self.tenant_id, + }, + ) + raise HTTPException(status_code=500, detail=str(e))keep/rulesengine/rulesengine.py (2)
Line range hint
147-148: Address TODO comment regarding unit testsThe TODO comment indicates missing unit tests for the rule evaluation logic. Given the complexity of CEL rule processing and the recent changes to alert-incident relationships, comprehensive testing is crucial.
Would you like me to help generate unit tests for the rule evaluation logic?
Line range hint
149-150: Consider fixing the source list workaroundThe comment indicates a temporary workaround for handling source as a list. This should be properly addressed to avoid potential issues.
Consider implementing proper list handling in the payload processing to eliminate the need for this workaround.
tests/test_rules_engine.py (3)
260-268: Consider optimizing bulk alert creation and last alert settingThe current implementation correctly sets fingerprints and updates last alerts. However, for better performance with bulk operations, consider creating a helper function that handles both bulk alert creation and last alert setting in a single database transaction.
def bulk_create_alerts_with_last_state(tenant_id: str, alerts: List[Alert], session) -> None: session.add_all(alerts) session.flush() # Ensure all alerts have IDs for alert in alerts: set_last_alert(tenant_id, alert, session) session.commit()
Line range hint
453-557: LGTM: Well-structured parameterized tests for resolution strategiesThe parameterized tests effectively cover both FIRST and LAST resolution strategies. Consider adding test cases for:
- Mixed status updates (some resolved, some firing)
- Concurrent resolution of multiple alerts
@pytest.mark.parametrize( "direction,status_sequence,expected_status", [ (ResolveOn.FIRST.value, [ ('fp1', AlertStatus.FIRING), ('fp2', AlertStatus.RESOLVED), ('fp1', AlertStatus.RESOLVED) ], IncidentStatus.RESOLVED), # Add more test cases here ] )🧰 Tools
🪛 Ruff
9-9:
sqlalchemy.descimported but unusedRemove unused import
(F401)
9-9:
sqlalchemy.ascimported but unusedRemove unused import
(F401)
Line range hint
559-573: Consider prioritizing implementation of critical test casesThe TODO list includes several test cases crucial for validating the fingerprint-based alert system, particularly:
- "test that alerts in the same group are being updated correctly"
- "test group are being updated correctly"
- "test that alerts in different groups are being updated correctly"
These cases are essential for ensuring the reliability of the new fingerprint-based relationship system.
Would you like me to help implement these test cases or create GitHub issues to track them?
🧰 Tools
🪛 Ruff
9-9:
sqlalchemy.descimported but unusedRemove unused import
(F401)
9-9:
sqlalchemy.ascimported but unusedRemove unused import
(F401)
tests/conftest.py (3)
19-19: Remove unused importThe
set_last_alertimport is not used in this file.-from keep.api.core.db import set_last_alert🧰 Tools
🪛 Ruff
19-19:
keep.api.core.db.set_last_alertimported but unusedRemove unused import:
keep.api.core.db.set_last_alert(F401)
552-577: LGTM! Consider explicit import for clarityThe LastAlert handling implementation is well-structured and efficient, using dictionary lookup for existing records and batch operations. However, to improve code clarity and address the static analysis warning:
-from keep.api.models.db.alert import * +from keep.api.models.db.alert import Alert, LastAlert🧰 Tools
🪛 Ruff
552-552:
LastAlertmay be undefined, or defined from star imports(F405)
568-568:
LastAlertmay be undefined, or defined from star imports(F405)
590-593: Enhance timing output readabilityConsider improving the timing output format for better readability.
- start_time = time.time() - alerts = setup_stress_alerts_no_elastic(num_alerts) - print(f"time taken to setup {num_alerts} alerts with db: ", time.time() - start_time) + start_time = time.time() + alerts = setup_stress_alerts_no_elastic(num_alerts) + elapsed_time = round(time.time() - start_time, 2) + print(f"Time taken to setup {num_alerts} alerts with db: {elapsed_time}s")keep/api/routes/incidents.py (1)
Line range hint
485-495: Consider consistent parameter naming.While the changes align with using fingerprints, consider using
alert_fingerprintsinstead offingerprintsto maintain consistency with the add endpoint and improve clarity.- fingerprints: List[str], + alert_fingerprints: List[str],🧰 Tools
🪛 Ruff
486-488: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
487-487: Do not perform function call
IdentityManagerFactory.get_auth_verifierin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
keep/api/utils/enrichment_helpers.py (2)
97-97: Avoid variable shadowing ofsessionUsing the same variable name
sessionfor both the function parameter and within the context manager can lead to confusion and potential errors due to variable shadowing. Consider renaming the variable within the context manager to avoid this.Apply this diff:
-with existed_or_new_session(session) as session: +with existed_or_new_session(session) as db_session:And update any references to
sessionwithin thewithblock todb_session.
111-112: Combine nested if statements for clarityTo improve readability and reduce unnecessary nesting, consider combining the nested
ifstatements into a single condition.Apply this diff:
-if with_incidents: - if alert.incidents: +if with_incidents and alert.incidents:🧰 Tools
🪛 Ruff
111-112: Use a single
ifstatement instead of nestedifstatements(SIM102)
keep/api/models/db/migrations/versions/2024-11-13-22-48_bdae8684d0b4.py (1)
21-21: Remove the unused variablemigration_metadata.The variable
migration_metadata(line 21) is defined but not used anywhere in the code. Unused variables can clutter the codebase and may cause confusion.Apply this diff to remove the unused variable:
- migration_metadata = sa.MetaData()keep/api/models/db/alert.py (1)
99-104: Remove redundantdefault_factory=Noneindeleted_atfieldIn the
deleted_atfield of theLastAlertToIncidentclass, bothdefault_factory=Noneanddefault=NULL_FOR_DELETED_ATare specified. Sincedefault_factoryanddefaultare mutually exclusive, consider removingdefault_factory=Noneto avoid redundancy.Apply this diff to clean up the field definition:
deleted_at: datetime = Field( - default_factory=None, nullable=True, primary_key=True, default=NULL_FOR_DELETED_AT, )tests/test_incidents.py (1)
357-362: Session parameter redundancy inadd_alerts_to_incident_by_incident_idThe
sessionparameter is being passed toadd_alerts_to_incident_by_incident_idat line 361, but if the function already uses the session context, this might be redundant.Consider removing the
sessionparameter if it's unnecessary.keep/api/core/db.py (1)
3559-3561: Simplify nested 'with' statementsTo improve readability, combine nested
withstatements into a single statement.Apply this diff:
- with existed_or_new_session(session) as session: - with session.no_autoflush: + with existed_or_new_session(session) as session, session.no_autoflush:🧰 Tools
🪛 Ruff
3559-3561: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
keep-ui/app/alerts/alert-associate-incident-modal.tsx(1 hunks)keep-ui/app/incidents/[id]/alerts/incident-alert-menu.tsx(1 hunks)keep-ui/entities/incidents/model/models.ts(1 hunks)keep/api/bl/incidents_bl.py(3 hunks)keep/api/core/db.py(33 hunks)keep/api/models/db/alert.py(5 hunks)keep/api/models/db/migrations/versions/2024-11-13-22-48_bdae8684d0b4.py(1 hunks)keep/api/routes/incidents.py(5 hunks)keep/api/routes/workflows.py(1 hunks)keep/api/tasks/process_event_task.py(2 hunks)keep/api/utils/enrichment_helpers.py(3 hunks)keep/rulesengine/rulesengine.py(2 hunks)tests/conftest.py(4 hunks)tests/test_incidents.py(18 hunks)tests/test_metrics.py(1 hunks)tests/test_rules_engine.py(9 hunks)
🧰 Additional context used
🪛 Ruff
keep/api/core/db.py
1300-1300: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1302-1302: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1305-1305: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1305-1305: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
1306-1306: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1314-1314: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1316-1316: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1319-1319: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1319-1319: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
1320-1320: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1328-1328: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1330-1330: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1334-1334: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1334-1334: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
1335-1335: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1343-1343: Alert may be undefined, or defined from star imports
(F405)
1791-1791: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1794-1794: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1797-1797: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1808-1808: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1810-1810: Incident may be undefined, or defined from star imports
(F405)
1810-1810: Rule may be undefined, or defined from star imports
(F405)
1810-1810: Incident may be undefined, or defined from star imports
(F405)
1811-1811: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1811-1811: Incident may be undefined, or defined from star imports
(F405)
1811-1811: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1813-1813: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1813-1813: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
1814-1814: LastAlertToIncident may be undefined, or defined from star imports
(F405)
2821-2821: Incident may be undefined, or defined from star imports
(F405)
2823-2823: Optional may be undefined, or defined from star imports
(F405)
2829-2829: UUID may be undefined, or defined from star imports
(F405)
2833-2833: LastAlertToIncident may be undefined, or defined from star imports
(F405)
2834-2834: LastAlertToIncident may be undefined, or defined from star imports
(F405)
2835-2835: LastAlertToIncident may be undefined, or defined from star imports
(F405)
2836-2836: LastAlertToIncident may be undefined, or defined from star imports
(F405)
2837-2837: LastAlertToIncident may be undefined, or defined from star imports
(F405)
2837-2837: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3102-3102: Incident may be undefined, or defined from star imports
(F405)
3102-3102: Optional may be undefined, or defined from star imports
(F405)
3105-3105: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3105-3105: Alert may be undefined, or defined from star imports
(F405)
3106-3106: LastAlert may be undefined, or defined from star imports
(F405)
3107-3107: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3108-3108: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3108-3108: LastAlert may be undefined, or defined from star imports
(F405)
3109-3109: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3109-3109: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3111-3111: Alert may be undefined, or defined from star imports
(F405)
3111-3111: LastAlert may be undefined, or defined from star imports
(F405)
3111-3111: Alert may be undefined, or defined from star imports
(F405)
3113-3113: LastAlert may be undefined, or defined from star imports
(F405)
3114-3114: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3207-3207: UUID may be undefined, or defined from star imports
(F405)
3208-3208: Optional may be undefined, or defined from star imports
(F405)
3209-3209: Optional may be undefined, or defined from star imports
(F405)
3209-3209: Incident may be undefined, or defined from star imports
(F405)
3212-3212: Incident may be undefined, or defined from star imports
(F405)
3214-3214: Incident may be undefined, or defined from star imports
(F405)
3215-3215: Incident may be undefined, or defined from star imports
(F405)
3354-3354: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3356-3356: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3357-3357: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3387-3387: Alert may be undefined, or defined from star imports
(F405)
3387-3387: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3392-3392: Alert may be undefined, or defined from star imports
(F405)
3393-3393: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3395-3395: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3396-3396: LastAlert may be undefined, or defined from star imports
(F405)
3396-3396: LastAlert may be undefined, or defined from star imports
(F405)
3396-3396: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3397-3397: Alert may be undefined, or defined from star imports
(F405)
3397-3397: LastAlert may be undefined, or defined from star imports
(F405)
3397-3397: Alert may be undefined, or defined from star imports
(F405)
3399-3399: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3400-3400: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3402-3402: LastAlert may be undefined, or defined from star imports
(F405)
3403-3403: Alert may be undefined, or defined from star imports
(F405)
3407-3407: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3407-3407: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3469-3469: Optional may be undefined, or defined from star imports
(F405)
3494-3494: LastAlert may be undefined, or defined from star imports
(F405)
3495-3495: Alert may be undefined, or defined from star imports
(F405)
3495-3495: LastAlert may be undefined, or defined from star imports
(F405)
3495-3495: Alert may be undefined, or defined from star imports
(F405)
3497-3497: LastAlert may be undefined, or defined from star imports
(F405)
3498-3498: LastAlert may be undefined, or defined from star imports
(F405)
3527-3527: UUID may be undefined, or defined from star imports
(F405)
3548-3548: Incident may be undefined, or defined from star imports
(F405)
3551-3551: Optional may be undefined, or defined from star imports
(F405)
3553-3553: Optional may be undefined, or defined from star imports
(F405)
3553-3553: Incident may be undefined, or defined from star imports
(F405)
3559-3561: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
3567-3567: LastAlert may be undefined, or defined from star imports
(F405)
3568-3568: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3568-3568: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3568-3568: LastAlert may be undefined, or defined from star imports
(F405)
3570-3570: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3570-3570: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3571-3571: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3572-3572: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3609-3609: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3632-3632: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3632-3632: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3632-3632: Alert may be undefined, or defined from star imports
(F405)
3634-3634: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3634-3634: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3635-3635: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3636-3636: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3653-3653: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3655-3655: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3655-3655: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3656-3656: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3657-3657: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3669-3669: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3671-3671: LastAlert may be undefined, or defined from star imports
(F405)
3672-3672: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3672-3672: LastAlert may be undefined, or defined from star imports
(F405)
3672-3672: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3673-3673: Alert may be undefined, or defined from star imports
(F405)
3673-3673: LastAlert may be undefined, or defined from star imports
(F405)
3673-3673: Alert may be undefined, or defined from star imports
(F405)
3675-3675: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3675-3675: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3676-3676: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3691-3691: UUID may be undefined, or defined from star imports
(F405)
3706-3706: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3708-3708: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3708-3708: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3709-3709: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3710-3710: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3711-3711: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3732-3732: LastAlert may be undefined, or defined from star imports
(F405)
3733-3733: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3733-3733: LastAlert may be undefined, or defined from star imports
(F405)
3733-3733: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3734-3734: Alert may be undefined, or defined from star imports
(F405)
3734-3734: LastAlert may be undefined, or defined from star imports
(F405)
3734-3734: Alert may be undefined, or defined from star imports
(F405)
3736-3736: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3736-3736: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3737-3737: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3747-3747: LastAlert may be undefined, or defined from star imports
(F405)
3748-3748: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3748-3748: LastAlert may be undefined, or defined from star imports
(F405)
3748-3748: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3749-3749: Alert may be undefined, or defined from star imports
(F405)
3749-3749: LastAlert may be undefined, or defined from star imports
(F405)
3749-3749: Alert may be undefined, or defined from star imports
(F405)
3751-3751: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3751-3751: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
3752-3752: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3772-3772: LastAlert may be undefined, or defined from star imports
(F405)
3773-3773: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3773-3773: LastAlert may be undefined, or defined from star imports
(F405)
3773-3773: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3774-3774: Alert may be undefined, or defined from star imports
(F405)
3774-3774: LastAlert may be undefined, or defined from star imports
(F405)
3774-3774: Alert may be undefined, or defined from star imports
(F405)
3776-3776: LastAlertToIncident may be undefined, or defined from star imports
(F405)
3777-3777: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4239-4239: LastAlert may be undefined, or defined from star imports
(F405)
4239-4239: WorkflowToAlertExecution may be undefined, or defined from star imports
(F405)
4239-4239: LastAlert may be undefined, or defined from star imports
(F405)
4241-4241: Alert may be undefined, or defined from star imports
(F405)
4241-4241: LastAlert may be undefined, or defined from star imports
(F405)
4241-4241: Alert may be undefined, or defined from star imports
(F405)
4242-4242: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4242-4242: Alert may be undefined, or defined from star imports
(F405)
4242-4242: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4244-4244: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4244-4244: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
4245-4245: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4288-4288: LastAlert may be undefined, or defined from star imports
(F405)
4289-4289: Alert may be undefined, or defined from star imports
(F405)
4289-4289: LastAlert may be undefined, or defined from star imports
(F405)
4289-4289: Alert may be undefined, or defined from star imports
(F405)
4291-4291: AlertEnrichment may be undefined, or defined from star imports
(F405)
4291-4291: Alert may be undefined, or defined from star imports
(F405)
4291-4291: AlertEnrichment may be undefined, or defined from star imports
(F405)
4293-4293: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4293-4293: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4293-4293: LastAlert may be undefined, or defined from star imports
(F405)
4295-4295: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4295-4295: NULL_FOR_DELETED_AT may be undefined, or defined from star imports
(F405)
4296-4296: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4354-4354: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4354-4354: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4354-4354: Alert may be undefined, or defined from star imports
(F405)
4355-4355: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4396-4396: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4400-4400: LastAlert may be undefined, or defined from star imports
(F405)
4400-4400: Alert may be undefined, or defined from star imports
(F405)
4400-4400: LastAlert may be undefined, or defined from star imports
(F405)
4401-4401: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4401-4401: LastAlert may be undefined, or defined from star imports
(F405)
4401-4401: LastAlertToIncident may be undefined, or defined from star imports
(F405)
4493-4493: Optional may be undefined, or defined from star imports
(F405)
4494-4494: Optional may be undefined, or defined from star imports
(F405)
4494-4494: Alert may be undefined, or defined from star imports
(F405)
4497-4497: LastAlert may be undefined, or defined from star imports
(F405)
4500-4500: LastAlert may be undefined, or defined from star imports
(F405)
4501-4501: LastAlert may be undefined, or defined from star imports
(F405)
4507-4507: Alert may be undefined, or defined from star imports
(F405)
4507-4507: Optional may be undefined, or defined from star imports
(F405)
4522-4522: LastAlert may be undefined, or defined from star imports
(F405)
keep/api/utils/enrichment_helpers.py
111-112: Use a single if statement instead of nested if statements
(SIM102)
tests/conftest.py
19-19: keep.api.core.db.set_last_alert imported but unused
Remove unused import: keep.api.core.db.set_last_alert
(F401)
552-552: LastAlert may be undefined, or defined from star imports
(F405)
568-568: LastAlert may be undefined, or defined from star imports
(F405)
tests/test_incidents.py
16-16: keep.api.core.db.enrich_incidents_with_alerts imported but unused
Remove unused import: keep.api.core.db.enrich_incidents_with_alerts
(F401)
26-26: keep.api.models.db.alert.AlertToIncident imported but unused
Remove unused import: keep.api.models.db.alert.AlertToIncident
(F401)
🔇 Additional comments (27)
tests/test_metrics.py (2)
21-21: LGTM! The change aligns with the fingerprint transition.
The modification correctly updates the test to use fingerprints instead of alert IDs, which is consistent with the architectural changes described in the PR objectives.
21-21: Verify data migration impact on metrics.
Since this PR involves a database migration from alert IDs to fingerprints, we should verify that the metrics system correctly handles both old and new data during the migration period.
keep-ui/entities/incidents/model/models.ts (1)
34-34: Verify if fingerprint should be optional.
Since this is a new property being added to an existing interface, consider whether it should be marked as optional (fingerprint?: string) to maintain backward compatibility during the migration period.
Let's check how this interface is used in components:
✅ Verification successful
The fingerprint property should be required, not optional
Based on the codebase analysis:
- The
fingerprintproperty is consistently used alongsiderule_fingerprintin incident-related operations - API endpoints expect and return incidents with fingerprint (
getIncidentWithErrorHandling.tsx,useIncidentActions.tsx) - Components consuming
IncidentDto(IncidentActivity,IncidentChat, etc.) handle incidents with fingerprint - No evidence of backward compatibility concerns as this appears to be a core identifying property similar to existing
rule_fingerprint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of IncidentDto to determine if fingerprint should be optional
# Look for components that create or modify IncidentDto objects
# Search for direct IncidentDto object creation or modification
rg -t typescript "IncidentDto.*=.*{" keep-ui/
# Search for API responses typed as IncidentDto
rg -t typescript "Promise<IncidentDto>" keep-ui/
# Search for components that might need updates for the new property
rg -t typescript "user_generated_name.*status.*alerts_count" keep-ui/
Length of output: 338
Script:
#!/bin/bash
# Search for IncidentDto usage patterns with proper regex escaping
rg "IncidentDto\s*=\s*\{" keep-ui/
# Search for API responses and type definitions
rg "Promise<.*IncidentDto" keep-ui/
# Search for IncidentDto interface usage
rg ":\s*IncidentDto[\[\]\s\{\}]" keep-ui/
# Search for fingerprint property usage
rg "fingerprint" keep-ui/
Length of output: 9718
keep-ui/app/incidents/[id]/alerts/incident-alert-menu.tsx (1)
26-26: LGTM! Change aligns with the fingerprint-based relationship.
The modification correctly implements the transition from alert IDs to fingerprints as outlined in the PR objectives.
Let's verify the API endpoint compatibility and type definitions:
✅ Verification successful
Change is safe and correctly implemented
The verification confirms:
AlertDtointerface inmodels.tsxincludes thefingerprintfield- The component correctly uses the alert's fingerprint in the DELETE request payload
- Proper error handling and user confirmation are in place
- Authentication and content-type headers are correctly set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the API endpoint and AlertDto type definitions
# Check if AlertDto includes fingerprint field
ast-grep --pattern 'interface AlertDto {
$$$
fingerprint: $_
$$$
}'
# Check API endpoint handler for fingerprint support
rg -A 10 "DELETE.*incidents/.*/alerts" --type py
Length of output: 1912
Script:
#!/bin/bash
# Let's search for the API endpoint implementation in Python files using a broader pattern
rg -A 10 "incidents.*alerts.*delete" --type py -i
# Also check for FastAPI route definitions
rg -A 10 "@router\.(delete|app\.delete).*incidents.*alerts" --type py
# Let's also check the API client implementation to understand the expected payload
rg "removeAlertFromIncident" keep-ui/app/**/*.ts*
Length of output: 5378
Script:
#!/bin/bash
# Let's check the alerts client implementation for the API endpoint
rg -A 10 "removeAlertFromIncident|DELETE.*incidents.*alerts" keep-ui/app/alerts/alerts.client.tsx
# Also check the incident-alert-menu component implementation
cat keep-ui/app/incidents/[id]/alerts/incident-alert-menu.tsx
Length of output: 1805
keep-ui/app/alerts/alert-associate-incident-modal.tsx (2)
Line range hint 46-54: Verify API endpoint compatibility with the new payload structure.
The change to use fingerprints instead of alert IDs is correct according to the architectural changes. However, we should ensure proper error handling for potential fingerprint-related issues.
Let's verify the API endpoint implementation:
Consider adding error handling specific to fingerprint validation:
if (response.ok) {
handleSuccess();
await mutate();
toast.success("Alerts associated with incident successfully");
} else {
+ const errorData = await response.json().catch(() => ({}));
toast.error(
- "Failed to associated alerts with incident, please contact us if this issue persists."
+ errorData.message || "Failed to associate alerts with incident. Please ensure all fingerprints are valid."
);
}✅ Verification successful
These commands will help us verify the API endpoint implementation and how it handles fingerprints. If these don't yield results, we'll try searching in other potential locations or with different patterns.
Based on the search results, I can see that the API endpoint implementation in keep/api/routes/incidents.py accepts fingerprints for associating alerts with incidents. The codebase shows consistent usage of fingerprints throughout the system, including database operations, alert deduplication, and incident management. The backend is properly handling fingerprint-based operations with appropriate database models and migrations.
The API endpoint is compatible with the fingerprint-based payload structure and includes proper error handling.
- The backend uses fingerprints as the primary identifier for alerts across the system
- Database models and migrations support fingerprint-based operations
- The incident management system is designed to work with alert fingerprints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the POST /incidents/{id}/alerts endpoint implementation
rg -t python "def.*add_alerts_to_incident" -A 10
Length of output: 174
Script:
#!/bin/bash
# Let's search for API endpoint implementation in Python files
rg -g "*.py" "add_alerts_to_incident" -A 10
# Also search for any fingerprint-related validation or processing
rg -g "*.py" "fingerprint.*alert" -A 5
# Let's check the FastAPI/Flask routes for incidents
rg -g "*.py" "@.*route.*/incidents/.*alerts" -A 10
Length of output: 90803
47-47: Verify type safety of the fingerprint property.
The change from event_id to fingerprint aligns with the architectural changes. However, we should ensure type safety.
Let's verify the AlertDto interface includes the fingerprint property:
✅ Verification successful
Type safety of fingerprint property is verified
The AlertDto interface in keep-ui/app/alerts/models.tsx explicitly includes the fingerprint property as a required string field. The component correctly uses this type through Array<AlertDto> in its props interface, ensuring type safety for the fingerprint access in the API payload.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AlertDto interface definition
rg -t typescript "interface AlertDto" -A 10
Length of output: 174
Script:
#!/bin/bash
# Search for interface or type definitions containing Alert and fingerprint
rg -l "interface.*Alert|type.*Alert" | xargs rg "fingerprint"
# Search for the actual usage of fingerprint in the modal component
cat "keep-ui/app/alerts/alert-associate-incident-modal.tsx"
Length of output: 180812
Script:
#!/bin/bash
# Search for AlertDto interface/type definition and fingerprint property
rg -g "*.ts*" "type AlertDto|interface AlertDto" -A 20
# Also check the actual usage of AlertDto in the modal component
ast-grep --pattern 'interface AlertAssociateIncidentModalProps {
$$$
alerts: Array<AlertDto>;
$$$
}'
Length of output: 1693
keep/api/bl/incidents_bl.py (1)
Line range hint 97-221: Verify callers and database schema changes
Let's verify that all callers have been updated to use fingerprints and the database schema changes are in place.
✅ Verification successful
Based on the comprehensive results from the shell scripts, I can now generate the final response:
Transition to alert fingerprints is properly implemented
The codebase has been consistently updated to use alert fingerprints instead of alert IDs for incident management. Key findings:
- All callers of
add_alerts_to_incidentanddelete_alerts_from_incidentuse fingerprints - Database functions
add_alerts_to_incident_by_incident_idandremove_alerts_to_incident_by_incident_idare properly implemented with fingerprints - Test coverage is comprehensive, including:
- Tests for adding/removing alerts with same fingerprints
- Tests for incident merging with fingerprints
- Tests for alert status changes
- Tests for metadata handling
Remaining alert_ids usages are in different contexts (Grafana provider, AI suggestions) and don't affect this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the transition from alert_ids to alert_fingerprints
# Check for any remaining references to alert_ids in the codebase
echo "Checking for remaining alert_ids references..."
rg -l "alert_ids.*List\[UUID\]" --type py
# Check for database migration files
echo "Checking for related migration files..."
fd -e py -p ".*alert.*incident.*" migrations/
# Check for test coverage
echo "Checking for related test files..."
fd -e py -p "test.*incident.*" tests/
Length of output: 525
Script:
#!/bin/bash
# Let's check the actual implementation and test coverage
# Check test file content for alert_fingerprints usage
echo "Checking test implementation..."
cat tests/test_incidents.py
# Check for any direct callers of these methods
echo "Checking for callers..."
rg -A 5 "add_alerts_to_incident|delete_alerts_from_incident" --type py
# Check database related functions
echo "Checking database functions..."
rg -A 5 "add_alerts_to_incident_by_incident_id|remove_alerts_to_incident_by_incident_id" --type py
# Check if there are any remaining alert_ids usages in the codebase
echo "Checking for alert_ids usage..."
rg "alert_ids" --type py
Length of output: 56183
keep/rulesengine/rulesengine.py (2)
Line range hint 104-108: LGTM: Improved readability of resolution logic
The change to use elif for the last condition makes the code more maintainable by clearly indicating that the conditions are mutually exclusive. The logic remains functionally equivalent while being more readable.
84-84: Verify fingerprint-based alert assignment implementation
The change from alert_id to fingerprint aligns with the PR objectives to reduce the AlertToIncident table size. However, we should verify the complete implementation.
✅ Verification successful
Fingerprint-based alert assignment is correctly implemented
The change from alert_id to fingerprint is properly implemented across the codebase:
- The
assign_alert_to_incidentfunction inkeep/api/core/db.pyacceptsfingerprintparameter - The Alert model has fingerprint support through
AlertEnrichmentrelationship - The fingerprint is consistently used across multiple components:
- Rules engine (
rulesengine.py) - Event processing (
process_event_task.py) - Workflow scheduler (
workflowscheduler.py)
- Rules engine (
- The
AlertAudittable also tracks changes using fingerprint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation completeness of fingerprint-based alert assignment
# Check if assign_alert_to_incident function signature has been updated
rg -A 5 "def assign_alert_to_incident"
# Verify that AlertDto has the fingerprint attribute
ast-grep --pattern 'class AlertDto:
$$$
fingerprint: $_
$$$'
# Check for any remaining alert_id usages that might need updating
rg "alert_id.*=.*event\.event_id"
Length of output: 403
Script:
#!/bin/bash
# Let's verify the event object structure and fingerprint usage
# Check the Event/AlertDto class definition for fingerprint
rg -B 5 -A 10 "class.*Event"
# Look for fingerprint attribute usage in the codebase
rg "fingerprint.*=.*event\.fingerprint"
# Check for any remaining event_id references that might need updating
rg "event_id.*=.*event\."
# Look for AlertToIncident table definition
rg -A 10 "class AlertToIncident"
Length of output: 4276
Script:
#!/bin/bash
# Let's check the Event class and Alert model to understand the complete picture
# Check Alert model definition for fingerprint field
rg -B 5 -A 10 "class Alert.*table=True"
# Look for Event class definition that contains fingerprint
ast-grep --pattern 'class $_Event:
$$$
fingerprint: $_
$$$'
# Check the formatted_event usage and its class
rg -B 2 -A 2 "formatted_event.*=.*"
Length of output: 13768
tests/test_rules_engine.py (3)
11-12: LGTM: Import changes align with new fingerprint tracking functionality
The addition of set_last_alert import is consistent with the PR's objective to track the latest alert state using fingerprints.
Line range hint 367-451: LGTM: Comprehensive test coverage for incident resolution
The test case thoroughly validates the incident resolution behavior with the new fingerprint-based approach, including:
- Multiple alerts with same/different fingerprints
- Proper status transitions
- Alert count verification
🧰 Tools
🪛 Ruff
9-9: sqlalchemy.desc imported but unused
Remove unused import
(F401)
9-9: sqlalchemy.asc imported but unused
Remove unused import
(F401)
62-68: Verify consistent usage of fingerprint and set_last_alert pattern
The pattern of setting fingerprint and calling set_last_alert after commit is correct. Let's verify this pattern is consistently followed across the codebase.
✅ Verification successful
Let me gather more specific information about Alert creation patterns to verify the fingerprint usage.
Pattern of fingerprint and set_last_alert is consistently implemented
Based on the verification results:
- All Alert creations include the fingerprint field in both test and production code
- The set_last_alert is consistently called after db_session.commit() in all cases
- The pattern is properly implemented in both single alert and bulk alert scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Alert creation patterns that might be missing fingerprint or set_last_alert
ast-grep --pattern 'Alert(
$$$
event=$_,
$$$
)'
# Search for database commits followed by set_last_alert
rg -B 3 -A 3 'db_session.commit()'
Length of output: 12163
Script:
#!/bin/bash
# Search for Alert creation patterns to verify fingerprint field
ast-grep --pattern 'Alert(
$$$
)'
# Search for set_last_alert usage
rg "set_last_alert\(" -B 2 -A 2
Length of output: 8866
keep/api/tasks/process_event_task.py (2)
25-25: LGTM: Import addition is well-placed
The addition of set_last_alert import is properly grouped with other database-related imports.
187-189:
Add error handling for set_last_alert operation
The set_last_alert operation should be wrapped in a try-except block like other DB operations in this function to maintain consistency and prevent potential failures from breaking the entire transaction.
Apply this change:
-
- set_last_alert(tenant_id, alert, session=session)
-
+ try:
+ set_last_alert(tenant_id, alert, session=session)
+ except Exception:
+ logger.exception(
+ "Failed to set last alert",
+ extra={
+ "tenant_id": tenant_id,
+ "alert_id": alert.id,
+ "fingerprint": formatted_event.fingerprint,
+ },
+ )
+ # Don't raise here as this is not a critical operation
+ # The alert is still saved and can be processedAdditionally, verify the behavior of set_last_alert in case of transaction rollback.
keep/api/routes/incidents.py (3)
463-463: LGTM! Parameter change aligns with fingerprint-based approach.
The parameter rename from alert_ids to alert_fingerprints accurately reflects the transition to using fingerprints instead of alert IDs.
473-473: Verify the business logic layer implementation.
The function call has been updated to use fingerprints. Let's verify that the incident_bl.add_alerts_to_incident method has been properly updated to handle fingerprints.
✅ Verification successful
Business logic layer implementation is properly updated
The add_alerts_to_incident method in keep/api/bl/incidents_bl.py has been correctly implemented to accept incident_id and alert_fingerprints parameters, matching the function call signature in the routes layer. The implementation is consistent with the changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of add_alerts_to_incident in the business logic layer
ast-grep --pattern 'async def add_alerts_to_incident(self, incident_id, alert_fingerprints)'
Length of output: 244
Script:
#!/bin/bash
# Search for the implementation of add_alerts_to_incident using ripgrep
rg "async def add_alerts_to_incident" -A 5
Length of output: 916
617-617:
Avoid accessing private attributes.
The change from incident.alerts to incident._alerts suggests accessing a private attribute. This is generally discouraged as it:
- Breaks encapsulation
- Makes the code more fragile to internal implementation changes
- Could indicate a missing public accessor method
Consider using a public property or method instead.
keep/api/utils/enrichment_helpers.py (1)
104-107:
Missing else statement may cause unexpected behavior
There appears to be a missing else: statement before line 107, which could lead to incorrect variable assignments and unexpected behavior.
Apply this diff:
if isinstance(_object, Alert):
alert, alert_to_incident = _object, None
+else:
alert, alert_to_incident = _objectLikely invalid or redundant comment.
keep/api/models/db/alert.py (5)
64-65: Removal of relationships in AlertToIncident is acceptable
Commenting out the alert and incident relationships in the AlertToIncident class aligns with the shift from using alert IDs to fingerprints. This change helps in modifying the relationship between alerts and incidents as per the PR objectives.
76-83: New LastAlert class is correctly defined
The introduction of the LastAlert class with appropriate fields and primary key aligns with the new data model. It effectively stores the latest alert information using fingerprints.
84-118: New LastAlertToIncident class is correctly defined
The LastAlertToIncident class properly models the relationship between the latest alerts and incidents using fingerprints. The field definitions and primary keys are set up correctly, supporting the new structure.
143-146: Removal of alert_to_incident_link relationship in Incident is acceptable
Commenting out the alert_to_incident_link relationship in the Incident class reflects the updated data model that now utilizes fingerprints. This aligns with the PR objectives to simplify the alert-to-incident relationship.
215-215: Use of PrivateAttr for _alerts in Incident
Changing _alerts to a private attribute using PrivateAttr() is appropriate if the attribute is intended for internal use within the class. Ensure that this change does not impact any external access or ORM relationships that depend on _alerts.
tests/test_incidents.py (3)
520-526: Clarify initial assertion on empty incident alerts
At lines 525-526, you're asserting that the incident initially has zero alerts. Ensure that this is expected and that any setup steps haven't added alerts to the incident unintentionally.
592-595: Consistent use of fingerprints in alert addition
Ensure that when adding alerts to incidents, you're consistently using fingerprints. At line 592, you're using [a.fingerprint for a in alerts_1], which is correct.
245-255:
Ensure correct AlertStatus conversion
In the create_alert call starting at line 245, you're passing AlertStatus(status), where status is obtained from IncidentStatus. This may lead to incorrect or invalid AlertStatus values.
Please verify that status corresponds to a valid AlertStatus. If necessary, map IncidentStatus to AlertStatus appropriately.
keep/api/core/db.py (1)
Line range hint 1791-1814: Duplicate issue with 'LastAlertToIncident' usage
The same issue with LastAlertToIncident being potentially undefined occurs here due to wildcard imports.
🧰 Tools
🪛 Ruff
1791-1791: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1794-1794: LastAlertToIncident may be undefined, or defined from star imports
(F405)
1797-1797: LastAlertToIncident may be undefined, or defined from star imports
(F405)
keep/api/models/db/migrations/versions/2024-11-13-22-48_bdae8684d0b4.py
Outdated
Show resolved
Hide resolved
keep/api/models/db/migrations/versions/2024-11-13-22-48_bdae8684d0b4.py
Outdated
Show resolved
Hide resolved
keep/api/models/db/migrations/versions/2024-11-13-22-48_bdae8684d0b4.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
keep/api/routes/workflows.py (3)
Line range hint
673-691: Add error handling for database operations.While the tracing implementation is good, the function should handle potential database errors to prevent unhandled exceptions from propagating to the client.
Consider wrapping the database operation in a try-catch block:
@router.get("/executions", description="Get workflow executions by alert fingerprint") def get_workflow_executions_by_alert_fingerprint( authenticated_entity: AuthenticatedEntity = Depends( IdentityManagerFactory.get_auth_verifier(["read:workflows"]) ), session: Session = Depends(get_session), ) -> list[WorkflowToAlertExecutionDTO]: with tracer.start_as_current_span("get_workflow_executions_by_alert_fingerprint"): + try: latest_workflow_to_alert_executions = ( get_last_workflow_workflow_to_alert_executions( session=session, tenant_id=authenticated_entity.tenant_id ) ) + except Exception as e: + logger.error(f"Failed to fetch workflow executions: {e}") + raise HTTPException( + status_code=500, + detail="Failed to fetch workflow executions", + )
Line range hint
193-210: Improve input validation using Pydantic models.The current validation using try-except with TypeError is too broad. Consider using Pydantic models for better type safety and validation.
Consider creating a dedicated Pydantic model:
from pydantic import BaseModel, Field from typing import Optional, List from datetime import datetime class EventBody(BaseModel): id: Optional[str] = None name: Optional[str] = None fingerprint: Optional[str] = None source: Optional[List[str]] = Field(default_factory=list) lastReceived: Optional[datetime] = NoneThen update the validation:
- try: - event = event_class(**event_body) - except TypeError: + try: + validated_body = EventBody(**event_body) + event = event_class(**validated_body.dict()) + except ValidationError as e: raise HTTPException( status_code=400, - detail="Invalid event format", + detail=f"Invalid event format: {str(e)}", )
Line range hint
1-1000: Consider architectural improvements for better maintainability.The codebase could benefit from the following improvements:
- Create a centralized error handler using FastAPI's exception handlers
- Extract common validation logic into reusable functions
- Break down large functions into smaller, more focused ones
- Standardize response formats across endpoints
Here's an example of how to implement a centralized error handler:
from fastapi import FastAPI, Request from fastapi.responses import JSONResponse @app.exception_handler(HTTPException) async def http_exception_handler(request: Request, exc: HTTPException): return JSONResponse( status_code=exc.status_code, content={ "status": "error", "message": exc.detail, "code": exc.status_code } ) @app.exception_handler(Exception) async def general_exception_handler(request: Request, exc: Exception): return JSONResponse( status_code=500, content={ "status": "error", "message": "Internal server error", "code": 500 } )Consider extracting common validation logic:
def validate_workflow_exists(tenant_id: str, workflow_id: str) -> Workflow: workflow = get_workflow(tenant_id=tenant_id, workflow_id=workflow_id) if not workflow: raise HTTPException( status_code=404, detail=f"Workflow {workflow_id} not found" ) return workflowtests/test_incidents.py (1)
245-254: Simplify alert creation in testsThe alert creation block could be simplified to reduce code duplication. Consider creating a helper function for generating test alerts with specific fingerprints.
def create_test_alert(fingerprint, status, severity, service): return create_alert( fingerprint, AlertStatus(status), datetime.utcnow(), { "severity": AlertSeverity.from_number(severity), "service": service, }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
keep/api/models/db/migrations/versions/2024-11-13-22-48_bdae8684d0b4.py(1 hunks)keep/api/routes/workflows.py(1 hunks)tests/test_incidents.py(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keep/api/models/db/migrations/versions/2024-11-13-22-48_bdae8684d0b4.py
🔇 Additional comments (4)
keep/api/routes/workflows.py (1)
193-194: LGTM: Operator precedence issue has been fixed.
The parentheses in the condition ensure correct evaluation order.
tests/test_incidents.py (3)
5-5: LGTM: Import changes align with fingerprint tracking
The addition of LastAlertToIncident and desc imports supports the new fingerprint-based alert tracking system.
Also applies to: 26-26
539-547: LGTM: Proper verification of latest alert timestamp
The test correctly verifies that the incident is associated with the latest alert for a given fingerprint, which is crucial for the new fingerprint-based system.
Line range hint 694-799: Address the commented-out test case
The test test_merge_incidents_app is commented out. This test appears to be important for verifying the API endpoints for merging incidents. Consider either updating it to work with fingerprints or removing it if it's no longer needed.
Would you like help updating this test to work with the new fingerprint-based system?
There was a problem hiding this comment.
Kudos, @VladimirFilonov, awesome work! Overall, looks good to me. left one minor comment
dosubot
bot
added
size:L
|
|
VladimirFilonov
force-pushed
the
feature/2398-change-relation-between-alerts-and-incidents-to-work-with-fingerprints-instead-of-alert_ids
branch
from
0ca5e87
to
8e61ae5
Compare
dosubot
bot
added
size:XL
VladimirFilonov
force-pushed
the
feature/2398-change-relation-between-alerts-and-incidents-to-work-with-fingerprints-instead-of-alert_ids
branch
from
8e61ae5
to
70c67fa
Compare
VladimirFilonov
force-pushed
the
feature/2398-change-relation-between-alerts-and-incidents-to-work-with-fingerprints-instead-of-alert_ids
branch
from
0c90b89
to
7c50d25
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2473 +/- ##
==========================================
- Coverage 69.63% 69.30% -0.33%
==========================================
Files 160 163 +3
Lines 15149 15402 +253
==========================================
+ Hits 10549 10675 +126
- Misses 4600 4727 +127 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
VladimirFilonov
force-pushed
the
feature/2398-change-relation-between-alerts-and-incidents-to-work-with-fingerprints-instead-of-alert_ids
branch
from
e05f0b9
to
7eeec0d
Compare
Co-authored-by: Kirill Chernakov <[email protected]> Signed-off-by: Vladimir Filonov <[email protected]>
VladimirFilonov
force-pushed
the
feature/2398-change-relation-between-alerts-and-incidents-to-work-with-fingerprints-instead-of-alert_ids
branch
from
7eeec0d
to
7a6d912
Compare
…and-incidents-to-work-with-fingerprints-instead-of-alert_ids
Closes #2398
📑 Description
✅ Checks
ℹ Additional Information
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores