Skip to content
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

fix: use targetPort instead of port on ServiceMonitor #859

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

tomasbedrich
Copy link
Contributor

@tomasbedrich tomasbedrich commented Jun 5, 2023

What does this PR do?

fixes #852

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

@tomasbedrich tomasbedrich marked this pull request as ready for review June 5, 2023 11:18
Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldez ldez changed the title fix: Generated ServiceMonitor doesn't work #852 fix: Generated ServiceMonitor doesn't work Jun 5, 2023
@mloiseleur
Copy link
Contributor

Thanks @tomasbedrich, for both clear bug report and this PR !

@mloiseleur mloiseleur changed the title fix: Generated ServiceMonitor doesn't work fix: use targetPort instead of port on ServiceMonitor Jun 5, 2023
@traefiker traefiker merged commit 78cb8f1 into traefik:master Jun 5, 2023
@jnoordsij
Copy link
Collaborator

Just noticed this change when upgrading to the new chart version. I think it doesn't do much wrong, but it is a behavioral change: the port options refers to the (Kubernetes) service port, while targetPort refers to the service pods. In this chart, both are called metrics, so no difference/issues there.

The reason why using the service monitor on its own does not work, is because the default service does not expose the metrics endpoint. So to have the service monitor working, you need to either add ports.metrics.expose = true to expose it, OR add a separate metrics service by adding metrics.prometheus.service.enabled = true (see #727).

So if issues arise with the new targetPort, this might be the solution to the original issue without changing the chart.

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

Successfully merging this pull request may close these issues.

Generated ServiceMonitor doesn't work
5 participants