feat: config from env#406
Conversation
noxware
force-pushed
the
302-config-from-env
branch
from
2d1436c to
f5654a0
Compare
| const ENV_DIAL9_TASK_DUMP_ENABLED: &str = "DIAL9_TASK_DUMP_ENABLED"; | ||
| const ENV_DIAL9_TASK_DUMP_IDLE_THRESHOLD_MS: &str = "DIAL9_TASK_DUMP_IDLE_THRESHOLD_MS"; | ||
|
|
||
| const DEFAULT_ENABLED: bool = false; |
There was a problem hiding this comment.
Note: When configuring the library manually, from Rust code, we default to enabled, but, by looking at the issue and production_use.rs I decided to default to disabled when using from_env.
| const DEFAULT_CPU_PROFILE_ENABLED: bool = cfg!(all(target_os = "linux", feature = "cpu-profiling")); | ||
| const DEFAULT_SCHEDULE_PROFILE_ENABLED: bool = | ||
| cfg!(all(target_os = "linux", feature = "cpu-profiling")); | ||
| const DEFAULT_TASK_DUMP_ENABLED: bool = false; |
There was a problem hiding this comment.
Defaults to false as it seems to be expensive.
| trace_dir: PathBuf, | ||
|
|
||
| // None means the underlying RotatingWriter builder owns the default. | ||
| rotation_period: Option<Duration>, |
There was a problem hiding this comment.
Note: The issue showcases 15 as the proposed default, but in my implementation, I'm simply letting the builder use its existing default (which seems to be 60).
In general, I tried to avoid having different defaults to the ones that are already in-place. The only exception was DIAL9_ENABLED as mentioned in other comment.
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct ParsedEnvConfig { |
There was a problem hiding this comment.
Split "parsed" and "resolved" mainly to get better observability in tests and to reduce the responsibility of the parse_env_config function but I know it adds some duplication.
| "unknown-service".to_string() | ||
| } | ||
|
|
||
| fn warn(message: fmt::Arguments<'_>) { |
There was a problem hiding this comment.
Note: I added this function to mimic the error logging logic from build_or_disabled but I would personally just do tracing::warn!(), wdyt?
| /// `from_env()` behavior. The returned config is built with | ||
| /// [`Dial9ConfigBuilder::build_or_disabled`], so writer setup failures are | ||
| /// logged and downgraded to a plain Tokio runtime. | ||
| pub fn from_env() -> Self { |
There was a problem hiding this comment.
We could also have an additional try_from_env that returns a Result to handle in case something goes wrong.
There was a problem hiding this comment.
Related to this, the issue currently says
When configuration is invalid, debug builds panic and release builds fallback to disabled
But I don't know if there is much value on having a different behavior in debug for this function, so I skipped that. Opinion?
rcoh
left a comment
rcoh
left a comment
There was a problem hiding this comment.
look good! two small comments
| /// | --- | --- | --- | | ||
| /// | `DIAL9_S3_BUCKET` | unset | Upload sealed trace segments to this bucket. | | ||
| /// | `DIAL9_SERVICE_NAME` | binary name | Service name used in S3 keys and metadata. | | ||
| /// | `DIAL9_S3_PREFIX` | unset | Optional S3 object key prefix. | |
There was a problem hiding this comment.
lets default this to dial9-traces
There was a problem hiding this comment.
I did the change. Please note the following implications:
- The default is handled here, so it's different than the builder's one. I suppose that's good for backwards compatibility, so this is not important.
- My implementation reports all blank strings as invalid (to avoid hard to debug issues caused by "invisible" strings in configuration), so, with this change, there is no way to have "no prefix" through env vars. This is probably important. WDYT?
|
An unrelated CI job seems to be failing due to a missing |
|
@rcoh I noticed #350, which seems to propose a better default for Then, when that issue gets addressed in the builder API, we can just change |
2 participants
Description
This PR gathers inspiration from the proposal in its issue, from the
production_use.rsexample, from this real world usage and from the programmatic builder itself, to define a set of standard env vars and a function that reads them to launch dial9 with zero code.Out of scope
DIAL9_ENABLED_FILErequires a special setup for the watcher.Closes
#302