feat: add env vars for proxy configurations #1120
Conversation
bin/proving-service/README.md
Outdated
| # Host of the proxy server | ||
| host = "0.0.0.0" | ||
| PROXY_HOST="0.0.0.0" | ||
| # Port of the proxy server | ||
| port = 8082 | ||
| PROXY_PORT="8082" | ||
| # Host of the workers update endpoint | ||
| PROXY_WORKERS_UPDATE_PORT="8083" | ||
| # Timeout for a new request to be completed | ||
| timeout_secs = 100 | ||
| PROXY_TIMEOUT_SECS="100" | ||
| # Timeout for establishing a connection to the worker | ||
| connection_timeout_secs = 10 | ||
| PROXY_CONNECTION_TIMEOUT_SECS="10" | ||
| # Maximum amount of items that a queue can handle | ||
| max_queue_items = 10 | ||
| PROXY_MAX_QUEUE_ITEMS="10" | ||
| # Maximum amount of retries that a request can take | ||
| max_retries_per_request = 1 | ||
| PROXY_MAX_RETRIES_PER_REQUEST="1" | ||
| # Maximum amount of requests that a given IP address can make per second | ||
| max_req_per_sec = 5 | ||
| PROXY_MAX_REQ_PER_SEC="5" | ||
| # Time to wait before checking the availability of workers | ||
| available_workers_polling_time_ms = 20 | ||
| PROXY_AVAILABLE_WORKERS_POLLING_TIME_MS="20" | ||
| # Interval to check the health of the workers | ||
| health_check_interval_secs = 1 | ||
| PROXY_HEALTH_CHECK_INTERVAL_SECS="1" | ||
| # Host of the metrics server | ||
| prometheus_host = "127.0.0.1" | ||
| PROXY_PROMETHEUS_HOST="127.0.0.1" | ||
| # Port of the metrics server | ||
| prometheus_port = 6192 | ||
| PROXY_PROMETHEUS_PORT="6192" | ||
| # Log level | ||
| RUST_LOG="info" |
There was a problem hiding this comment.
If we only use clap then I would suggest either directly copying --help output here, or only documenting the important ones and tell the user to check out more options with --help.
There was a problem hiding this comment.
We don't have this as command options, but are used to create the ProxyConfig struct. ATM, I;ve removed the TOML file and we are trying to create the ProxyConfig struct with the env vars or default values if not present. But since we don't have it as options, --help is not printing them.
Though I can change it to be part of the command if we consider that is it more useful that way.
There was a problem hiding this comment.
imo the default should always be cli unless there is a compelling reason not to. I would make these all clap variables with these default values, and with the env args set.
There was a problem hiding this comment.
I think this would basically mean that we merge StartProxy command and ProxyConfig structs, right?
There was a problem hiding this comment.
Correct yes, at least from the external API sense.
One might still keep the ProxyConfig struct to decouple the cli interface from the proxy code but whether or not that's applicable depends.
A good illustration of this ^^ is the node where currently each component has its own config, as well as a "normalized" variant for miden-node start node. When we refactor that we'll keep component config structs (but without toml support). Then its up to the clap/main binary code to convert between the cli args and the component config as it sees fit. e.g.
let cli_args = clap::parse()?;
let proxy_handle = Proxy {
endpoint: cli_args.endpoint,
timeout: cli_args.timeout,
request: RequestLimits {
retries: cli_args.requests.max_retries,
...
},
...
}.spawn();
| At the moment, when a worker added to the proxy stops working and can not connect to it for a request, the connection is marked as retriable meaning that the proxy will try reaching another worker. The number of retries is configurable via the `max_retries_per_request` value in the configuration file. | ||
| You can customize the proxy service by setting environment variables. Possible customizations can be found by running `miden-proving-service start-proxy --help`. | ||
|
|
||
| An example `.env` file is provided in the crate's root directory. To use the variables from a file, in any `Unix-like` operating systems, you can run `source <your-file>` |
There was a problem hiding this comment.
nit: missing period at the end of the paragraph.
Also, I would probably remove backticks from around "Unix-like". The last sentence could read: "To use the variables from a file in any Unix-like operating system, you can run source <your-file>.
| /// This method will make a request to the proxy adding workers. | ||
| AddWorkers(AddWorkers), |
There was a problem hiding this comment.
nit: should the text here be: "This command will make a request to the proxy to add the specified workers."
| /// This method will make a request to the proxy removing workers. | ||
| RemoveWorkers(RemoveWorkers), |
There was a problem hiding this comment.
nit: should the text here be: "This command will make a request to the proxy to remove the specified workers."
| pub struct AddWorkers { | ||
| workers: Vec<String>, | ||
| proxy_host: String, | ||
| proxy_update_workers_port: u64, | ||
| } |
There was a problem hiding this comment.
These are new command-line parameters, right? If so, we should mention them in the readme (under the "Updating workers on a running proxy" section).
| pub struct RemoveWorkers { | ||
| workers: Vec<String>, | ||
| proxy_host: String, | ||
| proxy_update_workers_port: u64, | ||
| } |
There was a problem hiding this comment.
Same comment as for AddWorkers command.
closes #1026