[AMORO-2932] fix prometheus exporter issues in helm chart

[XBaith]

Why are the changes needed?

Close #2932 .

Brief change log

• Can report metrics via prometheus in k8s

How was this patch tested?

• Add some test cases that check the changes thoroughly including negative and positive cases if possible

• Add screenshots for manual tests if appropriate

• Run test locally before making a pull request

Documentation

• Does this pull request introduce a new feature? (no)
• If yes, how is the feature documented? (not documented)

[zhoujinsong]

prometheus seems to be ambiguous. I am not sure if the service name must be the same with the plugin name.

[XBaith]

Yes it is. AMS need to load the plugin class base on the name, and plugin name is same as property name in configmap

[baiyangtx]

When I first used the -exporter suffix considering because that we might implement a reporter based on push-gateway in the future.

[baiyangtx]

What's problem for '-' in kubernetes ?

[XBaith]

'-' is valid, the problem is the name should be less than 15 characters

[XBaith]

prometheus push-gateway has limitations for some scenarios and not suitable for long-lived jobs(refer to https://prometheus.io/docs/practices/pushing/#should-i-be-using-the-pushgateway). I suggest using prometheus operator + podmonitor for this purpose

[zhoujinsong]

How about prom-exporter? exporter seems to be an important part of the plugin name.

[XBaith]

@zhoujinsong I think it's fine, as long as it's stated that it's an abbreviation of prometheus

[XBaith]

@baiyangtx How about adding a additional property portName which can be used as containerPort in deployment and port in podmonitor

plugin:
  metricReporters:
    - name: prometheus-exporter
      enabled: true
      properties:
        port: 7001
        portName: metrics # which is hereby normatively defined as https://datatracker.ietf.org/doc/html/rfc6335#section-5.1
        interval: 60s
      service:
        type: ClusterIP
        port: "7001"
        nodePort: ~
        annotations: {
          "prometheus.io/scrape": "true",
          "prometheus.io/path": "/metrics",
          "prometheus.io/port": "7001"
        }

[XBaith]

apiVersion: monitoring.coreos.com/v1
kind: PodMonitor
metadata:
  labels:
    app.kubernetes.io/name: amoro-monitor
  name: amoro-monitor
spec:
  namespaceSelector:
    any: true
  podMetricsEndpoints:
    {{- range .Values.plugin.metricReporters }}
    - interval: {{ .properties.interval }}
      port: {{ .properties.portName }}
    {{- end}}
  selector:
    matchLabels:
      app.kubernetes.io/name: ams

[baiyangtx]

In this PR, can we simply handle the problem and only adapt helm charts for promethues?

metricReporters:
    promethues-exporter:
        enabled: true
        port: 7001
        properties: ~

and in deployment.yaml

          ports:
            - name: rest
              containerPort: {{ .Values.server.rest.port }}
            - name: table
              containerPort: {{ .Values.server.table.port }}
            - name: optimizing
              containerPort: {{ .Values.server.optimizing.port }}
            {{- include "amoro.pod.container.ports" . | nindent 8 }}

and we can manager port in _pod.tpl

{{- define "amoro.pod.container.ports" -}}
            - name: rest
              containerPort: {{ .Values.server.rest.port }}
            - name: table
              containerPort: {{ .Values.server.table.port }}
            - name: optimizing
              containerPort: {{ .Values.server.optimizing.port }}
{{- if .Values.metricRepoerters.promethuesExporter.enabled -}} 
            - name: promethues
              containerPort: {{ .Values.metricRepoerters.promethuesExporter.port }}
{{- end -}}
{{- end -}}

[XBaith]

@baiyangtx PTAL

[baiyangtx]

LGTM

[zhoujinsong]

Thanks for the work. @XBaith
Thanks for the review. @baiyangtx