add support for Alloy as monitoring agent #62
Conversation
| switch s.config.MonitoringAgent { | ||
| case common.MonitoringAgentPrometheus: | ||
| bundleConfiguration.Apps[common.MonitoringPrometheusAgentAppName] = app{ | ||
| Enabled: s.config.IsMonitored(cluster), |
There was a problem hiding this comment.
I think this Can bé simplified without the Switch with: s.config.IsMonitored(cluster) && common.MonitoringAgentPrometheus == s.config.MonitoringAgent. this would definitely reduce duplication
There was a problem hiding this comment.
If you want to keep the Switch, maybe move the bundleConfiguration.Apps outside thé Switch?
There was a problem hiding this comment.
You could use a function liké enableApp or disabledApp instead?
There was a problem hiding this comment.
This implementation make it clear what is happening each case. I don't see how I can move bundleConfiguration outside the switch. Also I would rather not add a function which would obfuscate the behavior here.
If you feel like making a suggestion do it via github suggestion because I am having a hard time to grasp what you have in mind.
There was a problem hiding this comment.
Having a validation in main.go would be nice and allow to fail early, but here we would still need to handle all possible different values for the logging agent.
Merging the observability and logging operator would require major refactoring so I would rather leave this for later.
There was a problem hiding this comment.
Well if you add validation in main.go then you can change thé metricAgent type to an enum type with only 2 allowed values, alloy and prometheusAgent so you don't need a Switch but an if else right?
There was a problem hiding this comment.
Also, you need to check the current observability bundle app version supports alloy right?
There was a problem hiding this comment.
I added the check to ensure observability bundle support alloy as monitoring agent.
I rather keep the switch as is for now and not add enum, it's safer with enum true, but still we have a case where multiple values are possible and I prefer to handle all cases including when the value is not what's expected.
There was a problem hiding this comment.
Let's go with that for now then :)
| switch s.config.MonitoringAgent { | ||
| case common.MonitoringAgentPrometheus: | ||
| bundleConfiguration.Apps[common.MonitoringPrometheusAgentAppName] = app{ | ||
| Enabled: s.config.IsMonitored(cluster), |
There was a problem hiding this comment.
If you want to keep the Switch, maybe move the bundleConfiguration.Apps outside thé Switch?
| switch s.config.MonitoringAgent { | ||
| case common.MonitoringAgentPrometheus: | ||
| bundleConfiguration.Apps[common.MonitoringPrometheusAgentAppName] = app{ | ||
| Enabled: s.config.IsMonitored(cluster), |
There was a problem hiding this comment.
You could use a function liké enableApp or disabledApp instead?
pkg/common/types.go
Outdated
| @@ -30,6 +30,12 @@ const ( | |||
|
|
|||
| GCPManagedClusterKind = "GCPManagedCluster" | |||
| GCPManagedClusterKindProvider = "gke" | |||
|
|
|||
| MonitoringAgentPrometheus = "prometheus-agent" | |||
There was a problem hiding this comment.
Maybe they should bé in thé monitoring config and not in common?
There was a problem hiding this comment.
I moved this into a new common/monitoring package, I keep this into a separate package to avoid later circular import problem with the upcoming implementation.
|
Can we go with this PR then @giantswarm/team-atlas ? |
Towards: giantswarm/roadmap#3522
This PR add support for Alloy to be used as monitoring agent instead of Prometheus Agent. This is configurable via the
--monitoring-agentflag in the observability-operator.The operator will then disable Prometheus Agent and enable Alloy (the opposite is also possible) app via the observability-bundle config.