[helm] Enable metrics reporting on helm charts

[morazow]

Purpose

Linked issue: close #2677

Adds support for setting up the metrics on Helm chart values.

Brief change log

• Added section in the helm chart values
• Updated pods and services with metrics related configurations

Tests

Using helm unit tests

API and Format

NA

Documentation

Added usage docs to the Deploying with Helm documentation

[affo]

Amazing, thank you!
Some nits here in there, very punctual.

[affo]

I think this should not stay in the metrics.

We should add annotations externally, e.g.:

annotations:
  pod:
    common:
    coordinator:
    tablet

to allow adding arbitrary annotations, and not only for the context of metrics 🤝
See Bitnami example: https://artifacthub.io/packages/helm/bitnami/zookeeper

[morazow]

@affo thanks for the review 🙏

On the bitnami charts, they have separate annotations for metrics (under metrics.service):

metrics.service.annotations | Annotations for Prometheus to auto-discover the metrics endpoint | {}

https://github.com/bitnami/charts/blob/main/bitnami/zookeeper/values.yaml#L826-L844

But I agree, we would common and pod annotations later on.

[affo]

They have it in Zookeeper because they have a specific additional service for metrics.
In this PR you are not adding that service, so you would not have any specific annotations for that 🤝

[morazow]

Hello @affo, another question: at the moment for metrics we are adding the annotations in services, e.g, svc-coordinator.yaml.

Would it make sense to have:

annotations:
  service:

instead?

[affo]

Ah sorry, I did not notice that!
I think you are right, often times, exposing metrics through a headless service is the default way to go for solving connectivity issues and let Prometheus use the k8s DNS, hence:

• your design is correct, annotations makes sense under metrics
• but, you should expose the port on the service if metrics are enabled, otherwise this won't work!

Basically, we have to look at what a user would do:

Case 1: no Prometheus operator

Use annotations-based discovery.
You want to add to your service:

metadata:
  annotations:
    prometheus.io/scrape: "true"
    prometheus.io/port: "9249"
    prometheus.io/path: "/metrics"

The user would configure its Prometheus server to scrape those with relabel rules.

Case 2: WITH Prometheus operator

No annotation is required, but you need your service to expose the metrics port with a name and make it selectable via labels! E.g.:

...
metadata:
  labels:
    monitoring: enabled
spec:
  clusterIP: None                 # headless: DNS returns pod IPs
  selector: {}
  ports:
    - name: metrics               # IMPORTANT: ServiceMonitor references this name
      port: 9100
      targetPort: 9100

You would use the ServiceMonitor CRD like this (maintained by the user):

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  labels:
    release: prometheus           # IMPORTANT: must match Prometheus' CR serviceMonitorSelector
spec:
  selector:
    matchLabels:
      monitoring: enabled         # matches the Service labels above
  namespaceSelector:
    matchNames: []
  endpoints:
    - port: metrics               # matches Service port name
      path: /metrics
      interval: 30s
      scrapeTimeout: 10s

Conclusion

We should allow:

• specific service annotations for covering case 1
• add a named port for case 2 -> ideally the user may choose the port name
• add the possibility to add labels to the service for selection for case 2

Probably better to have a separate metrics service in order to have a clear separation of concerns 🤝

[affo]

Add link to Prometheus operator for reference

[affo]

Maybe better to provide the 2 prometheus examples?

[affo]

Better to add the Prometheus guide here 🤝
Should explain the scraping one and the ServiceMonitor one.

[affo]

why don't we apply a similar pattern to the SASL PR, where one enables reporters based on a reporters key?

In that case, we can set sane defaults for every reporter without making that appear 🤔

[morazow]

You mean like below:

reporters: prometheus
   prometheus:
       port: 9249
   jmx:
       port: 9250

?

If so, this would not work, in reporters one could enable both at the same time.