feat(agent-config): allow extensible configuration via ConfigExtension trait#111
Conversation
duncanista
requested review from
Lewis-E and
jchrostek-dd
and removed request for
a team
duncanista
force-pushed
the
jordan.gonzalez/config/allow-extensible-configuration
branch
from
9fe85fb to
7c6eb51
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
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.
duncanista
force-pushed
the
jordan.gonzalez/config/allow-extensible-configuration
branch
from
3c8af4d to
e8ba905
Compare
duncanpharvey
left a comment
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.
duncanista
deleted the
jordan.gonzalez/config/allow-extensible-configuration
branch
…nt-config (#1249) ## Overview First step of migrating bottlecap's in-tree configuration module onto the shared `datadog-agent-config` crate (lives in [DataDog/serverless-components](https://github.com/DataDog/serverless-components/tree/main/crates/datadog-agent-config)). This PR adds the foundation only — it introduces a `LambdaConfig` extension struct (in `bottlecap/src/config/mod.rs`, alongside the existing legacy code) that implements the upstream `ConfigExtension` trait, plus a `LambdaConfigSource` deserialization shape that figment uses for both env-var and YAML loading (dual extraction). Nothing in bottlecap consumes the extension yet — the existing `bottlecap::config::Config` struct is untouched. The follow-up #1251 (stacked on this) replaces it with `Config<LambdaConfig>`, deletes the duplicated env.rs / yaml.rs / deserializer modules, and removes the legacy `#[macro_export]` `merge_*` macros at the top of `mod.rs` (`LambdaConfig::merge_from` deliberately uses fully-qualified upstream macro paths today to coexist with them). ### Why this shape The upstream crate is purpose-built for this migration — see PR DataDog/serverless-components#111 ("ConfigExtension trait") which landed exactly the extensibility hook we need. Each consumer (bottlecap here, future serverless agents elsewhere) supplies its own extension type for fields that don't generalize, while sharing one canonical implementation for all the core agent fields (site, api_key, logs/APM/OTLP/proxy/trace propagation, etc.). ### Fields in the extension The 19 Lambda-specific fields that have no upstream equivalent: - `api_key_secret_arn`, `kms_api_key`, `api_key_ssm_arn`, `api_key_secret_reload_interval` - `serverless_logs_enabled` (OR-merged with the `DD_LOGS_ENABLED` alias) - `serverless_flush_strategy` (custom `FlushStrategy` deserializer for `"end" | "end,N" | "periodically,N" | "continuously,N"`) - `enhanced_metrics`, `lambda_proc_enhanced_metrics` - `capture_lambda_payload`, `capture_lambda_payload_max_depth` - `compute_trace_stats_on_extension`, `span_dedup_timeout` - `dd_org_uuid` (sourced from `DD_ORG_UUID`, source field `org_uuid` → config field `dd_org_uuid`) - `serverless_appsec_enabled`, `appsec_rules`, `appsec_waf_timeout` - `api_security_enabled`, `api_security_sample_delay` - `custom_metrics_exclude_tags` (sourced from `DD_LAMBDA_CUSTOMER_METRICS_EXCLUDE_TAGS` / `lambda_customer_metrics_exclude_tags:` in YAML) Kept in the extension rather than upstreamed because they're Lambda-runtime concerns. `custom_metrics_exclude_tags` is arguably generalizable to other serverless targets but stays here for now; we can migrate it upstream when another consumer needs it. ### Pre-requisites that already merged - DataDog/serverless-components#135 — bumps libdatadog rev across all serverless-components crates from `0a3304c` to `48da0d8` to match bottlecap. - DataDog/serverless-components#136 — switches `datadog-agent-config`'s libdd transitive deps to `default-features = false` and exposes `https` / `fips` opt-in features. Without this, the FIPS dep tree was poisoned by an implicit `https` (ring) path. Bottlecap pins all three serverless-components crates (`dogstatsd`, `datadog-fips`, `datadog-agent-config`) to the merge SHA `bb4dedee` so cargo deduplicates `dogstatsd`. ## Testing 37 new tests in `bottlecap/src/config/mod.rs::lambda_config_tests` cover each extension field from both env vars and YAML where applicable, plus: - Per-field env round-trip (`api_key_secret_arn`, `kms_api_key`, `api_key_ssm_arn`, …) - YAML coverage for the two source-to-config renames: `org_uuid` → `dd_org_uuid` and `lambda_customer_metrics_exclude_tags` → `custom_metrics_exclude_tags` - `DD_LOGS_ENABLED` ↔ `DD_SERVERLESS_LOGS_ENABLED` OR-merge semantics - `FlushStrategy` — `"end"`, `"end,N"` (`EndPeriodically`), `"periodically,N"`, `"continuously,N"`, invalid → `Default` - Duration parsing — seconds, microseconds, `_ignore_zero` variant - env precedence over YAML - Forgiving fallback when a single field is malformed (default preserved instead of failing the whole extraction) ``` $ cargo test --lib config::lambda_config_tests test result: ok. 37 passed; 0 failed; 0 ignored ``` Existing bottlecap config behavior is unchanged (this PR adds; it doesn't replace yet). ## Follow-up - #1251 — stacked on this branch — does the actual wire-in: replaces `bottlecap::config::Config` with `Config<LambdaConfig>`, deletes the duplicated config files, and removes the legacy `#[macro_export]` macros at the top of `mod.rs`.
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