Skip to content

Conversation

@ido177
Copy link
Contributor

@ido177 ido177 commented Oct 15, 2025

closes: #56664


Add ServiceMonitor for StatsD to enable Prometheus monitoring. This allows Prometheus to scrape metrics from the StatsD service

@Miretpl
Copy link
Contributor

Miretpl commented Oct 16, 2025

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 [WIP] at the beginning of the title to mark it somehow that it is not ready to review yet (tests are failing)?

@ido177
Copy link
Contributor Author

ido177 commented Oct 17, 2025

Oops, I missed that one. Is there an automated way to fill up the values.schema.json file(some cli tool like helm schema or some script)? I'm sure it's not done manually, but I couldn't find a documented workflow.

I tried helm schema, but it changes the file wrong way, apparently something else should be used or maybe some extra args required.

@ido177 ido177 force-pushed the add-statsd-service-monitor-into-helm-chart branch 3 times, most recently from 4c41afa to 17c4ffc Compare October 17, 2025 10:10
@Miretpl
Copy link
Contributor

Miretpl commented Oct 17, 2025

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 🤔

@potiuk potiuk force-pushed the add-statsd-service-monitor-into-helm-chart branch from 17c4ffc to 91fe609 Compare October 18, 2025 23:00
Copy link
Contributor

@Miretpl Miretpl left a 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.

@ido177
Copy link
Contributor Author

ido177 commented Oct 21, 2025

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.

@RomanKrasavtsev
Copy link

@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.

@jscheffl jscheffl added this to the Airflow Helm Chart 1.19.0 milestone Nov 23, 2025
@jscheffl
Copy link
Contributor

One request as leftover: Can you please also add a small test to see that based on the configuration the desired outcome is generated?

@Miretpl
Copy link
Contributor

Miretpl commented Dec 22, 2025

Hi @ido177, could you add a small test case for this?

@noderat
Copy link

noderat commented Dec 23, 2025

I would suggest adding this behind helm's APIVersions capability check such as {{- if .Capabilities.APIVersions.Has "monitoring.coreos.com/v1/ServiceMonitor" }}

@jscheffl
Copy link
Contributor

Still missing some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add serviceMonitor for statsd into helm chart

5 participants