From 0da791ef98b775abd30f61e4a5f4f35f148ea141 Mon Sep 17 00:00:00 2001 From: Vladimir Filonov Date: Wed, 16 Oct 2024 16:07:07 +0400 Subject: [PATCH] fix: When assigning alert to in cident by alert id, get all alerts with same fingerprints (#2208) Co-authored-by: Matvey Kukuy --- keep/api/core/db.py | 36 ++++++++++--- .../versions/2024-10-14-08-34_83c1020be97d.py | 49 +++++++++++------ tests/test_incidents.py | 53 +++++++++++++++++++ 3 files changed, 116 insertions(+), 22 deletions(-) diff --git a/keep/api/core/db.py b/keep/api/core/db.py index 15b3b9dd3..10a00c445 100644 --- a/keep/api/core/db.py +++ b/keep/api/core/db.py @@ -1231,7 +1231,7 @@ def get_last_alerts( if with_incidents: query = query.add_columns(AlertToIncident.incident_id.label("incident")) query = query.outerjoin( - AlertToIncident, + AlertToIncident, and_(AlertToIncident.alert_id == Alert.id, AlertToIncident.deleted_at == NULL_FOR_DELETED_AT), ) @@ -2900,8 +2900,27 @@ def get_future_incidents_by_incident_id( return query.all(), total_count +def get_all_same_alert_ids( + tenant_id: str, + alert_ids: List[str | UUID], + session: Optional[Session] = None +): + with existed_or_new_session(session) as session: + fingerprints_subquery = session.query(Alert.fingerprint).where( + Alert.tenant_id == tenant_id, + col(Alert.id).in_(alert_ids) + ).subquery() + query = session.scalars( + select(Alert.id) + .where( + Alert.tenant_id == tenant_id, + col(Alert.fingerprint).in_(fingerprints_subquery) + )) + return query.all() + + def get_alerts_data_for_incident( - alert_ids: list[str | UUID], session: Optional[Session] = None + alert_ids: List[str | UUID], session: Optional[Session] = None ) -> dict: """ Function to prepare aggregated data for incidents from the given list of alert_ids @@ -2983,7 +3002,10 @@ def add_alerts_to_incident( ) with existed_or_new_session(session) as session: + with session.no_autoflush: + all_alert_ids = get_all_same_alert_ids(tenant_id, alert_ids, session) + # Use a set for faster membership checks existing_alert_ids = set( session.exec( @@ -2991,13 +3013,13 @@ def add_alerts_to_incident( AlertToIncident.deleted_at == NULL_FOR_DELETED_AT, AlertToIncident.tenant_id == tenant_id, AlertToIncident.incident_id == incident.id, - col(AlertToIncident.alert_id).in_(alert_ids), + col(AlertToIncident.alert_id).in_(all_alert_ids), ) ).all() ) new_alert_ids = [ - alert_id for alert_id in alert_ids if alert_id not in existing_alert_ids + alert_id for alert_id in all_alert_ids if alert_id not in existing_alert_ids ] if not new_alert_ids: @@ -3106,6 +3128,8 @@ def remove_alerts_to_incident_by_incident_id( if not incident: return None + all_alert_ids = get_all_same_alert_ids(tenant_id, alert_ids, session) + # Removing alerts-to-incident relation for provided alerts_ids deleted = ( session.query(AlertToIncident) @@ -3113,7 +3137,7 @@ def remove_alerts_to_incident_by_incident_id( AlertToIncident.deleted_at == NULL_FOR_DELETED_AT, AlertToIncident.tenant_id == tenant_id, AlertToIncident.incident_id == incident.id, - col(AlertToIncident.alert_id).in_(alert_ids), + col(AlertToIncident.alert_id).in_(all_alert_ids), ).update({ "deleted_at": datetime.now(datetime.now().astimezone().tzinfo), }) @@ -3121,7 +3145,7 @@ def remove_alerts_to_incident_by_incident_id( session.commit() # Getting aggregated data for incidents for alerts which just was removed - alerts_data_for_incident = get_alerts_data_for_incident(alert_ids, session) + alerts_data_for_incident = get_alerts_data_for_incident(all_alert_ids, session) service_field = get_json_extract_field(session, Alert.event, "service") diff --git a/keep/api/models/db/migrations/versions/2024-10-14-08-34_83c1020be97d.py b/keep/api/models/db/migrations/versions/2024-10-14-08-34_83c1020be97d.py index 01efc9e3b..7df119231 100644 --- a/keep/api/models/db/migrations/versions/2024-10-14-08-34_83c1020be97d.py +++ b/keep/api/models/db/migrations/versions/2024-10-14-08-34_83c1020be97d.py @@ -28,22 +28,34 @@ def drop_and_restore_f_keys(table_name, conn): # Drop all foreign keys for fk in existing_f_keys: - op.drop_constraint(fk['name'], table_name, type_='foreignkey') - print(f"Dropped foreign key: {fk['name']}") + try: + op.drop_constraint(fk['name'], table_name, type_='foreignkey') + print(f"Dropped foreign key: {fk['name']}") + except NotImplementedError as e: + if "No support for ALTER of constraints in SQLite dialect." in str(e): + print("No support for ALTER of constraints in SQLite dialect, constraint should be overriden later so skipping") + else: + raise e try: yield finally: # Restore all foreign keys for fk in existing_f_keys: - op.create_foreign_key( - fk['name'], - table_name, - fk['referred_table'], - fk['constrained_columns'], - fk['referred_columns'], - ondelete=fk['options'].get('ondelete') - ) - print(f"Restored foreign key: {fk['name']}") + try: + op.create_foreign_key( + fk['name'], + table_name, + fk['referred_table'], + fk['constrained_columns'], + fk['referred_columns'], + ondelete=fk['options'].get('ondelete') + ) + print(f"Restored foreign key: {fk['name']}") + except NotImplementedError as e: + if "No support for ALTER of constraints in SQLite dialect." in str(e): + print("No support for ALTER of constraints in SQLite dialect, constraint should be overriden later so skipping") + else: + raise e def upgrade() -> None: @@ -64,11 +76,16 @@ def upgrade() -> None: conn = op.get_bind() with drop_and_restore_f_keys("alerttoincident", conn): - - with op.batch_alter_table("alerttoincident", schema=None) as batch_op: - inspector = inspect(conn) - existing_primary_key = inspector.get_pk_constraint('alerttoincident', schema=None) - batch_op.drop_constraint(existing_primary_key['name'], type_="primary") + try: + with op.batch_alter_table("alerttoincident", schema=None) as batch_op: + inspector = inspect(conn) + existing_primary_key = inspector.get_pk_constraint('alerttoincident', schema=None) + batch_op.drop_constraint(existing_primary_key['name'], type_="primary") + except ValueError as e: + if "Constraint must have a name" in str(e): + print("Constraint must have a name, constraint should be overriden later so skipping") + else: + raise e with op.batch_alter_table("alerttoincident", schema=None) as batch_op: batch_op.create_primary_key( diff --git a/tests/test_incidents.py b/tests/test_incidents.py index 0ffff3d06..5cd78fbd2 100644 --- a/tests/test_incidents.py +++ b/tests/test_incidents.py @@ -372,3 +372,56 @@ def test_incident_metadata(db_session, client, test_app, setup_stress_alerts_no_ assert data["services"] == ["keep", "keep-test", "keep-test-2"] assert "sources" in data assert data["sources"] == ["keep", "keep-test", "keep-test-2"] + + +def test_add_alerts_with_same_fingerprint_to_incident(db_session, create_alert): + create_alert( + "fp1", + AlertStatus.FIRING, + datetime.utcnow(), + {"severity": AlertSeverity.CRITICAL.value}, + ) + create_alert( + f"fp1", + AlertStatus.FIRING, + datetime.utcnow(), + {"severity": AlertSeverity.CRITICAL.value}, + ) + create_alert( + f"fp2", + AlertStatus.FIRING, + datetime.utcnow(), + {"severity": AlertSeverity.CRITICAL.value}, + ) + + db_alerts = db_session.query(Alert).all() + + fp1_alerts = [alert for alert in db_alerts if alert.fingerprint == "fp1"] + fp2_alerts = [alert for alert in db_alerts if alert.fingerprint == "fp2"] + + assert len(db_alerts) == 3 + assert len(fp1_alerts) == 2 + assert len(fp2_alerts) == 1 + + incident = create_incident_from_dict( + SINGLE_TENANT_UUID, {"user_generated_name": "test", "user_summary": "test"} + ) + + assert len(incident.alerts) == 0 + + add_alerts_to_incident_by_incident_id( + SINGLE_TENANT_UUID, incident.id, [fp1_alerts[0].id] + ) + + incident = get_incident_by_id(SINGLE_TENANT_UUID, incident.id) + + assert len(incident.alerts) == 2 + + remove_alerts_to_incident_by_incident_id( + SINGLE_TENANT_UUID, incident.id, [fp1_alerts[0].id] + ) + + incident = get_incident_by_id(SINGLE_TENANT_UUID, incident.id) + + assert len(incident.alerts) == 0 +