monitoringalerting #37

Merged
wjro merged 6 commits from monitoringalerting into master 2025-05-21 17:32:27 +00:00
3 changed files with 21 additions and 26 deletions
Showing only changes of commit eb8a8a2e04 - Show all commits

View File

@ -43,8 +43,8 @@ async fn main() {
.await .await
.unwrap(); .unwrap();
let monitoring_stack_score = let mut monitoring_stack_score = MonitoringAlertingStackScore::new();
MonitoringAlertingStackScore::new_with_ns(&lamp_stack.config.namespace); monitoring_stack_score.namespace = Some(lamp_stack.config.namespace.clone());
maestro.register_all(vec![Box::new(lamp_stack), Box::new(monitoring_stack_score)]); maestro.register_all(vec![Box::new(lamp_stack), Box::new(monitoring_stack_score)]);
// Here we bootstrap the CLI, this gives some nice features if you need them // Here we bootstrap the CLI, this gives some nice features if you need them

View File

@ -4,7 +4,7 @@ use std::{collections::HashMap, str::FromStr};
use crate::modules::helm::chart::HelmChartScore; use crate::modules::helm::chart::HelmChartScore;
pub fn kube_prometheus_score(config: &KubePrometheusConfig) -> HelmChartScore { pub fn kube_prometheus_helm_chart_score(config: &KubePrometheusConfig) -> HelmChartScore {
//TODO this should be make into a rule with default formatting that can be easily passed as a vec //TODO this should be make into a rule with default formatting that can be easily passed as a vec
//to the overrides or something leaving the user to deal with formatting here seems bad //to the overrides or something leaving the user to deal with formatting here seems bad
let values = r#" let values = r#"

View File

@ -9,7 +9,7 @@ use crate::{
topology::{HelmCommand, Topology}, topology::{HelmCommand, Topology},
}; };
use super::{config::KubePrometheusConfig, kube_prometheus::kube_prometheus_score}; use super::{config::KubePrometheusConfig, kube_prometheus::kube_prometheus_helm_chart_score};
#[derive(Debug, Clone, Serialize)] #[derive(Debug, Clone, Serialize)]
pub enum AlertChannel { pub enum AlertChannel {

Nice, but did you test smtp? If it's not working yet there should be a // TODO comment or something like that.

I would expect to see server, username, password or something like that here. Then there is a bajillion providers with no standards. Emails are not a simple topic. Maybe we should even only support Webhook for now.

Nice, but did you test smtp? If it's not working yet there should be a // TODO comment or something like that. I would expect to see server, username, password or something like that here. Then there is a bajillion providers with no standards. Emails are not a simple topic. Maybe we should even only support Webhook for now.
@ -33,9 +33,17 @@ pub enum Stack {
pub struct MonitoringAlertingStackScore { pub struct MonitoringAlertingStackScore {
pub alert_channel: Option<AlertChannel>, pub alert_channel: Option<AlertChannel>,
pub monitoring_stack: Stack, pub monitoring_stack: Stack,
johnride marked this conversation as resolved
Review

No need for this monitoring_stack: Stack field for now. This is a case of YAGNI. We support only one type and I don't see in the very short term a use case that would force us.

Ron Jeffries, a co-founder of XP, explained the philosophy: "Always implement things when you actually need them, never when you just foresee that you [will] need them."[8] John Carmack wrote "It is hard for less experienced developers to appreciate how rarely architecting for future requirements / applications turns out net-positive."[9]
https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it


Another note, I'm realising now something that slipped by me yesterday :

I think we should have two scores for monitoring : cluster monitoring and app monitoring. The one we want with LampScore is ApplicationMonitoringScore. It comes with quite a bit of boilerplate but at least it makes sense to deploy it anywhere. Then we also provide a ClusterMonitoringScore. Under the hood they could very well use the same Interpret and work together efficiently.

No need for this `monitoring_stack: Stack` field for now. This is a case of YAGNI. We support only one type and I don't see in the very short term a use case that would force us. > Ron Jeffries, a co-founder of XP, explained the philosophy: "Always implement things when you actually need them, never when you just foresee that you [will] need them."[8] John Carmack wrote "It is hard for less experienced developers to appreciate how rarely architecting for future requirements / applications turns out net-positive."[9] > https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it --- Another note, I'm realising now something that slipped by me yesterday : I think we should have two scores for monitoring : cluster monitoring and app monitoring. The one we want with LampScore is ApplicationMonitoringScore. It comes with quite a bit of boilerplate but at least it makes sense to deploy it anywhere. Then we also provide a ClusterMonitoringScore. Under the hood they could very well use the same Interpret and work together efficiently.
pub namespace: Option<String>,
} }
impl MonitoringAlertingStackScore { impl MonitoringAlertingStackScore {
pub fn new() -> Self {
Self {
alert_channel: None,
monitoring_stack: Stack::KubePrometheusStack,
namespace: None,
}
}
fn match_alert_channel(&self, config: &mut KubePrometheusConfig) { fn match_alert_channel(&self, config: &mut KubePrometheusConfig) {

Poor function name. What's the use of this function?

Something like configure_alerts maybe?

Poor function name. What's the use of this function? Something like `configure_alerts` maybe?
if let Some(alert_channel) = &self.alert_channel { if let Some(alert_channel) = &self.alert_channel {
match alert_channel { match alert_channel {
@ -55,25 +63,13 @@ impl MonitoringAlertingStackScore {
} }
} }
} }
fn build_kube_prometheus_helm_chart_config(&self) -> KubePrometheusConfig {
let mut config = KubePrometheusConfig::new();
self.match_alert_channel(&mut config);
if let Some(ns) = &self.namespace {
config.namespace = ns.clone();
} }
// pub fn new_with_ns(ns: &str) -> Self { config
// let mut config = KubePrometheusConfig::default();
// let namespace = ns.to_string();
// config.namespace = namespace;
// let score = kube_prometheus_score(&config);
// Self {
// alert_channel: None,
// monitoring_stack: Some(Stack::KubePrometheusStack),
// }
// }
//}
impl Default for MonitoringAlertingStackScore {
fn default() -> Self {
Self {
alert_channel: None,
monitoring_stack: Stack::KubePrometheusStack,
}
} }
} }
@ -81,10 +77,9 @@ impl<T: Topology + HelmCommand> Score<T> for MonitoringAlertingStackScore {
fn create_interpret(&self) -> Box<dyn Interpret<T>> { fn create_interpret(&self) -> Box<dyn Interpret<T>> {
match &self.monitoring_stack { match &self.monitoring_stack {
Stack::KubePrometheusStack => { Stack::KubePrometheusStack => {
let mut config = KubePrometheusConfig::new(); let config = self.build_kube_prometheus_helm_chart_config();
self.match_alert_channel(&mut config); let helm_chart = kube_prometheus_helm_chart_score(&config);
let score = kube_prometheus_score(&config); helm_chart.create_interpret()
score.create_interpret()
} }
Stack::OtherStack => { Stack::OtherStack => {
todo!() todo!()