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

add cache metrics for NGINX plus #540

Merged
merged 1 commit into from
Nov 13, 2023
Merged

add cache metrics for NGINX plus #540

merged 1 commit into from
Nov 13, 2023

Conversation

sheharyaar
Copy link
Contributor

Proposed changes

Fixes #522 and exposes cache metrics from NGINX Plus API.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING
    guide
  • I have proven my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have ensured the README is up to date
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch on my own fork

@sheharyaar sheharyaar requested a review from a team as a code owner November 3, 2023 09:57
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Nov 3, 2023
@brianehlert brianehlert added the enhancement Pull requests for new features/feature enhancements label Nov 3, 2023
@shaun-nx shaun-nx self-requested a review November 10, 2023 09:19
@sheharyaar
Copy link
Contributor Author

The lint check has failed, do I need to perform gofumt in my branch or is it fine ?

@jjngx
Copy link
Contributor

jjngx commented Nov 13, 2023

The lint check has failed, do I need to perform gofumt in my branch or is it fine ?

Yes, please do run gofumpt.

Signed-off-by: Mohammad Shehar Yaar Tausif <sheharyaar48@gmail.com>
@github-actions github-actions bot removed the enhancement Pull requests for new features/feature enhancements label Nov 13, 2023
@sheharyaar
Copy link
Contributor Author

Done, fixed lint issues and rebased

@shaun-nx
Copy link

Hi @sheharyaar, thanks for contributing to this project! We can merge this PR as soon as the lint errors are fixed.

I get these two errors when running make lint locally

collector/nginx_plus.go:1210: File is not `gofumpt`-ed (gofumpt)

collector/nginx_plus.go:1190:97: unnecessary conversion (unconvert)
		ch <- prometheus.MustNewConstMetric(c.cacheZoneMetrics["cold"], prometheus.GaugeValue, float64(cold), name)

For the first error, just run gomt -w collector/nginx_plus.go

For the 2nd error, it seems like the float64(cold) conversion isn't needed.
I would remove this conversation and test if the metric still displays as you expect.

@shaun-nx
Copy link

@sheharyaar ignore my last command, I was late to the trigger 😆

@shaun-nx shaun-nx merged commit d57f771 into nginxinc:main Nov 13, 2023
12 checks passed
@shaun-nx
Copy link

Thank you @sheharyaar ! 🎉

@sheharyaar
Copy link
Contributor Author

Gald to contribute, welcome !

@lucacome lucacome added the enhancement Pull requests for new features/feature enhancements label Nov 14, 2023
@sheharyaar sheharyaar deleted the sheharyaar/issue522 branch November 14, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Expose cache metrics through exporter
5 participants