-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨Expose metrics http server for extra endpoints #824
✨Expose metrics http server for extra endpoints #824
Conversation
Welcome @hypnoglow! |
Hi @hypnoglow. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
57f77ca
to
88ca0c6
Compare
/ok-to-test |
I am not really sure if I like the idea of allowing to re-use the builtin server for arbitrary handlers rather than just adding a new listener as If this is about pprof/other diagnostics in particular we could probably add an option to enable those specifically. WDYT? |
That's a fair statement, although seems there might be some potential useful use cases, and we can leave it up to the users. I don't have strong opinions here, I think either way can be fine |
88ca0c6
to
f761d5f
Compare
/assign @vincepri |
f761d5f
to
d95ee5c
Compare
This allows users to register extra http endpoints on the http server that serves metrics.
d95ee5c
to
3592cfb
Compare
defer close(s) | ||
go func() { | ||
defer GinkgoRecover() | ||
Expect(m.Start(s)).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the start race with the the Get request we do later on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this in a similar way as in other tests where HTTP requests are involved. It seems to work, but I see no strong argument why this could not race potentially, maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm okay, interesting. Well, we can still look at it if we actually see it flake.
/lgtm Thanks for your work! |
@vincepri is this currently still missing something from your POV? |
I think we're good, I was hoping to get the TODOs in issues though /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hypnoglow, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Allows users to add extra http handlers served on custom path to the http server that serves metrics. Might be useful to register some diagnostic endpoints e.g. pprof or custom debug info.
Ref: #684
Some additional thoughts: since we expose this, naming the server "metrics server" is not 100% correct anymore. Probably we might call it "diagnostic server" or something, that describes the purpose more clearly. Of course renaming would be a breaking change. As far as I found, the only metrics-related name we expose in the API is
MetricsBindAddress
in manager options.