feat: add env vars for proxy configurations #1120

SantiagoPittella · 2025-02-03T14:58:31Z

closes #1026

bobbinth

Thank you! Looks good. Not a full review yet, but I left some comments inline.

bin/proving-service/Cargo.toml

bin/proving-service/src/commands/worker.rs

bin/proving-service/.env

bin/proving-service/src/commands/proxy.rs

Mirko-von-Leipzig · 2025-02-04T12:49:08Z

bin/proving-service/README.md

 # 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"


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.

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.

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.

I think this would basically mean that we merge StartProxy command and ProxyConfig structs, right?

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();

bobbinth

Looks good! Thank you! I left some comments inline. After these are addressed, we are good to merge.

bin/proving-service/src/commands/update_workers.rs

bin/proving-service/src/commands/mod.rs

bin/proving-service/README.md

CHANGELOG.md

bobbinth

Looks good! Thank you! I left a few more comments inline.

Also, I noticed that --help does not work on any of the commands except for start-proxy. Could we fix this?

bin/proving-service/README.md

bin/proving-service/src/commands/mod.rs

bin/proving-service/src/commands/update_workers.rs

SantiagoPittella · 2025-02-06T15:04:48Z

@bobbinth i tried running --help with each command and did not have any issue with it. Though I saw that there was a lot of room to improve the messages, in particular adding more information on each argument. The commit b169aff tackles it.

igamigo

LGTM! Leaving some minor suggestions, feel free to disregard if an alternative is preferred

igamigo · 2025-02-06T15:10:55Z

CHANGELOG.md

@@ -10,6 +10,7 @@
 - Renamed the protobuf file of the transaction prover to `tx_prover.proto` (#1110).
 - [BREAKING] Renamed `AccountData` to `AccountFile` (#1116).
 - Implement transaction batch prover in Rust (#1112).
+- Refactored config file for `miden-proving-service` to be based on environment variables (#1120).


nit: I think this might have to be [BREAKING]

igamigo · 2025-02-06T15:12:07Z

bin/proving-service/README.md

-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 system, you can run source <your-file>.


Suggested change

An example `.env` file is provided in the crate's root directory. To use the variables from a file in any Unix-like operating system, you can run source <your-file>.

An example `.env` file is provided in the crate's root directory. To use the variables from a file in any Unix-like operating system, you can run `source <your-file>`.

igamigo · 2025-02-06T15:18:05Z

bin/proving-service/src/commands/mod.rs

-    pub timeout_secs: u64,
+#[derive(Debug, Parser)]
+pub(crate) struct ProxyConfig {
+    /// Time in milliseconds to poll available workers.


nit: Considering the health check setting is called health_check_interval_secs, I'd suggest renaming this variable to use a similar convention. Could be something like available_workers_polling_interval_ms or maybe availability_check_interval_ms.

Also, this setting is for polling for availability and assigning tasks, right? If so it would be good to specify int he doc comment:

Suggested change

/// Time in milliseconds to poll available workers.

/// Interval in milliseconds at which the system polls for available workers to assign new tasks.

Sure, I just update the var to available_workers_polling_interval_ms.

igamigo · 2025-02-06T15:21:14Z

bin/proving-service/src/commands/mod.rs

+    /// Port of the proxy.
+    #[clap(long, default_value = "8082", env = "MPS_PORT")]
+    pub(crate) port: u16,
+    /// Maximum time in seconds to complete the entire request.


Suggested change

/// Maximum time in seconds to complete the entire request.

/// Maximum time in seconds allowed for a request to complete. Once exceeded, the request is aborted.

bobbinth

Looks good! Thank you!

bin/proving-service/README.md

SantiagoPittella added 3 commits February 3, 2025 11:57

feat: add env vars for proxy configurations

f5e701e

docs: update changelog

1f373db

feat: remove toml file and init command

b2379f1

bobbinth requested review from Mirko-von-Leipzig and bobbinth February 4, 2025 03:58

bobbinth reviewed Feb 4, 2025

View reviewed changes

bin/proving-service/Cargo.toml Outdated Show resolved Hide resolved

bin/proving-service/src/commands/worker.rs Outdated Show resolved Hide resolved

bin/proving-service/.env Outdated Show resolved Hide resolved

bin/proving-service/src/commands/proxy.rs Outdated Show resolved Hide resolved

Mirko-von-Leipzig reviewed Feb 4, 2025

View reviewed changes

SantiagoPittella added 8 commits February 4, 2025 18:42

remove env var arguments from start-worker command

0d5a621

review: remove env support for start-proxy command

0d4ea4f

review: remove figment and dotenvy

ecbf0f5

review: use MPS_ as env vars prefix

ba5bace

rename vars in .env with MPS_ prefix

38befc7

docs: update readme

c4fb6d2

Merge branch 'next' into santiagopittella-proxy-configs-through-env-vars

795b212

fix docs and remove unused dependency

e4fe076

bobbinth approved these changes Feb 4, 2025

View reviewed changes

SantiagoPittella added 2 commits February 5, 2025 14:29

review: merge ProxyConfig and StartProxy structs

d4ff777

review: update changelog entry

9732563

bobbinth reviewed Feb 6, 2025

View reviewed changes

SantiagoPittella added 4 commits February 6, 2025 11:27

review: improve env file section of the readme

3697ff7

review: improve commands documentation

b169aff

docs: update add/remove workers section of the readme

ba17e63

fix: start proxy command was bad formatted

816bdfc

igamigo reviewed Feb 6, 2025

View reviewed changes

SantiagoPittella added 5 commits February 6, 2025 15:28

review: mark the change as breaking

c8e6265

review: add missing backticks in readme

e5181cd

review: rename polling time frequency

ff9e61b

review: update comment of worker polling time

54709bb

review: update comment of timeout time

c96863b

SantiagoPittella requested a review from igamigo February 6, 2025 21:21

bobbinth approved these changes Feb 7, 2025

View reviewed changes

bin/proving-service/README.md Show resolved Hide resolved

SantiagoPittella mentioned this pull request Feb 7, 2025

chore: combine host and port in {add, remove}-workers command #1130

Open

SantiagoPittella merged commit 710289f into next Feb 7, 2025
12 checks passed

SantiagoPittella deleted the santiagopittella-proxy-configs-through-env-vars branch February 7, 2025 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add env vars for proxy configurations #1120

feat: add env vars for proxy configurations #1120

SantiagoPittella commented Feb 3, 2025

bobbinth left a comment

Mirko-von-Leipzig Feb 4, 2025

SantiagoPittella Feb 4, 2025

Mirko-von-Leipzig Feb 5, 2025

bobbinth Feb 5, 2025

Mirko-von-Leipzig Feb 5, 2025 •

edited

Loading

bobbinth left a comment

bobbinth left a comment

SantiagoPittella commented Feb 6, 2025

igamigo left a comment

igamigo Feb 6, 2025

igamigo Feb 6, 2025

igamigo Feb 6, 2025

SantiagoPittella Feb 6, 2025

igamigo Feb 6, 2025

bobbinth left a comment

	An example `.env` file is provided in the crate's root directory. To use the variables from a file in any Unix-like operating system, you can run source <your-file>.
	An example `.env` file is provided in the crate's root directory. To use the variables from a file in any Unix-like operating system, you can run `source <your-file>`.

	/// Time in milliseconds to poll available workers.
	/// Interval in milliseconds at which the system polls for available workers to assign new tasks.

	/// Maximum time in seconds to complete the entire request.
	/// Maximum time in seconds allowed for a request to complete. Once exceeded, the request is aborted.

feat: add env vars for proxy configurations #1120

feat: add env vars for proxy configurations #1120

Conversation

SantiagoPittella commented Feb 3, 2025

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mirko-von-Leipzig Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

SantiagoPittella commented Feb 6, 2025

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Mirko-von-Leipzig Feb 5, 2025 •

edited

Loading