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

feat: add serviceMonitor for apisix-gateway #706

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mstefany
Copy link
Contributor

Metrics' port & URL is defined here (port 9091 & URL /apisix/prometheus/metrics).

And it doesn't match what's in the serviceMonitor. TargetPort should be prometheus, a.k.a. 9091, and path is not specified which defaults to /metrics which is incorrect.

Fact that it's not working in default configuration is visible in my logs:

apisix-ingress-controller-6b4d5cc778-f582v apisix 10.254.82.207 - - [17/Jan/2024:11:00:22 +0000] 10.254.83.238:9080 "GET /metrics HTTP/1.1" 404 47 0.000 "-" "Prometheus/2.48.1" - - - "http://10.254.83.238:9080"

@mstefany
Copy link
Contributor Author

mstefany commented Mar 5, 2024

Please, is it possible to have this reviewed and approved?

@zll600
Copy link

zll600 commented Mar 6, 2024

ping @AlinsRan @Revolyssup

1 similar comment
@zll600
Copy link

zll600 commented Mar 6, 2024

ping @AlinsRan @Revolyssup

@flassagn
Copy link

Guys any feedback about the review ? We would like to enable the ingress controller monitoring ! 🙏

@Revolyssup
Copy link
Contributor

Guys any feedback about the review ? We would like to enable the ingress controller monitoring ! 🙏

I will take a look at this tomorrow and review. Thanks for reminding.

@Revolyssup Revolyssup changed the title fix: fix serviceMonitor port and path if enabled fix: allow configuring servicemonitor.path in ingress controller Mar 19, 2024
@Revolyssup Revolyssup changed the title fix: allow configuring servicemonitor.path in ingress controller fix: fix servicemonitor.path in ingress controller and make it configurable Mar 19, 2024
@Revolyssup Revolyssup self-requested a review March 19, 2024 06:02
@mstefany
Copy link
Contributor Author

I pushed the updated version, so that linter will be happy this time.

@flassagn
Copy link

flassagn commented Apr 2, 2024

Hi @Revolyssup,
The PR needs two approval. Could you assign another maintainer here ? I really would like to scrape ingress controller metrics ! 🙏

@mstefany mstefany changed the title fix: fix servicemonitor.path in ingress controller and make it configurable feat: add serviceMonitor for apisix-gateway Apr 12, 2024
@mstefany
Copy link
Contributor Author

@AlinsRan @Revolyssup I updated the PR:

  • because both apisix-ingress-controller and apisix containers have ports named http, original ServiceMonitor would scrape both containers (if apisix-gateway is enabled), and cause error every 15 seconds, because there are no metrics on http port 9080 of gateway at /metrics URL. For this reason I am renaming http port of apisix-gateway-controller to http-controller.
  • enabling ServiceMonitor of gateway is now done separately in .Values.gateway.serviceMonitor, and it also provisions separate ServiceMonitor object to scrape such metrics

Please, review the code if this approach is acceptable. I have updated README.md with helm-docs so there shouldn't be linter errors.

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

Successfully merging this pull request may close these issues.

None yet

5 participants