NationTech/harmony
6
0
Fork 1
Code Issues 32 Pull Requests 7 Actions 7 Packages Projects Releases 1 Wiki Activity

monitoringalerting #37

Merged
wjro merged 6 commits from monitoringalerting into master 2025-05-21 17:32:27 +00:00
Conversation 0 Commits 6 Files Changed 8 +416 -31
wjro commented 2025-05-15 19:23:40 +00:00
Owner
Copy Link

WIP: Something here feels off to me, the config fields for which services are deployed through via the helm chart are not easily modifiable. Since harmony is opinionated I,m not sure how much of a problem that is. Do we want the end user to have the liberty/responsibility to chose which services are deployed by default or not

WIP: Something here feels off to me, the config fields for which services are deployed through via the helm chart are not easily modifiable. Since harmony is opinionated I,m not sure how much of a problem that is. Do we want the end user to have the liberty/responsibility to chose which services are deployed by default or not
wjro added 2 commits 2025-05-15 19:23:41 +00:00
wjro added 1 commit 2025-05-15 19:31:32 +00:00
johnride requested changes 2025-05-15 20:33:30 +00:00
johnride left a comment
Owner
Copy Link

Good improvement over the first version, still some work to do to have it really look good. Keep it up!

Good improvement over the first version, still some work to do to have it really look good. Keep it up!
harmony/src/modules/monitoring/kube_prometheus.rs Outdated
@ -34,1 +34,4 @@
"#;
let mut values_overrides: HashMap<NonBlankString, String> = HashMap::new();
macro_rules! insert_flag {
johnride commented 2025-05-15 19:41:50 +00:00
Owner
Copy Link

Using a macro here is weird at the very least.

It makes code not easily readable insert_flag is way less obvious to me what it does than values_overrides.insert(...) would

Use a function if you want to make the NonBlankString boilerplate more readable. And if you want to write a macro that does something valuable you should validate the NonBlankString at compile time like we do with out ip! and mac_address! macros.

Using a macro here is weird at the very least. It makes code not easily readable `insert_flag` is way less obvious to me what it does than `values_overrides.insert(...)` would Use a function if you want to make the NonBlankString boilerplate more readable. And if you want to write a macro that does something valuable you should validate the NonBlankString at compile time like we do with out ip! and mac_address! macros.
harmony/src/modules/monitoring/kube_prometheus.rs Outdated
@ -43,3 +75,3 @@
chart_version: None,
values_overrides: None,
values_overrides: Some(values_overrides),
values_yaml: Some(values.to_string()),
johnride commented 2025-05-15 19:54:34 +00:00
Owner
Copy Link

Why use values_overrides here when there already is a values_yaml right above? Would make it a lot more readable I think.

Why use values_overrides here when there already is a values_yaml right above? Would make it a lot more readable I think.
harmony/src/modules/monitoring/monitoring_alerting.rs Outdated
@ -11,0 +12,4 @@
use super::{config::KubePrometheusConfig, kube_prometheus::kube_prometheus_helm_chart_score};
#[derive(Debug, Clone, Serialize)]
pub enum AlertChannel {
johnride commented 2025-05-15 20:00:34 +00:00
Owner
Copy Link

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.
harmony/src/modules/monitoring/monitoring_alerting.rs
@ -11,0 +19,4 @@
},
Smpt {
email_address: EmailAddress,
service_name: String,
johnride commented 2025-05-15 19:43:21 +00:00
Owner
Copy Link

What is service_name used for? As this is user facing, a bit of rust doc (with triple slashes ///) to describe how to use would be useful here.

What is service_name used for? As this is user facing, a bit of rust doc (with triple slashes ///) to describe how to use would be useful here.
johnride marked this conversation as resolved
harmony/src/modules/monitoring/monitoring_alerting.rs
@ -16,2 +34,2 @@
pub monitoring_stack: HelmChartScore,
pub namespace: String,
pub alert_channel: Option<AlertChannel>,
pub monitoring_stack: Stack,
johnride commented 2025-05-15 19:47:52 +00:00
Owner
Copy Link

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.
johnride marked this conversation as resolved
harmony/src/modules/monitoring/monitoring_alerting.rs Outdated
@ -32,3 +47,1 @@
Self {
monitoring_stack: kube_prometheus_score(ns),
namespace: ns.to_string(),
fn match_alert_channel(&self, config: &mut KubePrometheusConfig) {
johnride commented 2025-05-15 20:12:02 +00:00
Owner
Copy Link

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?
harmony/src/modules/monitoring/monitoring_alerting.rs Outdated
@ -35,0 +47,4 @@
fn match_alert_channel(&self, config: &mut KubePrometheusConfig) {
if let Some(alert_channel) = &self.alert_channel {
match alert_channel {
//opt1
johnride commented 2025-05-15 19:50:29 +00:00
Owner
Copy Link

Useless comments

Useless comments
wjro added 2 commits 2025-05-20 19:59:06 +00:00
wjro added 1 commit 2025-05-20 20:05:46 +00:00
johnride approved these changes 2025-05-21 16:34:57 +00:00
johnride left a comment
Owner
Copy Link

The name MonitoringAlertingStackScore bothered me all along but I couldn't figure out why. Now I did :

The word Stack.

It should be MonitoringAlertingScore or TelemetryScore or something like that.

Our goal with this is to provide a functionnality to the app developers. This functionnality is not a specific stack, but only the Monitoring and Alerting features.

And now that I think of it, maybe they should be separate : MonitoringScore and AlertingScore.

Both could depend on the same feature provider under the hood. Could be prometheus grafana, hyperdx, datadog, etc.

Let's merge this as-is for now but I think we have a nice opportunity for a great design here!

The name MonitoringAlertingStackScore bothered me all along but I couldn't figure out why. Now I did : The word Stack. It should be MonitoringAlertingScore or TelemetryScore or something like that. Our goal with this is to provide a functionnality to the app developers. This functionnality is not a specific stack, but only the Monitoring and Alerting features. And now that I think of it, maybe they should be separate : MonitoringScore and AlertingScore. Both could depend on the same feature provider under the hood. Could be prometheus grafana, hyperdx, datadog, etc. Let's merge this as-is for now but I think we have a nice opportunity for a great design here!
wjro merged commit 19bd47a545 into master 2025-05-21 17:32:26 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
johnride
Labels
Clear labels   
Architecture Decision Record

   
Good first issue

   
RFC
No Label
Architecture Decision Record
Good first issue
RFC
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#37
No description provided.
Delete Branch "monitoringalerting"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?