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

docs: Add disable serving metrics info if port 0 #416

Merged
merged 1 commit into from
Jan 9, 2022

Conversation

mikesmithgh
Copy link
Collaborator

I was troubleshooting a couple things and wanted to disable prometheus metrics and came across the ability to disable it by setting the port to 0. See: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/manager/manager.go#L199

@grzesuav @AmitKumarDas I am thinking it might be better to default the value to 0 and have the user enable it if necessary. Thoughts?

Signed-off-by: Mike Smith <10135646+mjsmith1028@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #416 (a328188) into master (d5014ac) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #416   +/-   ##
=======================================
  Coverage   49.80%   49.80%           
=======================================
  Files          54       54           
  Lines        4883     4883           
=======================================
  Hits         2432     2432           
  Misses       2202     2202           
  Partials      249      249           
Flag Coverage Δ
integration 42.96% <ø> (-0.11%) ⬇️
unit 28.73% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5014ac...a328188. Read the comment docs.

@AmitKumarDas
Copy link
Contributor

@mjsmith1028 I have some queries w.r.t changing the default behaviour of metrics. What issues do you face with current default metrics settings. It will be nice to stick to existing defaults (to avoid changes to all end user deployments) if it is not a bug.

@mikesmithgh
Copy link
Collaborator Author

@mjsmith1028 I have some queries w.r.t changing the default behaviour of metrics. What issues do you face with current default metrics settings. It will be nice to stick to existing defaults (to avoid changes to all end user deployments) if it is not a bug.

Hi @AmitKumarDas, I don't have any issues. I was just thinking to disable it by default because some users may not have prometheus setup. But, I agree that makes sense to leave it as is to avoid a breaking change if users are on the current default port 9999 👍 thanks!

@grzesuav grzesuav merged commit 96059e8 into metacontroller:master Jan 9, 2022
@grzesuav
Copy link
Contributor

grzesuav commented Jan 9, 2022

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants