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

feat: load a config from a file #112

Merged
merged 11 commits into from
Jun 9, 2022
Merged

feat: load a config from a file #112

merged 11 commits into from
Jun 9, 2022

Conversation

ramfox
Copy link
Contributor

@ramfox ramfox commented Jun 2, 2022

  • use config create to build config
  • implement config::Source on each config struct
  • tests in:
    • gateway
    • p2p
    • store
    • ctl
    • util
  • more manual testing for adding non-default addrs

we can now use a iroh_util::make_config function that takes:

  1. default config
  2. a list of possible file paths where a config can be loaded
  3. a prefix that this config's env vars will be labeled (eg,
    IROH_GATEWAY_PORT=4000, the prefix is IROH_GATEWAY & the field
    that you are trying to set is port
  4. command line flag overrides

The function layers these options, starting with the default config and
ending with the command line flags.

  • add metrics config to each config file
  • add IROH_METRICS as a valid env var prefix
  • add IROH_INSTANCE_ID & IROH_ENV to set metrics::Config.instance_id & metrics::Config::service_env, respectfully
  • refactor each process to get metrics configuration using our new make_config code path

@ramfox ramfox added feat New feature or request c-gateway labels Jun 2, 2022
@ramfox ramfox self-assigned this Jun 2, 2022
`iroh_rpc_client::Config` implements the `config::Source` trait
files
`make_config` asks for a:
1) default config
2) a list of possible file paths where a config can be loaded
3) a prefix that this config's env vars will be labeled (eg,
   `IROH_GATEWAY_PORT=4000`, the prefix is `IROH_GATEWAY` & the field
   that you are trying to set is `port`
4) command line flag overrides

The method layers these options, starting with the default config and
ending with the command line flags.
}
}
/// rpc listening addr
pub rpc_addr: SocketAddr,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed this not because I necessarily think it's better, but just so that the way we are handling the rpc_addr and rpc_clients is uniform over each config.

headers.insert(CACHE_CONTROL, VALUE_NO_CACHE_NO_TRANSFORM.clone());
headers.insert(ACCEPT_RANGES, VALUE_NONE.clone());
self.headers = headers;
self.headers = default_headers();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed to test the conversion between config::Config and the http::HeaderMap, so split this out so it could be used to help with tests separate from the gateway::Config

@ramfox ramfox marked this pull request as ready for review June 8, 2022 06:22
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise LGTM. Have some functional requests though.
There's a common set of env vars that I use across services which are mainly utilized by the metrics and ops side of things.
Every service has to define their:

Note that CARGO_PKG_VERSION & CARGO_PKG_NAME are compile time env vars and are not in this bunch, so just ignore those.

And then there are metrics specific ones at https://github.com/n0-computer/iroh/blob/main/iroh-metrics/src/config.rs#L30
Which include:

  • IROH_METRICS_DEBUG
  • IROH_METRICS_COLLECTOR_ENDPOINT
  • IROH_METRICS_PROM_GATEWAY_ENDPOINT

These should also be present in all the services. Ideally default for endpoints should be "" which is interpreted as a --no-metrics arg but that can be a separate thing. I want to split metrics and traces anyways.

It would be great to absorb these into the config too so we have a unified interface for managing those.

Allow using `IROH_METRICS_*` env vars to set fields in the metrics
config. Also allows for `IROH_INSTANCE_ID` to set
`metrics::Config.instance_id` & `IROH_ENV` to set
`metrics::Config.service_env` fields.
@ramfox
Copy link
Contributor Author

ramfox commented Jun 9, 2022

@Arqu

Okay, latest commit has these changes!

The biggest change is that the metrics::Config creation process has been refactored to match the other config structs a bit more. For example, it is expected to have a `Config::default()), after which we layer on values found in the config files, then values found in the env vars, then flag overrides (and unique to the metrics config, we layer on compile-time values after that).

This also de-dups some of the work we were previously doing around pulling from env vars (except for the compile-time values)!

I've done some manual testing on top of the tests we already have (by replacing random metrics values & seeing if they properly output in a printed config), but would love to know if this disrupts any services we already have running.

@ramfox ramfox requested a review from Arqu June 9, 2022 04:22
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally perfectly fine!

@ramfox ramfox merged commit 652b7ff into n0-computer:main Jun 9, 2022
@ramfox ramfox deleted the config branch June 9, 2022 15:50
dignifiedquire added a commit that referenced this pull request Feb 20, 2023
Tests are failing because of port reuse issues and shutdown, should be
fixed with the other PRs merged

Needed to allow work on hole punching in the short term.

Ref #111 

See

- aws/s2n-quic#1607
- aws/s2n-quic#1608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants