Skip to content

Commit 2c523de

Browse files
parmesantparmesant
committed
updates: coderabbit suggestions and bugfixes
1 parent 15d8fa8 commit 2c523de

File tree

6 files changed

+229
-103
lines changed

6 files changed

+229
-103
lines changed

‎src/alerts/alert_enums.rs

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,10 @@
1616
*
1717
*/
1818

19-
use std::{
20-
fmt::{self, Display},
21-
str::FromStr,
22-
};
19+
use std::fmt::{self, Display};
2320

2421
use chrono::{DateTime, Utc};
2522
use derive_more::derive::FromStr;
26-
use serde::ser::Error;
2723
use ulid::Ulid;
2824

2925
use crate::alerts::{
@@ -272,10 +268,8 @@ impl Display for NotificationState {
272268
NotificationState::Notify => write!(f, "notify"),
273269
NotificationState::Mute(till_time) => {
274270
let till = match till_time.as_str() {
275-
"indefinite" => DateTime::<Utc>::MAX_UTC.to_rfc3339(),
276-
_ => DateTime::<Utc>::from_str(till_time)
277-
.map_err(|e| std::fmt::Error::custom(e.to_string()))?
278-
.to_rfc3339(),
271+
"indefinite" => &DateTime::<Utc>::MAX_UTC.to_rfc3339(),
272+
_ => till_time,
279273
};
280274
write!(f, "{till}")
281275
}

‎src/alerts/alert_structs.rs

Lines changed: 100 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,9 @@ pub struct AlertRequest {
259259
pub severity: Severity,
260260
pub title: String,
261261
pub query: String,
262-
pub alert_type: AlertType,
262+
pub alert_type: String,
263+
pub anomaly_config: Option<AnomalyConfig>,
264+
pub forecast_config: Option<ForecastConfig>,
263265
pub threshold_config: ThresholdConfig,
264266
#[serde(default)]
265267
pub notification_config: NotificationConfig,
@@ -282,7 +284,30 @@ impl AlertRequest {
282284
title: self.title,
283285
query: self.query,
284286
datasets,
285-
alert_type: self.alert_type,
287+
alert_type: {
288+
match self.alert_type.as_str() {
289+
"anomaly" => {
290+
if let Some(conf) = self.anomaly_config {
291+
AlertType::Anomaly(conf)
292+
} else {
293+
return Err(AlertError::Metadata(
294+
"anomalyConfig is required for anomaly type alerts",
295+
));
296+
}
297+
}
298+
"forecast" => {
299+
if let Some(conf) = self.forecast_config {
300+
AlertType::Forecast(conf)
301+
} else {
302+
return Err(AlertError::Metadata(
303+
"forecastConfig is required for forecast type alerts",
304+
));
305+
}
306+
}
307+
"threshold" => AlertType::Threshold,
308+
_ => return Err(AlertError::Metadata("Invalid alert type provided")),
309+
}
310+
},
286311
threshold_config: self.threshold_config,
287312
eval_config: self.eval_config,
288313
targets: self.targets,
@@ -319,12 +344,78 @@ pub struct AlertConfig {
319344
pub tags: Option<Vec<String>>,
320345
}
321346

347+
#[derive(Debug, serde::Serialize, serde::Deserialize, Clone)]
348+
#[serde(rename_all = "camelCase")]
349+
pub struct AlertConfigResponse {
350+
pub version: AlertVersion,
351+
#[serde(default)]
352+
pub id: Ulid,
353+
pub severity: Severity,
354+
pub title: String,
355+
pub query: String,
356+
pub datasets: Vec<String>,
357+
pub alert_type: &'static str,
358+
pub anomaly_config: Option<AnomalyConfig>,
359+
pub forecast_config: Option<ForecastConfig>,
360+
pub threshold_config: ThresholdConfig,
361+
pub eval_config: EvalConfig,
362+
pub targets: Vec<Ulid>,
363+
// for new alerts, state should be resolved
364+
#[serde(default)]
365+
pub state: AlertState,
366+
pub notification_state: NotificationState,
367+
pub notification_config: NotificationConfig,
368+
pub created: DateTime<Utc>,
369+
pub tags: Option<Vec<String>>,
370+
}
371+
372+
impl AlertConfig {
373+
pub fn to_response(self) -> AlertConfigResponse {
374+
AlertConfigResponse {
375+
version: self.version,
376+
id: self.id,
377+
severity: self.severity,
378+
title: self.title,
379+
query: self.query,
380+
datasets: self.datasets,
381+
alert_type: {
382+
match self.alert_type {
383+
AlertType::Threshold => "threshold",
384+
AlertType::Anomaly(_) => "anomaly",
385+
AlertType::Forecast(_) => "forecast",
386+
}
387+
},
388+
anomaly_config: {
389+
match &self.alert_type {
390+
AlertType::Anomaly(conf) => Some(conf.clone()),
391+
_ => None,
392+
}
393+
},
394+
forecast_config: {
395+
match self.alert_type {
396+
AlertType::Forecast(conf) => Some(conf),
397+
_ => None,
398+
}
399+
},
400+
threshold_config: self.threshold_config,
401+
eval_config: self.eval_config,
402+
targets: self.targets,
403+
state: self.state,
404+
notification_state: self.notification_state,
405+
notification_config: self.notification_config,
406+
created: self.created,
407+
tags: self.tags,
408+
}
409+
}
410+
}
411+
322412
#[derive(Debug, Serialize)]
323413
pub struct AlertsSummary {
324414
pub total: u64,
325415
pub triggered: AlertsInfoByState,
326-
pub paused: AlertsInfoByState,
327-
pub resolved: AlertsInfoByState,
416+
pub disabled: AlertsInfoByState,
417+
#[serde(rename = "not-triggered")]
418+
pub not_triggered: AlertsInfoByState,
328419
}
329420

330421
#[derive(Debug, Serialize)]
@@ -381,3 +472,8 @@ impl AlertQueryResult {
381472
}
382473
}
383474
}
475+
476+
#[derive(serde::Deserialize)]
477+
pub struct NotificationStateRequest {
478+
pub state: String,
479+
}

‎src/alerts/alerts_utils.rs

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -267,13 +267,18 @@ async fn update_alert_state(
267267
alert: &dyn AlertTrait,
268268
message: Option<String>,
269269
) -> Result<(), AlertError> {
270-
let guard = ALERTS.write().await;
271-
let alerts = if let Some(alerts) = guard.as_ref() {
272-
alerts
273-
} else {
274-
return Err(AlertError::CustomError("No AlertManager set".into()));
270+
// Get the alert manager reference while holding the lock briefly
271+
let alerts = {
272+
let guard = ALERTS.read().await;
273+
if let Some(alerts) = guard.as_ref() {
274+
alerts.clone()
275+
} else {
276+
return Err(AlertError::CustomError("No AlertManager set".into()));
277+
}
278+
// Lock is released here
275279
};
276280

281+
// Now perform the state update without holding the ALERTS lock
277282
if let Some(msg) = message {
278283
alerts
279284
.update_state(*alert.get_id(), AlertState::Triggered, Some(msg))

‎src/alerts/mod.rs

Lines changed: 75 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -567,21 +567,6 @@ impl AlertConfig {
567567
}
568568
}
569569

570-
pub async fn modify(&mut self, alert: AlertRequest) -> Result<(), AlertError> {
571-
// Validate that all target IDs exist
572-
for id in &alert.targets {
573-
TARGETS.get_target_by_id(id).await?;
574-
}
575-
self.title = alert.title;
576-
self.query = alert.query;
577-
self.alert_type = alert.alert_type;
578-
self.threshold_config = alert.threshold_config;
579-
self.eval_config = alert.eval_config;
580-
self.targets = alert.targets;
581-
self.state = AlertState::default();
582-
Ok(())
583-
}
584-
585570
pub fn get_eval_frequency(&self) -> u64 {
586571
match &self.eval_config {
587572
EvalConfig::RollingWindow(rolling_window) => rolling_window.eval_frequency,
@@ -1048,29 +1033,65 @@ impl AlertManagerTrait for Alerts {
10481033
session: SessionKey,
10491034
tags: Vec<String>,
10501035
) -> Result<Vec<AlertConfig>, AlertError> {
1051-
let mut alerts: Vec<AlertConfig> = Vec::new();
1052-
for (_, alert) in self.alerts.read().await.iter() {
1053-
// filter based on whether the user can execute this query or not
1054-
if user_auth_for_query(&session, alert.get_query())
1036+
// First, collect all alerts without performing auth checks to avoid holding the lock
1037+
let all_alerts: Vec<AlertConfig> = {
1038+
let alerts_guard = self.alerts.read().await;
1039+
alerts_guard
1040+
.values()
1041+
.map(|alert| alert.to_alert_config())
1042+
.collect()
1043+
};
1044+
// Lock is released here, now perform expensive auth checks
1045+
1046+
let authorized_alerts = if tags.is_empty() {
1047+
// Parallelize authorization checks
1048+
let futures: Vec<_> = all_alerts
1049+
.into_iter()
1050+
.map(|alert| async {
1051+
if user_auth_for_query(&session.clone(), &alert.query)
1052+
.await
1053+
.is_ok()
1054+
{
1055+
Some(alert)
1056+
} else {
1057+
None
1058+
}
1059+
})
1060+
.collect();
1061+
1062+
futures::future::join_all(futures)
10551063
.await
1056-
.is_ok()
1057-
{
1058-
alerts.push(alert.to_alert_config());
1059-
}
1060-
}
1061-
if tags.is_empty() {
1062-
return Ok(alerts);
1063-
}
1064-
// filter alerts based on tags
1065-
alerts.retain(|alert| {
1066-
if let Some(alert_tags) = &alert.tags {
1067-
alert_tags.iter().any(|tag| tags.contains(tag))
1068-
} else {
1069-
false
1070-
}
1071-
});
1064+
.into_iter()
1065+
.flatten()
1066+
.collect()
1067+
} else {
1068+
// Parallelize authorization checks and then filter by tags
1069+
let futures: Vec<_> = all_alerts
1070+
.into_iter()
1071+
.map(|alert| async {
1072+
if user_auth_for_query(&session, &alert.query).await.is_ok() {
1073+
Some(alert)
1074+
} else {
1075+
None
1076+
}
1077+
})
1078+
.collect();
1079+
1080+
futures::future::join_all(futures)
1081+
.await
1082+
.into_iter()
1083+
.flatten()
1084+
.filter(|alert| {
1085+
if let Some(alert_tags) = &alert.tags {
1086+
alert_tags.iter().any(|tag| tags.contains(tag))
1087+
} else {
1088+
false
1089+
}
1090+
})
1091+
.collect()
1092+
};
10721093

1073-
Ok(alerts)
1094+
Ok(authorized_alerts)
10741095
}
10751096

10761097
/// Returns a single alert that the user has access to (based on query auth)
@@ -1258,11 +1279,11 @@ pub async fn get_alerts_summary() -> Result<AlertsSummary, AlertError> {
12581279
let total = alerts.len() as u64;
12591280

12601281
let mut triggered = 0;
1261-
let mut resolved = 0;
1262-
let mut paused = 0;
1282+
let mut not_triggered = 0;
1283+
let mut disabled = 0;
12631284
let mut triggered_alerts: Vec<AlertsInfo> = Vec::new();
1264-
let mut paused_alerts: Vec<AlertsInfo> = Vec::new();
1265-
let mut resolved_alerts: Vec<AlertsInfo> = Vec::new();
1285+
let mut disabled_alerts: Vec<AlertsInfo> = Vec::new();
1286+
let mut not_triggered_alerts: Vec<AlertsInfo> = Vec::new();
12661287

12671288
// find total alerts for each state
12681289
// get title, id and state of each alert for that state
@@ -1277,16 +1298,16 @@ pub async fn get_alerts_summary() -> Result<AlertsSummary, AlertError> {
12771298
});
12781299
}
12791300
AlertState::Disabled => {
1280-
paused += 1;
1281-
paused_alerts.push(AlertsInfo {
1301+
disabled += 1;
1302+
disabled_alerts.push(AlertsInfo {
12821303
title: alert.get_title().to_string(),
12831304
id: *alert.get_id(),
12841305
severity: alert.get_severity().clone(),
12851306
});
12861307
}
12871308
AlertState::NotTriggered => {
1288-
resolved += 1;
1289-
resolved_alerts.push(AlertsInfo {
1309+
not_triggered += 1;
1310+
not_triggered_alerts.push(AlertsInfo {
12901311
title: alert.get_title().to_string(),
12911312
id: *alert.get_id(),
12921313
severity: alert.get_severity().clone(),
@@ -1299,25 +1320,25 @@ pub async fn get_alerts_summary() -> Result<AlertsSummary, AlertError> {
12991320
triggered_alerts.sort_by_key(|alert| get_severity_priority(&alert.severity));
13001321
triggered_alerts.truncate(5);
13011322

1302-
paused_alerts.sort_by_key(|alert| get_severity_priority(&alert.severity));
1303-
paused_alerts.truncate(5);
1323+
disabled_alerts.sort_by_key(|alert| get_severity_priority(&alert.severity));
1324+
disabled_alerts.truncate(5);
13041325

1305-
resolved_alerts.sort_by_key(|alert| get_severity_priority(&alert.severity));
1306-
resolved_alerts.truncate(5);
1326+
not_triggered_alerts.sort_by_key(|alert| get_severity_priority(&alert.severity));
1327+
not_triggered_alerts.truncate(5);
13071328

13081329
let alert_summary = AlertsSummary {
13091330
total,
13101331
triggered: AlertsInfoByState {
13111332
total: triggered,
13121333
alert_info: triggered_alerts,
13131334
},
1314-
paused: AlertsInfoByState {
1315-
total: paused,
1316-
alert_info: paused_alerts,
1335+
disabled: AlertsInfoByState {
1336+
total: disabled,
1337+
alert_info: disabled_alerts,
13171338
},
1318-
resolved: AlertsInfoByState {
1319-
total: resolved,
1320-
alert_info: resolved_alerts,
1339+
not_triggered: AlertsInfoByState {
1340+
total: not_triggered,
1341+
alert_info: not_triggered_alerts,
13211342
},
13221343
};
13231344
Ok(alert_summary)

‎src/alerts/target.rs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -564,21 +564,21 @@ impl CallableTarget for AlertManager {
564564
match payload.alert_info.alert_state {
565565
AlertState::Triggered => alert["labels"]["status"] = "triggered".into(),
566566
AlertState::NotTriggered => {
567-
alert["labels"]["status"] = "resolved".into();
567+
alert["labels"]["status"] = "not-triggered".into();
568568
alert["annotations"]["reason"] =
569569
serde_json::Value::String(payload.default_resolved_string());
570570
alert["endsAt"] = Utc::now()
571571
.to_rfc3339_opts(chrono::SecondsFormat::Millis, true)
572572
.into();
573573
}
574-
AlertState::Disabled => alert["labels"]["status"] = "paused".into(), // AlertState::Silenced => {
575-
// alert["labels"]["status"] = "silenced".into();
576-
// alert["annotations"]["reason"] =
577-
// serde_json::Value::String(payload.default_silenced_string());
578-
// // alert["endsAt"] = Utc::now()
579-
// // .to_rfc3339_opts(chrono::SecondsFormat::Millis, true)
580-
// // .into();
581-
// }
574+
AlertState::Disabled => alert["labels"]["status"] = "disabled".into(), // AlertState::Silenced => {
575+
// alert["labels"]["status"] = "silenced".into();
576+
// alert["annotations"]["reason"] =
577+
// serde_json::Value::String(payload.default_silenced_string());
578+
// // alert["endsAt"] = Utc::now()
579+
// // .to_rfc3339_opts(chrono::SecondsFormat::Millis, true)
580+
// // .into();
581+
// }
582582
};
583583

584584
if let Err(e) = client

0 commit comments

Comments
 (0)