feat(operator): Support ServiceMonitor #619
Conversation
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
WalkthroughThis pull request introduces a new ServiceMonitor resource for the ClickHouse operator. The changes include a new template in the Helm chart to create the ServiceMonitor, which is conditionally deployed based on a flag in the values file. In addition, the values file receives a new section for configuring the ServiceMonitor, with fields for enabling it, specifying labels, and setting scrape parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant H as Helm Engine
participant T as Template Renderer
participant K as Kubernetes API
participant P as Prometheus
U->>H: Initiates Helm install/upgrade
H->>T: Render templates with values.yaml data
alt ServiceMonitor.enabled true
T->>K: Create ServiceMonitor resource
K-->>T: Confirm resource creation
P->>K: Discover ServiceMonitor via label selectors
K-->>P: Return monitored endpoints info
else ServiceMonitor.enabled false
T->>K: Skip ServiceMonitor resource creation
end
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/clickhouse/values.yaml (1)
766-778: Enhance ServiceMonitor configuration documentation.The ServiceMonitor configuration could benefit from improved documentation and additional configuration options.
Apply this diff to enhance the configuration:
serviceMonitor: - # serviceMonitor.enabled -- ServiceMonitor Custom resource is created for a [prometheus-operator](https://github.com/prometheus-operator/prometheus-operator) + # -- If true, creates a Prometheus Operator ServiceMonitor (also requires prometheus-operator to be installed) enabled: false - # serviceMonitor.additionalLabels -- additional labels for service monitor + # -- Additional labels that can be used so ServiceMonitor resource(s) can be discovered by Prometheus additionalLabels: {} - # serviceMonitor.interval -- + # -- Prometheus scrape interval interval: 30s - # serviceMonitor.scrapeTimeout -- Prometheus ServiceMonitor scrapeTimeout. If empty, Prometheus uses the global scrape timeout unless it is less than the target's scrape interval value in which the latter is used. + # -- Prometheus scrape timeout. Leave empty to use the global scrape timeout scrapeTimeout: "" - # serviceMonitor.relabelings -- Prometheus [RelabelConfigs] to apply to samples before scraping + # -- RelabelConfigs to apply to samples before scraping. + # More info: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config relabelings: [] - # serviceMonitor.metricRelabelings -- Prometheus [MetricRelabelConfigs] to apply to samples before ingestio + # -- MetricRelabelConfigs to apply to samples before ingestion. + # More info: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#metric_relabel_configs metricRelabelings: [] + # -- Namespace where servicemonitor resource should be created + namespace: "" + # -- The labels to add to any time series or alerts when communicating with external systems + honorLabels: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml(1 hunks)charts/clickhouse/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml (1)
15-17: Verify service selector labels.The current selector might be too broad. Consider adding more specific labels to ensure the ServiceMonitor only targets the intended service.
Run this script to verify the service labels:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the service has matching labels for the ServiceMonitor # Test: Search for the service definition rg -A 5 "kind:\s+Service.*metrics-exporter" charts/Length of output: 53
Action Required: Manual Verification of Service Selector Labels
The initial search did not yield any service definitions with the “metrics-exporter” label, making automated confirmation inconclusive. It’s necessary to manually verify that the labels returned by
clickhouseOperator.selectorLabelsexactly match those on the intended service to ensure the ServiceMonitor targets only the desired service.
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml (1)
1-35: Consider adding a CRD existence check.While the template is well-structured, it would be beneficial to add a check for the ServiceMonitor CRD existence to prevent deployment failures when the Prometheus Operator is not installed.
Add this check at the beginning of the file:
{{- if .Values.clickhouseOperator.serviceMonitor.enabled }} +{{- if $.Capabilities.APIVersions.Has "monitoring.coreos.com/v1/ServiceMonitor" }} apiVersion: monitoring.coreos.com/v1 ... {{- end }} +{{- else }} +{{- fail "ServiceMonitor enabled but CRD not found. Please install the Prometheus Operator first." }} +{{- end }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (2)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml (2)
1-11: LGTM! Well-structured resource definition and metadata.The ServiceMonitor resource is properly defined with appropriate conditional creation, correct API version, and comprehensive metadata including dynamic name, namespace, and label configuration.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
12-34: LGTM! Comprehensive monitoring configuration.The ServiceMonitor specification is well-structured with:
- Complete endpoint configuration including essential fields
- Proper selector configuration
- Flexible optional parameters for customization
Note: The endpoint configuration has been improved as per the previous review comment.
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml
Outdated
Show resolved
Hide resolved
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml (1)
21-34: Consider adding validation for time-based parameters.While the optional parameters are well-structured, consider adding validation for
intervalandscrapeTimeoutto ensure they are valid Prometheus duration strings (e.g., "30s", "1m").Add validation using a helper template. Example:
{{- define "validateDuration" -}} {{- $duration := . -}} {{- if not (regexMatch "^([0-9]+(ns|us|µs|ms|s|m|h))+$" $duration) -}} {{- fail (printf "Invalid duration format: %s. Must be a valid Prometheus duration string (e.g., 30s, 1m)" $duration) -}} {{- end -}} {{- $duration -}} {{- end -}}Then use it like:
{{- with .Values.clickhouseOperator.serviceMonitor.interval }} - interval: {{ . }} + interval: {{ include "validateDuration" . }} {{- end }} {{- with .Values.clickhouseOperator.serviceMonitor.scrapeTimeout }} - scrapeTimeout: {{ . }} + scrapeTimeout: {{ include "validateDuration" . }} {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (3)
charts/clickhouse/templates/clickhouse-operator/servicemonitor.yaml (3)
1-11: LGTM! Well-structured metadata configuration.The conditional creation and metadata configuration follow Kubernetes best practices, with proper templating for name, namespace, and labels.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
13-17: LGTM! Complete endpoint configuration.The endpoint configuration includes all essential fields for Prometheus scraping: port, path, scheme, and honorLabels.
18-20: LGTM! Proper selector configuration.The selector configuration correctly uses matchLabels with the appropriate template function for generating selector labels.
Fix #183
Summary by CodeRabbit