optional metrics resources #683
Conversation
|
One thing that was brought up on the issue that we missed, we should do this same conditional rendering for all metrics components. The service, configMap, HPA, etc. |
|
Have added the checks for |
There was a problem hiding this comment.
I think this should be based on metrics.enabled like it was in the first commit (then changed).
That's because now we'll disable fluentd based on prometheus-operator and that's confusing.
Also to be consistent we should add fluentd.enabled but it doesn't make sense because we use FluentD for everything.
So for logs we have to use logs.enabled. We already have traces.enabled.
I'd suggest to go with metrics.enabled as well.
|
I'm afraid that we cannot disable metrics using prometheus flag, because customers can have their own prometheus and still wants to send metrics. We should go with two flags unfortunately |
|
Good point on existing Prometheus case. It will require two seperate flags. |
|
Please check this out, we should be able to use one flag to rule them all: helm/helm#2111 |
…com/SumoLogic/sumologic-kubernetes-collection into vsinghal-optional-metrics-resources
| @@ -1,3 +1,4 @@ | |||
| {{- if eq .Values.fluentd.metrics.enabled true }} | |||
There was a problem hiding this comment.
I'd rather use a new flag - sumologic.metrics.enabled like we do for traces.
There was a problem hiding this comment.
Or actually both sumologic.metrics.enabled and fluentd.metrics.enabled in this case (as this configmap is for fluentd)
There was a problem hiding this comment.
I used this flag as this is part of fluentd itself. and we already have fluentd.events.enabled as compared to traces which is not part of the fluentd section.
Also I feel having both sumologic.metrics.enabled and fluentd.metrics.enabled will not add any functionality as it can be covered by only one.
Thoughts? cc @frankreno @samjsong
There was a problem hiding this comment.
hmm, I'm not sure I understand the usecase for having both sumologic.metrics.enabled and fluentd.metrics.enabled. I can't think of a case where you would want the fluentd pipeline enabled but not the metrics resources, or vice versa. @perk-sumo can you share your reasoning please?
There was a problem hiding this comment.
I feel we should have sumologic.metrics.enabled instead of fluentd.metrics.enabled, especially if we gonna replace fluentd in the future, but it's meaningful change, so I would stick to the current flag
There was a problem hiding this comment.
The idea is to have global flag that is not attached to the underlying implementation so that we (or the user) can switch the tool easily.
There are couple of usecases:
- disable all the metrics, all the fluentd's, prometheus, configmaps etc - set
sumologic.metrics.enabled=false - enable all the metrics in the default mode:
sumologic.metrics.enabled=true - enable metrics, but use own prometheus:
sumologic.metrics.enabled=trueandprometheus-operator.enabled=false - enable metrics, but use own fluentd for tagging:
sumologic.metrics.enabled=trueandfluentd.metrics.enabled=false - enable metrics, but use experimental OTC for that:
sumologic.metrics.enabled=trueandfluentd.metrics.enabled=false,prometheus-operator.enabled=false,otelcol.metrics.enabled=true
There was a problem hiding this comment.
What @perk-sumo suggestion seems to be the cleanest approach.
| @@ -6,7 +6,7 @@ dependencies: | |||
| - name: prometheus-operator | |||
| version: 8.13.8 | |||
| repository: https://kubernetes-charts.storage.googleapis.com/ | |||
| condition: prometheus-operator.enabled | |||
| condition: sumologic.metrics.enabled,prometheus-operator.enabled | |||
There was a problem hiding this comment.
👍 Please add this into the fluent-bit as well:
condition: sumologic.logs.enabled,fluent-bit.enabled
There was a problem hiding this comment.
LGTM, except @perk-sumo findings
// @perk-sumo suggestions are awesome :O
// Is it the new feature?
Co-authored-by: Marcin Stozek <58700054+perk-sumo@users.noreply.github.com>
Description
Fixes #678
Testing performed