Pull Admin module out of Metrics
#220
Conversation
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
- Rename `admin_response` ➡ `collect_metrics` - Move some `pub` crates to `pub(crate)` - Refactor `test_util.rs` to align more with the builder pattern.
markmandel
force-pushed
the
feature/admin-server
branch
from
e72e601
to
eb3a2dd
Compare
src/test_utils.rs
Outdated
| } | ||
|
|
||
| /// Create and run a server with the admin interface enabled. | ||
| pub fn run_server_with_admin(&mut self, config: Config) { |
There was a problem hiding this comment.
this should be something like run_server_with_config? since admin's not particulary specific to the function (i.e there's other things that are enabled as well)
src/test_utils.rs
Outdated
| } | ||
|
|
||
| /// Create and run a server (no admin) | ||
| fn run_server_with_arguments(&mut self, config: Config, filter_registry: FilterRegistry) { |
There was a problem hiding this comment.
I'm finding this function signature a bit confusing, currently run_server_with_admin(config) enables admin while run_server_with_arguments(config, filter_reg) disables it. I didn't specify anything admin related in either call yet they have opposite behaviors e.g I can't have run_server_with_arguments(config, filter_reg) and have admin enabled?
I think this would be clearer if we say admin is always disabled and to enable it pass in the builder explicitly so we avoid any combinatorial argument passing issues
There was a problem hiding this comment.
Since we have such nice defaults on the proxy::Builder I got rid of the various run_server functions, and kept just a single function that takes a proxy::Builder -- I think it makes things much neater overall. Have a look, let me know what you think.
There was a problem hiding this comment.
Can we keep the run_server(config) function? I think its simple api would be worth it to avoid having to duplicate Builder::from(Arc::new(server_config)).disable_admin() in every test case since it allows the most common scenario to be config free
There was a problem hiding this comment.
So run_server_with_config(config) and run_server_with_builder(builder) ?
There was a problem hiding this comment.
Yeah that sounds reasonable to me!
This pulls the
hyperserver out of theMetricsmodule and move it into its ownAdminmodule that handles metrics, and in the future, the health/liveness endpoint as well.Closes #101