affo commented on code in PR #2711:
URL: https://github.com/apache/fluss/pull/2711#discussion_r2918137899
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -183,6 +183,17 @@ The following table lists the configurable parameters of
the Fluss chart and the
| `listeners.internal.port` | Internal communication port | `9123` |
| `listeners.client.port` | Client port (intra-cluster) | `9124` |
+### Metrics Parameters
+
+| Parameter | Description | Default |
+|-----------|-------------|---------|
+| `metrics.reporters` | Comma-separated reporter selector; use `none` to
disable metrics | `none` |
+| `metrics.jmx.port` | JMX reporter port range | `9250` |
+| `metrics.prometheus.port` | Prometheus reporter port | `9249` |
+| `metrics.prometheus.service.portName` | Named port exposed on metrics
services (used by ServiceMonitor endpoint `port`) | `metrics` |
+| `metrics.prometheus.service.labels` | Additional labels added to metrics
services for ServiceMonitor selectors | `{}` |
Review Comment:
```suggestion
| `metrics.prometheus.service.labels` | Additional labels added to metrics
services | `{}` |
```
Same here
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -183,6 +183,17 @@ The following table lists the configurable parameters of
the Fluss chart and the
| `listeners.internal.port` | Internal communication port | `9123` |
| `listeners.client.port` | Client port (intra-cluster) | `9124` |
+### Metrics Parameters
+
+| Parameter | Description | Default |
+|-----------|-------------|---------|
+| `metrics.reporters` | Comma-separated reporter selector; use `none` to
disable metrics | `none` |
+| `metrics.jmx.port` | JMX reporter port range | `9250` |
+| `metrics.prometheus.port` | Prometheus reporter port | `9249` |
+| `metrics.prometheus.service.portName` | Named port exposed on metrics
services (used by ServiceMonitor endpoint `port`) | `metrics` |
Review Comment:
```suggestion
| `metrics.prometheus.service.portName` | Named port exposed on metrics
services | `metrics` |
```
I would remove this, otherwise it feels like these Helm Charts are offering
a ServiceMonitor
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -183,6 +183,17 @@ The following table lists the configurable parameters of
the Fluss chart and the
| `listeners.internal.port` | Internal communication port | `9123` |
| `listeners.client.port` | Client port (intra-cluster) | `9124` |
+### Metrics Parameters
+
+| Parameter | Description | Default |
+|-----------|-------------|---------|
+| `metrics.reporters` | Comma-separated reporter selector; use `none` to
disable metrics | `none` |
+| `metrics.jmx.port` | JMX reporter port range | `9250` |
+| `metrics.prometheus.port` | Prometheus reporter port | `9249` |
+| `metrics.prometheus.service.portName` | Named port exposed on metrics
services (used by ServiceMonitor endpoint `port`) | `metrics` |
+| `metrics.prometheus.service.labels` | Additional labels added to metrics
services for ServiceMonitor selectors | `{}` |
+| `metrics.prometheus.service.annotations` | Optional annotations added to
metrics services (for annotation-based scraping) | `{}` |
+
Review Comment:
We should add a final row like:
```md
| metrics.reporter.<reporter>.<option> | Configure <option> for <reporter>
(refer to the [Fluss
configuration](https://fluss.apache.org/docs/maintenance/configuration/#metrics)
| NA |
```
##########
helm/README.md:
##########
@@ -93,6 +93,78 @@ Important Fluss options surfaced by the chart:
- internal.listener.name: Which listener is used for internal communication
(defaults to INTERNAL).
- tablet-server.id: Required to be unique per TabletServer. The chart
auto‑derives this from the StatefulSet pod ordinal at runtime.
+### Metrics and Prometheus Scraping
Review Comment:
This can go away once https://github.com/apache/fluss/issues/2840 gets fixed
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -255,6 +267,63 @@ listeners:
port: 9124
```
+### Metrics and Monitoring
+
+When `metrics.enabled` is `true`, adds the following `server.yaml` config
entries:
+
+- `metrics.reporters`: comma-separated reporter names from `metrics.reporters`
+- `metrics.reporter.<name>.<option>`: one entry per reporter option in
`metrics.reporters`
+
+If a metrics key is already provided in `configurationOverrides` (for example,
`metrics.reporters` or `metrics.reporter.prometheus.port`), the chart keeps the
value from `configurationOverrides`.
+
+#### Prometheus Annotation Based Scraping
+
+The example values below show how to add annotations to the metrics services
so that a Prometheus server can discovery and scrape them automatically based
on the annotations:
+
+```yaml
+metrics:
+ enabled: true
+ reporters:
+ prometheus:
+ port: 9249
+ service:
+ annotations:
+ prometheus.io/scrape: "true"
+ prometheus.io/path: "/metrics"
+ prometheus.io/port: "9249"
+```
+
+#### Prometheus ServiceMonitor Based Scraping
+
+Similarly, if using the [Prometheus
Operator](https://prometheus-operator.dev/), use the values below to add labels
to the metrics services and then create a
[`ServiceMonitor`](https://prometheus-operator.dev/docs/api-reference/api/#monitoring.coreos.com/v1.ServiceMonitor)
that selects them:
Review Comment:
Not resolved
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -255,6 +267,63 @@ listeners:
port: 9124
```
+### Metrics and Monitoring
+
+When `metrics.enabled` is `true`, adds the following `server.yaml` config
entries:
+
+- `metrics.reporters`: comma-separated reporter names from `metrics.reporters`
+- `metrics.reporter.<name>.<option>`: one entry per reporter option in
`metrics.reporters`
+
+If a metrics key is already provided in `configurationOverrides` (for example,
`metrics.reporters` or `metrics.reporter.prometheus.port`), the chart keeps the
value from `configurationOverrides`.
+
+#### Prometheus Annotation Based Scraping
+
+The example values below show how to add annotations to the metrics services
so that a Prometheus server can discovery and scrape them automatically based
on the annotations:
Review Comment:
not resolved
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -255,6 +266,61 @@ listeners:
port: 9124
```
+### Metrics and Monitoring
+
+When `metrics.reporters` is not `none`, the chart adds the following
`server.yaml` config entries:
Review Comment:
By going through this, now `none` feels uncomfortable, why not simply empty?
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -255,6 +267,63 @@ listeners:
port: 9124
```
+### Metrics and Monitoring
+
+When `metrics.enabled` is `true`, adds the following `server.yaml` config
entries:
+
+- `metrics.reporters`: comma-separated reporter names from `metrics.reporters`
+- `metrics.reporter.<name>.<option>`: one entry per reporter option in
`metrics.reporters`
+
+If a metrics key is already provided in `configurationOverrides` (for example,
`metrics.reporters` or `metrics.reporter.prometheus.port`), the chart keeps the
value from `configurationOverrides`.
+
+#### Prometheus Annotation Based Scraping
+
+The example values below show how to add annotations to the metrics services
so that a Prometheus server can discovery and scrape them automatically based
on the annotations:
+
+```yaml
+metrics:
+ enabled: true
+ reporters:
+ prometheus:
+ port: 9249
+ service:
+ annotations:
+ prometheus.io/scrape: "true"
+ prometheus.io/path: "/metrics"
+ prometheus.io/port: "9249"
+```
+
+#### Prometheus ServiceMonitor Based Scraping
+
+Similarly, if using the [Prometheus
Operator](https://prometheus-operator.dev/), use the values below to add labels
to the metrics services and then create a
[`ServiceMonitor`](https://prometheus-operator.dev/docs/api-reference/api/#monitoring.coreos.com/v1.ServiceMonitor)
that selects them:
+
+```yaml
+metrics:
+ enabled: true
+ reporters:
+ prometheus:
+ port: 9249
+ service:
+ portName: metrics
+ labels:
+ monitoring: enabled
+```
+
+Then create a `ServiceMonitor` resource that matches the label:
Review Comment:
not resolved
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -183,6 +183,17 @@ The following table lists the configurable parameters of
the Fluss chart and the
| `listeners.internal.port` | Internal communication port | `9123` |
| `listeners.client.port` | Client port (intra-cluster) | `9124` |
+### Metrics Parameters
+
+| Parameter | Description | Default |
+|-----------|-------------|---------|
+| `metrics.reporters` | Comma-separated reporter selector; use `none` to
disable metrics | `none` |
+| `metrics.jmx.port` | JMX reporter port range | `9250` |
+| `metrics.prometheus.port` | Prometheus reporter port | `9249` |
+| `metrics.prometheus.service.portName` | Named port exposed on metrics
services (used by ServiceMonitor endpoint `port`) | `metrics` |
+| `metrics.prometheus.service.labels` | Additional labels added to metrics
services for ServiceMonitor selectors | `{}` |
+| `metrics.prometheus.service.annotations` | Optional annotations added to
metrics services (for annotation-based scraping) | `{}` |
Review Comment:
```suggestion
| `metrics.prometheus.service.annotations` | Optional annotations added to
metrics services | `{}` |
```
Later we explain how to use those fields
##########
helm/templates/_metrics.tpl:
##########
@@ -0,0 +1,91 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+{{/*
+Returns list of provided reporter names.
+*/}}
+{{- define "fluss.metrics.reporterNames" -}}
+{{- $metrics := .Values.metrics | default dict -}}
+{{- $reportersValue := default "none" $metrics.reporters | toString | trim -}}
+{{- if or (eq $reportersValue "") (eq $reportersValue "none") -}}
+[]
+{{- else -}}
+{{- $selected := list -}}
+{{- range $raw := regexSplit "\\s*,\\s*" $reportersValue -1 -}}
+{{- $name := trim $raw -}}
+{{- if ne $name "" -}}
+{{- if eq $name "none" -}}
+{{- fail "metrics.reporters cannot combine 'none' with other reporters" -}}
+{{- end -}}
+{{- $selected = append $selected $name -}}
+{{- end -}}
+{{- end -}}
+{{- $selected | toYaml -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Checks if prometheus reporter is enabled.
+*/}}
+{{- define "fluss.metrics.prometheusEnabled" -}}
+{{- $reporterNames := include "fluss.metrics.reporterNames" . | fromYamlArray
-}}
+{{- if has "prometheus" $reporterNames -}}
+true
+{{- end -}}
+{{- end -}}
+
+{{/*
+Renders metrics reporter configuration entries.
+Expects the root context as argument.
+
+From values:
+ metrics:
+ reporters: prometheus
+ prometheus:
+ port: 9249
+
+Renders:
+ metrics.reporters: prometheus
+ metrics.reporter.prometheus.port: 9249
+
+Keys already present in configurationOverrides are not rendered.
+*/}}
+{{- define "fluss.metrics.config" -}}
+{{- $config := .Values.configurationOverrides | default dict -}}
+{{- $metrics := .Values.metrics | default dict -}}
+{{- $reporterNames := include "fluss.metrics.reporterNames" . | fromYamlArray
-}}
+{{- if gt (len $reporterNames) 0 -}}
+{{- if not (hasKey $config "metrics.reporters") }}
+metrics.reporters: {{ join "," $reporterNames }}
+{{- end -}}
+{{- range $name := $reporterNames -}}
+{{- if not (hasKey $metrics $name) -}}
+{{- fail (printf "metrics.%s must be configured when metrics.reporters
includes %s" $name $name) -}}
+{{- end -}}
+{{- $reporterConfig := index $metrics $name -}}
+{{- range $option, $value := $reporterConfig -}}
+{{- if ne $option "service" -}}
+{{- $fullKey := printf "metrics.reporter.%s.%s" $name $option -}}
+{{- if not (hasKey $config $fullKey) }}
+{{ $fullKey }}: {{ tpl (printf "%v" $value) $ }}
Review Comment:
You are setting defaults in the service, however those may not match what
gets written to the configmap.
I think this would be a problem 🤔
I think leaving it open for the user to set `<name>.<option>` is a bit too
much, also it becomes difficult to document. should we only require `port`
explicitly and leave the rest if one wants to `configurationOverrides`?
##########
website/docs/install-deploy/deploying-with-helm.md:
##########
@@ -255,6 +266,61 @@ listeners:
port: 9124
```
+### Metrics and Monitoring
+
+When `metrics.reporters` is not `none`, the chart adds the following
`server.yaml` config entries:
+
+- `metrics.reporters`: comma-separated reporter names from `metrics.reporters`
+- `metrics.reporter.<name>.<option>`: one entry per reporter option in
`metrics.<name>`
+
+If a metrics key is already provided in `configurationOverrides` (for example,
`metrics.reporters` or `metrics.reporter.prometheus.port`), the chart keeps the
value from `configurationOverrides`.
+
+#### Prometheus Annotation Based Scraping
+
+The example values below show how to add annotations to the metrics services
so that a Prometheus server can discovery and scrape them automatically based
on the annotations:
+
+```yaml
+metrics:
+ reporters: prometheus
+ prometheus:
+ port: 9249
+ service:
+ annotations:
+ prometheus.io/scrape: "true"
+ prometheus.io/path: "/metrics"
+ prometheus.io/port: "9249"
+```
+
+#### Prometheus ServiceMonitor Based Scraping
+
+Similarly, if using the [Prometheus
Operator](https://prometheus-operator.dev/), use the values below to add labels
to the metrics services and then create a
[`ServiceMonitor`](https://prometheus-operator.dev/docs/api-reference/api/#monitoring.coreos.com/v1.ServiceMonitor)
that selects them:
+
+```yaml
+metrics:
+ reporters: prometheus
+ prometheus:
+ port: 9249
+ service:
+ portName: metrics
+ labels:
+ monitoring: enabled
+```
+
+Then create a `ServiceMonitor` resource that matches the label:
+
+```yaml
+apiVersion: monitoring.coreos.com/v1
+kind: ServiceMonitor
+metadata:
+ name: fluss-metrics
+spec:
+ selector:
+ matchLabels:
+ monitoring: enabled
+ endpoints:
+ - port: metrics
Review Comment:
```suggestion
# Matches `metrics.prometheus.service.portName`
- port: metrics
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
