feat: add service monitors support to prom #66

taha · 2025-06-25T05:54:34Z

taha commented

2025-06-25 05:54:34 +00:00

No description provided.

johnride reviewed 2025-06-25 19:40:00 +00:00

harmony/src/modules/monitoring/kube_prometheus/helm/kube_prometheus_helm_chart.rs Outdated

						
				@ -155,3 +154,3 @@

				  enabled: {prometheus}

				"#,

				    );

				    let prometheus_config =

johnride commented

2025-06-25 19:38:19 +00:00

Why not use the same pattern as the rest of the file?

taha commented

2025-06-25 20:20:20 +00:00

It's not a primitive, and in order to use it that way, it would need to have Display implemented. I tried.

It's not a primitive, and in order to use it that way, it would need to have `Display` implemented. I tried.

johnride commented

2025-06-26 13:30:05 +00:00

OK that makes sense. Why not implement Display? Or Serialize? I guess the format is not what an intuitive Display would do? But then I think it would be cleaner to name the struct in a way that makes it intuitive for it to Display to the appropriate format, then it can be easily reused somewhere else.

Also this file really deserves some tests to make sure the config output is correct.

OK that makes sense. Why not implement Display? Or Serialize? I guess the format is not what an intuitive Display would do? But then I think it would be cleaner to name the struct in a way that makes it intuitive for it to Display to the appropriate format, then it can be easily reused somewhere else. Also this file really deserves some tests to make sure the config output is correct.

harmony_macros/src/lib.rs Outdated

						
				@ -136,0 +136,4 @@

				/// Verify that a string is a valid http scheme

				/// Panics if not http or https

				#[proc_macro]

				pub fn http_scheme(input: TokenStream) -> TokenStream {

johnride commented

2025-06-25 19:35:37 +00:00

What's the use case for this? I don't see this used in this pr.

taha commented

2025-06-25 20:20:58 +00:00

For the httpScheme field, in endpoints. I hadn't yet pushed the commit that uses it

For the `httpScheme` field, in endpoints. I hadn't yet pushed the commit that uses it

johnride commented

2025-06-26 13:49:07 +00:00

Yeah ok, I see what you did there.

But this is not a good use case for Macros. Macros are a last resort when the type semantics are already correct and we can't rely on the regular rust type system anymore to provide a good UX/DX. In this case a macro outputting a String type prevents us from adding useful functionnality around the "Scheme" idea.

I can very well see us eventually detecting the scheme here and performing automatic endpoint discovery and health checks or something like that, which we cannot do if we keep this as a String. See my commnent above, this should be an enum.

Yeah ok, I see what you did there. But this is not a good use case for Macros. Macros are a last resort when the type semantics are already correct and we can't rely on the regular rust type system anymore to provide a good UX/DX. In this case a macro outputting a String type prevents us from adding useful functionnality around the "Scheme" idea. I can very well see us eventually detecting the scheme here and performing automatic endpoint discovery and health checks or something like that, which we cannot do if we keep this as a `String`. See my commnent above, this should be an enum.

taha marked this conversation as resolved

taha force-pushed monitoring_servicemonitor from 7455b28743 to 160a12dffe

2025-06-25 20:21:51 +00:00

Compare

johnride requested changes 2025-06-26 14:05:42 +00:00

johnride left a comment

The good :
It is quite easy to understand, makes sense and should work overall quite well. It is acceptable to move forward as-is except the macro that should be an enum.

The bad :
The architecture and UX/DX leave a lot to be desired I think. While we can move forward with this for now we need to come up with more cohesive designs to achieve our goals of robustness, automation and usability.

Overall, I let you be the judge of which comment you decide to fix right now, I believe they're all worth considering spending some time exploring them, especially the ones about the ServiceMonitor API.

The good : It is quite easy to understand, makes sense and should work overall quite well. It is acceptable to move forward as-is except the macro that should be an enum. The bad : The architecture and UX/DX leave a lot to be desired I think. While we can move forward with this for now we need to come up with more cohesive designs to achieve our goals of robustness, automation and usability. Overall, I let you be the judge of which comment you decide to fix right now, I believe they're all worth considering spending some time exploring them, especially the ones about the ServiceMonitor API.

examples/monitoring/src/main.rs Outdated

						
				@ -16,0 +34,4 @@

				        relabelings: vec![],

				    };

				    let service_monitor = ServiceMonitor {

johnride commented

2025-06-26 13:58:47 +00:00

That is inconvenient to use. Should be much shorter with something like :

ServiceMonitor {
 name: ...
 target_service: my_application, // This here is exactly what we're trying to achieve with harmony. To make the code more coherent and robust by using proper types instead of labels and strings that come out of nowhere to tie up deployments together
 ..Default::default()
}

There are other possible designs such as ServiceMonitor::for_service(my_service)

I think the key here is to figure out a way to allow users to easily define that their application is a service and then this service (core harmony type) can be used in other modules for stuff like ingress, ssl certificates, monitoring, load balancing, autoscaling, etc.

Same goes for the ServiceMonitorEndpoint above.

That is inconvenient to use. Should be much shorter with something like : ```rust ServiceMonitor { name: ... target_service: my_application, // This here is exactly what we're trying to achieve with harmony. To make the code more coherent and robust by using proper types instead of labels and strings that come out of nowhere to tie up deployments together ..Default::default() } ``` There are other possible designs such as `ServiceMonitor::for_service(my_service)` I think the key here is to figure out a way to allow users to easily define that their application is a service and then this service (core harmony type) can be used in other modules for stuff like ingress, ssl certificates, monitoring, load balancing, autoscaling, etc. Same goes for the ServiceMonitorEndpoint above.

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

						
				@ -41,0 +59,4 @@

				pub struct ServiceMonitorTLSConfig {

				    // ## Path to the CA file

				    // ##

				    pub ca_file: Option<String>,

johnride commented

2025-06-26 13:51:31 +00:00

Not a String, should be a path type of some sort. I think we already handle this somewhere else correctly.

You might have to implement Serialize for it or wrap it into another type that Serializes as a String but that's ok.

Not a String, should be a path type of some sort. I think we already handle this somewhere else correctly. You might have to implement Serialize for it or wrap it into another type that Serializes as a String but that's ok.

johnride marked this conversation as resolved

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

						
				@ -41,0 +99,4 @@

				    // ## HTTP path to scrape for metrics

				    // ##

				    pub path: String,

johnride commented

2025-06-26 13:42:04 +00:00

This should be a specific type that validates the path

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

						
				@ -41,0 +103,4 @@

				    // ## HTTP scheme to use for scraping

				    // ##

				    pub scheme: String,

johnride commented

2025-06-26 13:40:14 +00:00

This should be an enum :

pub enum URLScheme {
 HTTP,
 HTTPS,
 // Maybe others such as :
 FILE,
 FTP,
 OTHER(String), // With this we are both usable with more frequent schemes and extensible 
}

impl Display for URLScheme {
 // TODO
}

This should be an enum : ```rust pub enum URLScheme { HTTP, HTTPS, // Maybe others such as : FILE, FTP, OTHER(String), // With this we are both usable with more frequent schemes and extensible } impl Display for URLScheme { // TODO }

taha marked this conversation as resolved

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

						
				@ -41,0 +133,4 @@

				#[serde(rename_all = "camelCase")]

				pub struct MatchExpression {

				    pub key: String,

				    pub operator: String,

johnride commented

2025-06-26 13:42:39 +00:00

I guess operator is not any string? Probably should be an enum too.

taha marked this conversation as resolved

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

						
				@ -41,0 +155,4 @@

				    // # Service label for use in assembling a job name of the form <label value>-<port>

				    // # If no label is specified, the service name is used.

				    pub job_label: Option<String>,

johnride commented

2025-06-26 13:43:47 +00:00

I think we already do have a Label type somewhere. I think it would be more appropriate here than String. That's true for all the label related fields in this file.

This Label type might not be fully compatible in its current form/place but it is definitely a semantic that we will see very often in various use cases and implementations. I think it is worth for us to maintain a Label type which we can eventually provide very interesting functionnality for such as search, tracking, matching, versionning, etc.

I think we already do have a Label type somewhere. I think it would be more appropriate here than String. That's true for all the label related fields in this file. This Label type might not be fully compatible in its current form/place but it is definitely a semantic that we will see very often in various use cases and implementations. I think it is worth for us to maintain a Label type which we can eventually provide very interesting functionnality for such as search, tracking, matching, versionning, etc.