feat/teams-integration #40

wjro · 2025-05-26T15:37:08Z

wjro commented

2025-05-26 15:37:08 +00:00

No description provided.

wjro added 3 commits 2025-05-26 15:37:09 +00:00

feat: microsoft teams integration 77eb1228be

wip:using types to serialze to json and traits/impl to build alert channel routes for prometheus alert manager f94c899bf7

refactor: used AlertEndPoint trait and impl, built custom types for prometheus alert manager b5c6e1c99d

johnride requested changes 2025-05-26 16:01:57 +00:00

johnride left a comment

You missed the point of using a trait here. See my comment and refactor accordingly.

harmony/src/modules/monitoring/mod.rs Outdated

						
				@@ -5,0 +9,4 @@

				mod prometheus_msteams;

				#[derive(Debug, Clone, Serialize)]

				pub enum AlertChannel {

johnride commented

2025-05-26 16:00:20 +00:00

This defeats the purpose of using a trait. Also prevents extensibility by external users of Harmony.

If this were a trait, then anybody can impl AlertChannel for MyCustomMessageReceiver and use this AlertChannel in his Harmony deployment.

This defeats the purpose of using a trait. Also prevents extensibility by external users of Harmony. If this were a trait, then anybody can `impl AlertChannel for MyCustomMessageReceiver` and use this AlertChannel in his Harmony deployment.

wjro added 2 commits 2025-05-28 16:44:04 +00:00

refactor: Prometheus alert channel configuration for extensibility to respect OpenClosed, this makes adding new alert channel types easier, requiring only a new trait implementation without modifying core logic 78aadadd22

feat: added an example of adding an msteams channel f96572848a

johnride requested changes 2025-05-29 00:04:27 +00:00

johnride left a comment

Much better than previous iteration. But still a lot of room for improvement.

First : this should be split into 4-5 pull requests :

Propose the PrometheusAlertChannel trait with appropriate rustdoc in an individual p-r. This comes with the integration in the PrometheusScore and all the logic required to make it work. This is important to make sure we got the core logic right first.
Propose the typetag dependency with a rationale explaining why it is required here and the refactoring of other places where we used to do things differently (this one I think would be rejected)
Each PrometheusAlertChannel implementation should have its own, dedicated p-r
The msteams example deserves its own p-r too

Then, I have quite a few more comments that you will see below.

Much better than previous iteration. But still a lot of room for improvement. First : this should be split into 4-5 pull requests : 1. Propose the PrometheusAlertChannel trait with appropriate rustdoc in an individual p-r. This comes with the integration in the PrometheusScore and all the logic required to make it work. This is important to make sure we got the core logic right first. 2. Propose the typetag dependency with a rationale explaining why it is required here and the refactoring of other places where we used to do things differently (this one I think would be rejected) 3. Each PrometheusAlertChannel implementation should have its own, dedicated p-r 4. The msteams example deserves its own p-r too Then, I have quite a few more comments that you will see below.

examples/ms_teams_alert_channel/Cargo.toml

						
				@@ -0,0 +1,14 @@

				[package]

johnride commented

2025-05-28 23:22:33 +00:00

This should be behind a feature flag such as "msteams-deprecated"

johnride commented

2025-05-29 00:11:50 +00:00

Oh it's in the examples, forget this.

johnride marked this conversation as resolved

examples/ms_teams_alert_channel/src/main.rs

						
				@@ -0,0 +38,4 @@

				    webhook_url: Url,

				}

				#[typetag::serde]

johnride commented

2025-05-28 23:25:21 +00:00

typetag doesn't seem to be a good idea here. Why do you use it? Show me an actual use case that requires it versus having a trait bound + Serialize on the PrometheusAlertChannel trait.

typetag doesn't seem to be a good idea here. Why do you use it? Show me an actual use case that requires it versus having a trait bound `+ Serialize` on the PrometheusAlertChannel trait.

harmony/src/modules/monitoring/kube_prometheus/prometheus_alert_channel.rs

						
				@@ -0,0 +16,4 @@

				use std::fmt::Debug;

				use url::Url;

				#[typetag::serde(tag = "channel_type")]

johnride commented

2025-05-28 23:29:57 +00:00

Yeah I don't think this will work. Show me the actual json that this produces and why you need to use this.

Always be extremely careful when introducing a new dependency in the project. If we need a new dependency it can mean two things :

There is a new pattern / use case that emerged. This might be the case here but I don't think so, we already have a pattern for serializable traits that is binding the + Serialize requirement to the trait like we do with the Score trait.
We did something wrong in the rest of the codebase and introducing the dependency means that we must refactor every other place in the codebase that does this thing wrong

Yeah I don't think this will work. Show me the actual json that this produces and why you need to use this. Always be extremely careful when introducing a new dependency in the project. If we need a new dependency it can mean two things : 1. There is a new pattern / use case that emerged. This might be the case here but I don't think so, we already have a pattern for serializable traits that is binding the `+ Serialize` requirement to the trait like we do with the Score trait. 2. We did something wrong in the rest of the codebase and introducing the dependency means that we must refactor every other place in the codebase that does this thing wrong

harmony/src/modules/monitoring/kube_prometheus/prometheus_alert_channel.rs

						
				@@ -0,0 +18,4 @@

				#[typetag::serde(tag = "channel_type")]

				#[async_trait::async_trait]

				pub trait PrometheusAlertChannel: DynClone + Debug + Send + Sync {

johnride commented

2025-05-28 23:59:43 +00:00

I think PrometheusAlertChannel is a good name for the trait. However both function names feel off :

get_alert_manager_config_contribution : what does this mean? What is the word contribution doing here? I feel like the return type is correct or almost correct but the function name is weird. From my understanding here this trait's job is to provide a serializable alert channel yaml configuration that can be insterted in a list of prometheus alert channels.

get_dependency_score : this feels very wrong. I'm pretty sure this violates the Single Responsibility Principle as well as Harmony's architecture guidelines.

Please add a rustdoc explaining what is the purpose of this trait. Then review the SRP and make sure that it is followed here.

I think PrometheusAlertChannel is a good name for the trait. However both function names feel off : get_alert_manager_config_contribution : what does this mean? What is the word `contribution` doing here? I feel like the return type is correct or almost correct but the function name is weird. From my understanding here this trait's job is to provide a serializable alert channel yaml configuration that can be insterted in a list of prometheus alert channels. get_dependency_score : this feels very wrong. I'm pretty sure this violates the Single Responsibility Principle as well as Harmony's architecture guidelines. Please add a rustdoc explaining what is the purpose of this trait. Then review the SRP and make sure that it is followed here.

harmony/src/modules/monitoring/kube_prometheus/types.rs

						
				@@ -0,0 +1,94 @@

				use serde::{Deserialize, Serialize};

johnride commented

2025-05-28 23:36:50 +00:00

This entire file looks pretty good 👍

Almost all types seem to describe the proper abstractions of prometheus types.

However, the implementation specific types should be bundled with their respective implementations. Such as SlackConfig and WebhookConfig.

This entire file looks pretty good 👍 Almost all types seem to describe the proper abstractions of prometheus types. However, the implementation specific types should be bundled with their respective implementations. Such as SlackConfig and WebhookConfig.

harmony/src/modules/monitoring/monitoring_alerting.rs

						
				@@ -74,1 +65,4 @@

				            config.namespace = ns.clone();

				        };

				        let null_channel = NullReceiver::new();

johnride commented

2025-05-28 23:38:40 +00:00

Is this really required? This feels wrong, at least it would deserve a small comment explaining why you are ALWAYS instanciating a hardcoded NullReceiver

harmony/src/modules/monitoring/monitoring_alerting.rs

						
				@@ -77,0 +87,4 @@

				        let alert_manager_config = AlertManagerConfig {

				            global: global_config,

				            route: AlertManagerRoute {

johnride commented

2025-05-28 23:41:32 +00:00

Why is all this config hardcoded? Shouldn't it be exposed in the Score's configuration? I think it should have defaults so that users are not forced to set everything by themselves but it should not be hardcoded.

harmony/src/modules/monitoring/monitoring_alerting.rs

						
				@@ -91,3 +122,3 @@

				    }

				    async fn deploy_alert_channel_service<T: Topology + HelmCommand>(

				    async fn deploy_alert_channel_dependencies<T: Topology + HelmCommand>(

johnride commented

2025-05-28 23:58:05 +00:00

I am 99% sure this is wrong.

Dependencies should be handled at the Topology level as capabilities. But I'm not 100% sure because this has the drawback of not being "just in time" when the score is executed. This would be happening right at the Topology's initialization.

However, this implementation here has many drawbacks :

The PrometheusAlertChannel is handling dependencies in a unique way, that is not following the rest of Harmony's architecture.

Take the K8sAnywhereTopology for example : K8sResourceScore has a K8sClient dependency, which is provided by K8sAnywhere. Then, upon initialization, K8sAnywhere will make sure that it is fulfilling is contract by making sure that there is a K8sClient available, either by installing K3d locally or verifying that it is able to reach whatever cluster it is connected to.

This means that I would have expected this architecture :

trait PrometheusAlertChannel : Serialize {
  fn get_config(&self) -> Option<AlertChannelConfig>;
}

struct PrometheusAlertChannelInterpret {
  channel_config: AlertChannelConfig
}

#[derive(Serialize)]
struct DiscordAlertChannel<T: DiscordAlertSender> { // Use the generic here to specify the capability required by the DiscordAlertChannel. Then, in turn the PrometheusScore will be built with a `T` bound which will be the Topology in the end, and this Topology will be forced to implement the correct capability. Then it is guaranteed that when you deploy a DiscordAlertChannel on a given topology, this topology will manage the dependency.
//
// There might be some compilation detail I'm missing here, but this would be a lot more robust than the current implementation

}

impl PrometheusAlertChannel for DiscordAlertChannel {
 // TODO
}

I am 99% sure this is wrong. Dependencies should be handled at the Topology level as capabilities. But I'm not 100% sure because this has the drawback of not being "just in time" when the score is executed. This would be happening right at the Topology's initialization. However, this implementation here has many drawbacks : The PrometheusAlertChannel is handling dependencies in a unique way, that is not following the rest of Harmony's architecture. Take the K8sAnywhereTopology for example : K8sResourceScore has a K8sClient dependency, which is provided by K8sAnywhere. Then, upon initialization, K8sAnywhere will make sure that it is fulfilling is contract by making sure that there is a K8sClient available, either by installing K3d locally or verifying that it is able to reach whatever cluster it is connected to. --- This means that I would have expected this architecture : ```rust trait PrometheusAlertChannel : Serialize { fn get_config(&self) -> Option<AlertChannelConfig>; } struct PrometheusAlertChannelInterpret { channel_config: AlertChannelConfig } #[derive(Serialize)] struct DiscordAlertChannel<T: DiscordAlertSender> { // Use the generic here to specify the capability required by the DiscordAlertChannel. Then, in turn the PrometheusScore will be built with a `T` bound which will be the Topology in the end, and this Topology will be forced to implement the correct capability. Then it is guaranteed that when you deploy a DiscordAlertChannel on a given topology, this topology will manage the dependency. // // There might be some compilation detail I'm missing here, but this would be a lot more robust than the current implementation } impl PrometheusAlertChannel for DiscordAlertChannel { // TODO }

johnride closed this pull request

2025-06-10 13:27:23 +00:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#40

				`@@ -0,0 +1,14 @@`
				`[package]`

				`@@ -0,0 +1,94 @@`
				`use serde::{Deserialize, Serialize};`