add statsd service monitor into helm chart#56668
Conversation
|
Hi @ido177, if there is an addition to the values.yaml, the changes in the values.schema.yaml are also required (that is the reason of failing tests - https://github.com/apache/airflow/actions/runs/18531823289/job/52818611830?pr=56668). It looks like this PR is still in progress. Do you have the possibility to convert it to a draft? If not, maybe just add the |
|
Oops, I missed that one. Is there an automated way to fill up the I tried |
4c41afa to
17c4ffc
Compare
|
To be honest, I've done it manually every time as I didn't have many changes to introduce really, but some automated way of filling it will some check it a pre-commit could be nice 🤔 |
17c4ffc to
91fe609
Compare
Miretpl
left a comment
There was a problem hiding this comment.
From my end, the change looks good; the only question is whether we want to have in the official Helm chart things which require the installation of a custom resource in the k8s to work. If yes, we should probably add some information somewhere in the doc and values files that this feature will not work without the installation of a proper CRD.
|
If everything is fine with adding Service Monitor, I would also like to add it to the pgbouncer Metrics Exporter. It contains important metrics for monitoring. |
|
@ido177 perhaps it might be better to merge this PR first and create a separate one for PgBouncer, to keep the changes well-scoped and avoid expanding the file set here. |
|
One request as leftover: Can you please also add a small test to see that based on the configuration the desired outcome is generated? |
|
Hi @ido177, could you add a small test case for this? |
|
I would suggest adding this behind helm's APIVersions capability check such as |
|
Still missing some tests. |
ffc0e74 to
837ae39
Compare
6d12454 to
8e45e2c
Compare
8e45e2c to
f12d385
Compare
|
@ido177 Please allow for the apiVersion to be customizable for those of us who use a managed Prometheus offering. For example microsoft uses: azmonitoring.coreos.com/v1 Traefik does this: https://github.com/traefik/traefik-helm-chart/blob/e66342de879b16214216b9196dba9aece36a5d7a/traefik/templates/servicemonitor.yaml#L8 There will also need to be a way to disable the crd check if you are adding one. Traefik does this here: https://github.com/traefik/traefik-helm-chart/blob/e66342de879b16214216b9196dba9aece36a5d7a/traefik/templates/servicemonitor.yaml#L4 |
|
I was reading the code and attempting to understand what is added / achieved here. Did not understand it. A lot of YAML and complexity added. Is this all needed? Looks like rather an external helm reference is needed. Maybe I am not an expert in this area but see too much complexity for "just service monitoring". What would be the diff to liveness? |
closes: #56664
Add ServiceMonitor for StatsD to enable Prometheus monitoring. This allows Prometheus to scrape metrics from the StatsD service