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

Make metric port configurable #101

Closed
markmandel opened this issue Sep 9, 2020 · 4 comments · Fixed by #220
Closed

Make metric port configurable #101

markmandel opened this issue Sep 9, 2020 · 4 comments · Fixed by #220
Labels
area/operations Installation, updating, metrics etc kind/feature New feature or request

Comments

@markmandel
Copy link
Member

Just ran into this when trying to run a local demo - couldn't spin up two instances of quilkin locally, as both were trying to bind to 9091.

Would be handy to make this configurable in the yaml.

@markmandel markmandel added kind/feature New feature or request area/operations Installation, updating, metrics etc labels Sep 9, 2020
@markmandel
Copy link
Member Author

@iffyio I was going to dig into this, but found a couple of interesting things:

Within our proxy config docs, we have a admin.metrics section - but we don't have any such config settings that exist yet (unless I missed a bit?).

We do however have an admin.address in the Config model

Looking at Envoy - they do have a /stats that comes off the same port as the admin interface -- are we happy doing the same thing, or is there any reason we would want metrics to run off a different port to the admin port?

I am thinking that if we ever add paths to the admin port that allow mutation of the proxy, it would be more secure to have metrics be on a separate port (just digging into some security related Envoy discussion). So I think I would be pro-making it a separate port from everything else, and can be secured (or in the case of metrics, be more open) separately from anything else.

If we agree on that, I'll do the bits to flesh out the model, and get that integrated 👍

@iffyio
Copy link
Collaborator

iffyio commented Mar 16, 2021

Within our proxy config docs, we have a admin.metrics section - but we don't have any such config settings that exist yet (unless I missed a bit?).

Yeah the doc isn't correct looks more like. I think we can run metrics on the same admin port as envoy currently does and update to the docs to match that

@markmandel
Copy link
Member Author

Okay cool - it seems like it'll be fine. We can also do a TLS cert or other auth methods down the line if we decide we need more access control.

Working on this now.

@markmandel
Copy link
Member Author

I should say - as part of this work, I'll move the hyper server into it's own "Admin" module, and come up with a strategy that allows other modules to plug in.

markmandel added a commit that referenced this issue Mar 22, 2021
Update the model and documentation for the Admin interface to match, and
provide defaulting.

Next will be the implementation of the `Admin` functionality, and pulling
it out of `Metrics`.

Work on #101
markmandel added a commit that referenced this issue Mar 22, 2021
Update the model and documentation for the Admin interface to match, and
provide defaulting.

Next will be the implementation of the `Admin` functionality, and pulling
it out of `Metrics`.

Work on #101
markmandel added a commit that referenced this issue Mar 22, 2021
This pulls the `hyper` server out of the `Metrics` module and move it
into its own `Admin` module that handles metrics, and in the future, the
health/liveness endpoint as well.

Closes #101
markmandel added a commit that referenced this issue Mar 24, 2021
This pulls the `hyper` server out of the `Metrics` module and move it
into its own `Admin` module that handles metrics, and in the future, the
health/liveness endpoint as well.

Closes #101
markmandel added a commit that referenced this issue Mar 29, 2021
* Pull `Admin` module out of `Metrics`

This pulls the `hyper` server out of the `Metrics` module and move it
into its own `Admin` module that handles metrics, and in the future, the
health/liveness endpoint as well.

Closes #101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operations Installation, updating, metrics etc kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants