[Improvement] [Helm] Monitoring configuration

[dnskr]

Code of Conduct

• I agree to follow this project's Code of Conduct

Search before asking

• I have searched in the issues and found no similar issues.

What would you like to be improved?

Current monitoring configuration in the Helm chart seems to be inconsistent and not convenient to use.

Inconsistency

Property monitoring.prometheus.enabled is used for kyuubi.metrics.enabled which seems to be confusing, because metrics can be enabled without PROMETHEUS reporter. Also using monitoring.prometheus.enabled in PrometheusRule, ServiceMonitor and PodMonitor conditions is redundant, because having PROMETHEUS in metricsReporters is enough for the check.

Bug

The following conditions check if metricsReporters equal to PROMETHEUS:

kyuubi/charts/kyuubi/templates/kyuubi-alert.yaml

Line 18 in 8f37390

{{- if and .Values.monitoring.prometheus.enabled (eq .Values.metricsReporters "PROMETHEUS") .Values.prometheusRule.enabled }}

kyuubi/charts/kyuubi/templates/kyuubi-podmonitor.yaml

Line 18 in 8f37390

{{- if and .Values.monitoring.prometheus.enabled (eq .Values.metricsReporters "PROMETHEUS") .Values.podMonitor.enabled }}

kyuubi/charts/kyuubi/templates/kyuubi-servicemonitor.yaml

Line 18 in 8f37390

{{- if and .Values.monitoring.prometheus.enabled (eq .Values.metricsReporters "PROMETHEUS") .Values.serviceMonitor.enabled }}

However, metricsReporters is used to initialize kyuubi.metrics.reporters which is a comma-separated list for all metrics reporters. I.e. metricsReporters can be also PROMETHEUS,CONSOLE, JMX,PROMETHEUS,CONSOLE etc.
The implementation should consider such cases in the conditions and check if PROMETHEUS included into the comma-separated list.

The command renders PrometheusRule:

helm template kyuubi charts/kyuubi --set prometheusRule.enabled=true --set metricsReporters="PROMETHEUS" --show-only templates/kyuubi-alert.yaml

These commands also should render PrometheusRule:

helm template kyuubi charts/kyuubi --set prometheusRule.enabled=true --set metricsReporters="JMX\,PROMETHEUS" --show-only templates/kyuubi-alert.yaml
helm template kyuubi charts/kyuubi --set prometheusRule.enabled=true --set metricsReporters="PROMETHEUS\,CONSOLE" --show-only templates/kyuubi-alert.yaml
helm template kyuubi charts/kyuubi --set prometheusRule.enabled=true --set metricsReporters="JMX\,PROMETHEUS\,CONSOLE" --show-only templates/kyuubi-alert.yaml

How should we improve?

Fix the bug and change values structure, for instance to the following:

metrics:
  # Use for kyuubi.metrics.enabled value
  enabled: true
  # Use for kyuubi.metrics.reporters value
  reporters: PROMETHEUS
  # Use for kyuubi.metrics.prometheus.port value and for the chart templates
  prometheusPort: 10019
  # Move inside "metrics" tree to keep monitoring related configs together
  podMonitor:
    enabled: false
    ...
  # Move inside "metrics" tree to keep monitoring related configs together
  serviceMonitor:
    enabled: false
    ...
  # Move inside "metrics" tree to keep monitoring related configs together
  prometheusRule:
    enabled: false
    ...

Are you willing to submit PR?

• Yes. I would be willing to submit a PR with guidance from the Kyuubi community to improve.
• No. I cannot submit a PR at this time.