feat(agent-config): allow extensible configuration via ConfigExtension trait#111
Conversation
9fe85fb to
7c6eb51
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c6eb51564
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…n trait Introduces a generic `Config<E: ConfigExtension>` type that lets consumers define additional configuration fields without modifying or copy-pasting the core crate. Includes a unified `Source` type for dual extraction from both env vars and YAML, a `merge_fields!` macro to reduce merge boilerplate, and moves Lambda-specific fields out of the core Config struct. Also restructures the crate to use a conventional `src/` layout and adds a README documenting the extension API.
…s/ modules Move config source implementations (env, yaml) into `src/sources/` and type definitions with custom deserialization into `src/deserializers/`. Re-exports at the crate root preserve all existing import paths.
…zers/helpers.rs Extracts all generic deserializer functions (deserialize_optional_string, deserialize_with_default, duration parsers, key-value parsers, etc.) from lib.rs into src/deserializers/helpers.rs. Re-exported at the crate root so all existing import paths continue to work.
Reorganize lib.rs so an engineer opening the file immediately sees the Config struct and its fields, followed by the loading entry points, then the extension trait, builder, and macros. Sections are separated with headers for quick scanning.
Change NoExtensionSource from a unit struct to an empty struct so serde accepts map-shaped data from figment (env vars / YAML) instead of expecting null/unit. Prevents spurious warning logs on every get_config() call when no extension is used.
3c8af4d to
e8ba905
Compare
duncanpharvey
left a comment
There was a problem hiding this comment.
Approved with a few notes!
| ### Flat fields only | ||
|
|
||
| The single `Source` type is used for both env var and YAML extraction. This works when extension fields are top-level (flat) in the YAML file, which is the common case. If you need nested YAML structures that differ from the flat env var layout, implement `merge_from` with a nested source struct and handle the mapping manually. |
There was a problem hiding this comment.
Would having separate merge_from_env and merge_from_yaml resolve this issue for flat fields?
There was a problem hiding this comment.
Not really — the flat-field limitation isn't about the merge step, it's about the deserialization step. Figment uses a single key-value namespace per provider, so a flat Source struct works naturally for both env vars (DD_CUSTOM_FLAG=true) and YAML (custom_flag: true). Splitting into merge_from_env/merge_from_yaml wouldn't change the deserialization model.
The issue only shows up if you need nested YAML (e.g., lambda: { enhanced_metrics: true }) that maps to a flat env var (DD_ENHANCED_METRICS). For that case you'd need two different Source structs — one flat for env, one nested for YAML — which would mean two associated types on the trait. That's extra complexity we don't need yet since all current extension fields are flat.
If we hit that case later, we can add EnvSource/YamlSource associated types in a backward-compatible way. For now I'll update the doc to make this clearer.
| } | ||
|
|
||
| /// Source struct for deserialization. Must use #[serde(default)] and | ||
| /// graceful deserializers so one bad field doesn't fail the whole extraction. |
There was a problem hiding this comment.
Are there warning if #[serde(default)] or graceful deserializers aren't used? How can we make this clear to someone using the config extension in case they miss this note in the readme?
There was a problem hiding this comment.
Good call — there are runtime warnings but no compile-time enforcement. Here's what happens:
- Missing
#[serde(default)]: figment'sextract()fails because serde won't fill in defaults for absent fields → hits thetracing::warn!branch → extension falls back toE::default(). All extension fields silently get defaults. - Missing graceful deserializers: one malformed value (e.g.,
"yes"for abool) causes the whole extraction to fail → same warn + fallback behavior.
Both cases are silent in the sense that the extension consumer gets defaults without error, just a warning log. The README note is easy to miss.
I'll improve discoverability by:
- Adding a prominent
# Safety/ requirements section to theSourceassociated type doc (which IDEs surface on hover) - Explaining the failure mode directly ("extraction will fall back to defaults with a warning")
We can't enforce #[serde(default)] at compile time, but making the consequence clear in the type-level doc should help.
|
|
||
| #[allow(clippy::too_many_lines)] | ||
| fn merge_config(config: &mut Config, yaml_config: &YamlConfig) { | ||
| fn merge_config<E: ConfigExtension>(config: &mut Config<E>, yaml_config: &YamlConfig) { |
There was a problem hiding this comment.
Are there any consequences to having the same field defined in the extension as one that is already in the core config?
There was a problem hiding this comment.
No harmful consequences — the core and extension are extracted independently from the same figment, so both get their own copy of the value. The core field drives behavior as usual, and the extension field just has a duplicate. No errors, no data loss, no interference.
The main risk is confusion: someone might shadow api_key in their extension and expect it to override the core, but it wouldn't — config.api_key and config.ext.api_key would be independent copies.
I'll add a note about this in the trait doc.
Address reviewer feedback: document field name collision behavior, clarify Source type requirements and their runtime failure modes, and expand the README with collision and flat-field explanations.
Summary
Config<E: ConfigExtension = NoExtension>type that lets consumers define additional configuration fields without modifying or copy-pasting the core crateSourcetype for dual extraction from both env vars and YAML, and amerge_fields!macro to reduce merge boilerplatesrc/layout and adds a README documenting the extension APITest plan
cargo test -p datadog-agent-config)-D warningson lib and testscargo build --workspace)NoExtensionworks, extension receives env/yaml fields, env overrides yaml, defaults when not set, extension does not interfere with core fields