Skip to content

[Improvement] [Helm] Monitoring configuration #6565

Closed
@dnskr

Description

@dnskr

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:

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

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

{{- 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions