-
Notifications
You must be signed in to change notification settings - Fork 16.4k
add statsd service monitor into helm chart #56668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
closes: #56664
Add ServiceMonitor for StatsD to enable Prometheus monitoring. This allows Prometheus to scrape metrics from the StatsD service