Skip to content

Conversation

@dnskr
Copy link
Contributor

@dnskr dnskr commented Aug 3, 2024

🔍 Description

Issue References 🔗

This pull request fixes #6565

Describe Your Solution 🔧

  • Fixed misleading kyuubi.metrics properties in charts/kyuubi/templates/kyuubi-configmap.yaml.
  • Fixed condition to create PodMonitor, ServiceMonitor and PrometheusRule.
  • Move metrics related properties to metrics property tree.

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Disabled metrics - prometheus port is not renderred

helm template kyuubi charts/kyuubi --set metrics.enabled=false \
                                   -s templates/kyuubi-statefulset.yaml \
                                   -s templates/kyuubi-headless-service.yaml

JMX reporter only - prometheus port is not renderred

helm template kyuubi charts/kyuubi --set metrics.reporters="JMX" \
                                   -s templates/kyuubi-statefulset.yaml \
                                   -s templates/kyuubi-headless-service.yaml

Default properties - prometheus port is renderred in StatefulSet and Headless Service

helm template kyuubi charts/kyuubi -s templates/kyuubi-statefulset.yaml \
                                   -s templates/kyuubi-headless-service.yaml

Default properties - PodMonitor is not renderred

helm template kyuubi charts/kyuubi -s templates/kyuubi-podmonitor.yaml

Default properties - ServiceMonitor is not renderred

helm template kyuubi charts/kyuubi -s templates/kyuubi-servicemonitor.yaml

Default properties - PrometheusRule is not renderred

helm template kyuubi charts/kyuubi -s templates/kyuubi-alert.yaml

Enabled metrics, PodMonitor, ServiceMonitor, PrometheusRule, PROMETHEUS reporter and port set to 9999

helm template kyuubi charts/kyuubi --set metrics.enabled=true \
                                   --set metrics.reporters="PROMETHEUS\, JMX" \
                                   --set metrics.prometheusPort=9999 \
                                   --set metrics.podMonitor.enabled=true \
                                   --set metrics.serviceMonitor.enabled=true \
                                   --set metrics.prometheusRule.enabled=true \
                                   -s templates/kyuubi-statefulset.yaml \
                                   -s templates/kyuubi-headless-service.yaml \
                                   -s templates/kyuubi-configmap.yaml \
                                   -s templates/kyuubi-podmonitor.yaml \
                                   -s templates/kyuubi-servicemonitor.yaml \
                                   -s templates/kyuubi-alert.yaml

Install the chart and test Prometheus endpoint

helm install kyuubi charts/kyuubi --set metrics.enabled=true \
                                  --set metrics.reporters="PROMETHEUS\, JMX" \
                                  --set metrics.prometheusPort=9999
...
kyuubi@kyuubi-0:/opt/kyuubi$ curl 127.0.0.1:9999/metrics
# HELP kyuubi_buffer_pool_mapped_count Generated from Dropwizard metric import (metric=kyuubi.buffer_pool.mapped.count, type=com.codahale.metrics.jvm.JmxAttributeGauge)
# TYPE kyuubi_buffer_pool_mapped_count gauge
kyuubi_buffer_pool_mapped_count 0.0
# HELP kyuubi_gc_MarkSweepCompact_time Generated from Dropwizard metric import (metric=kyuubi.gc.MarkSweepCompact.time, type=com.codahale.metrics.jvm.GarbageCollectorMetricSet$$Lambda$227/1493158871)
# TYPE kyuubi_gc_MarkSweepCompact_time gauge
kyuubi_gc_MarkSweepCompact_time 91.0
...

Checklist 📝

Be nice. Be informative.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (2a990be) to head (1f20cab).

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #6580   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         677     677           
  Lines       41811   41811           
  Branches     5712    5712           
======================================
  Misses      41811   41811           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me

@dnskr dnskr self-assigned this Aug 6, 2024
@dnskr dnskr added this to the v1.10.0 milestone Aug 6, 2024
@dnskr dnskr closed this in 44fcf28 Aug 6, 2024
@dnskr
Copy link
Contributor Author

dnskr commented Aug 6, 2024

Thanks for reviews, merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement] [Helm] Monitoring configuration

4 participants