feat(config): introduce extension config on top of shared datadog-agent-config#1249
feat(config): introduce extension config on top of shared datadog-agent-config#1249duncanista wants to merge 9 commits into
Conversation
Introduces a `LambdaExtension` struct that implements the upstream
`ConfigExtension` trait from `datadog-agent-config`, plus a
`LambdaSource` deserialization shape used for both env-var and YAML
loading via figment's dual-extraction.
This is the first step of migrating bottlecap's in-tree config module
onto the shared serverless-components `datadog-agent-config` crate.
The extension carries the 19 Lambda-specific fields with no upstream
equivalent (api_key_secret_arn, kms_api_key, api_key_ssm_arn,
serverless_logs_enabled, serverless_flush_strategy, enhanced_metrics,
lambda_proc_enhanced_metrics, capture_lambda_payload,
capture_lambda_payload_max_depth, compute_trace_stats_on_extension,
span_dedup_timeout, api_key_secret_reload_interval, dd_org_uuid,
serverless_appsec_enabled, appsec_rules, appsec_waf_timeout,
api_security_enabled, api_security_sample_delay,
custom_metrics_exclude_tags).
Behavior preserved end-to-end with 33 tests covering each field from
both DD_* env vars and datadog.yaml, plus:
- DD_LOGS_ENABLED <-> DD_SERVERLESS_LOGS_ENABLED OR-merge
- FlushStrategy ("end", "periodically,N", invalid -> Default)
- Duration parsing (seconds, microseconds, ignore-zero)
- org_uuid -> dd_org_uuid and lambda_customer_metrics_exclude_tags ->
custom_metrics_exclude_tags source-to-config field mappings
- env precedence over YAML
- Forgiving fallback when a single field is malformed
The dep is pinned to the pre-merge SHA of
DataDog/serverless-components#135 (libdatadog rev alignment) for
development; will be re-pinned to the merged SHA before opening
the migration PR.
Follow-ups (in subsequent commits):
- Replace bottlecap::config::Config with
datadog_agent_config::Config<LambdaExtension>
- Delete duplicated env.rs / yaml.rs / deserializer modules
DataDog/serverless-components#135 merged at aaac1a5d63faf664750a3bf51b50b79449a31625. The previous pin was the pre-merge branch SHA used during development.
|
duncanista
changed the title
Datadog-agent-config transitively pulls dogstatsd@aaac1a5d (uses parse_metric_namespace from it); bottlecap's existing dogstatsd pin was still at 5b68f50f. Cargo treated the two as separate packages, which broke the FIPS clippy job — the new transitive copy didn't inherit bottlecap's rustls/fips feature plumbing and ring ended up in the dependency graph. Bumping dogstatsd and datadog-fips to the same SHA collapses the duplicate, restoring a clean FIPS dependency tree.
… mod.rs `LambdaExtension` collides nominally with "the Datadog Lambda Extension" (this entire project). `LambdaConfig` reads more naturally for the extension type that holds Lambda-specific configuration fields. While here, fold the standalone lambda_extension.rs into config/mod.rs so consumers don't need a separate module hop. The inlined code uses fully-qualified `datadog_agent_config::merge_fields!` / `datadog_agent_config::merge_string!` invocations to coexist with the legacy `#[macro_export]` macros still at the top of mod.rs — both go away together once the migration onto the upstream Config lands. Renames carried through: LambdaExtension -> LambdaConfig LambdaSource -> LambdaConfigSource mod lambda_extension -> (gone; inlined) All 33 LambdaConfig tests still pass.
Bumps datadog-agent-config to the upstream PR branch SHA carrying DataDog/serverless-components#136 (which switches the libdd deps to default-features = false and exposes new https/fips features), and plumbs them through bottlecap's default/fips feature spec so the right TLS provider is selected per build. Without this, datadog-agent-config's libdd transitive dependencies implicitly enabled `https` (ring-backed hyper-rustls) on top of the fips path that bottlecap activates, leaving both providers in the dependency graph. The datadog-fips build script then rejected the build because ring v0.17 was reachable via: ring v0.17 -> libdd-common feature "https" -> hyper-rustls feature "ring" -> rustls-webpki feature "ring" With agent-config now opting in via consumer features instead of defaults, the FIPS dep tree is clean again. Also regenerated LICENSE-3rdparty.csv to include the new datadog-agent-config package, per the dd-rust-license-tool check. TODO: re-pin to the merge SHA once DataDog/serverless-components#136 lands.
Picks up the doc-comment clarification from the copilot review on DataDog/serverless-components#136 (https/fips features aren't Cargo-enforced).
DataDog/serverless-components#136 merged at bb4dedeee20b949db3143c05e5a779b843a8a484. The previous pin was the pre-merge branch SHA used during development.
duncanista
left a comment
duncanista
left a comment
There was a problem hiding this comment.
Took a look in isolation against main. A few real things — none are merge blockers, but a couple are worth a response. Grouped by severity.
Question (worth a reply)
1. Cargo.lock has two dogstatsd packages now. The PR description says "pins three serverless-components crates to the same SHA", but the diff actually pins them to two different SHAs:
dogstatsd -> aaac1a5
datadog-fips -> aaac1a5
datadog-agent-config -> bb4dede <-- different
Because datadog-agent-config depends on dogstatsd = { path = "../dogstatsd" }, cargo resolves the lockfile with two distinct dogstatsd source entries (visible in the lock diff). Inspecting bb4dede upstream, the only diff vs. aaac1a5 is the agent-config TLS-feature gating PR (#136) — the dogstatsd and datadog-fips source trees are byte-identical between the two SHAs.
Suggest bumping all three to bb4dede so cargo deduplicates dogstatsd and the description matches reality. No type-system risk today (upstream's public Config doesn't expose dogstatsd::* types — it just uses the crate internally), but it's bloat and the lockfile noise will recur on every dep bump.
2. capture_lambda_payload_max_depth: Option<u32> doesn't have a graceful deserializer. The upstream ConfigExtension doc (lib.rs:296-300) is explicit:
The
Sourcetype must use#[serde(default)]on the struct and graceful deserializers (e.g.,deserialize_optional_bool_from_anything) on each field. Without these, a missing or malformed value will cause the entire extension extraction to fail — the extension silently falls back toE::default()with atracing::warn!log.
Every other field in LambdaConfigSource opts into a deser_* helper; this one doesn't. A user setting DD_CAPTURE_LAMBDA_PAYLOAD_MAX_DEPTH=foo would silently reset all extension fields to defaults (api_key_secret_arn, kms_api_key, …) and emit only a warn. This is also a subtle regression vs. the in-tree path, where a bad u32 only fails the legacy env extraction, not the whole config.
The in-tree code has the same shape, so this is not new behavior — but the upstream contract makes the blast radius bigger. Worth either adding a deserialize_optional_u32_from_anything (probably belongs upstream alongside the bool/duration helpers) or documenting why the field is allowed to violate the contract.
Nits
3. Test coverage doesn't match what the PR description claims. The description says "Per-field env + YAML round-trip", but most fields have env-only tests:
- env-only, no YAML:
api_security_enabled,api_security_sample_delay,serverless_appsec_enabled,appsec_rules,enhanced_metrics,lambda_proc_enhanced_metrics,compute_trace_stats_on_extension,span_dedup_timeout,api_key_secret_reload_interval,dd_org_uuid,custom_metrics_exclude_tags
The two source-field-to-config-field renames are exactly the cases where a YAML test matters most:
org_uuid→dd_org_uuid: only tested via env (DD_ORG_UUID). If a user writesorg_uuid: ...indatadog.yaml, is that the right key?dd_org_uuid:? Neither test exists.lambda_customer_metrics_exclude_tags→custom_metrics_exclude_tags: same — only env-tested.
Either tighten the description to "env round-trip per field, plus YAML for a representative subset" or add the missing YAML tests for the renames at minimum.
4. FlushStrategy coverage is partial. Tests cover "end", "periodically,N", and invalid. Missing: "end,N" → EndPeriodically(N), "continuously,N" → Continuously(N). The upstream's enum implements all four variants; bottlecap will rely on all four behaviorally. The legacy bottlecap/src/config/flush_strategy.rs tested these — leaning entirely on upstream's unit tests for them at the bottlecap boundary is fine, just call that out.
5. PR description mentions bottlecap/src/config/lambda_extension.rs, but the code is appended to the bottom of mod.rs. The 19-field type + 33-test module ends up roughly 570 lines tacked onto an already-2,243-line file. Splitting into a sibling lambda_extension.rs would (a) match the description, (b) let pub mod lambda_extension; host the use datadog_agent_config::... imports without sharing scope with the legacy code, and (c) make the eventual deletion-of-legacy step a much cleaner diff.
6. Comment at line 1815-1818 mentions colliding with the in-tree merge_* macros declared #[macro_export] at the top of the file. Worth noting in the description that the migration plan will delete those #[macro_export]s — anyone in bottlecap can still write crate::merge_string! today and get the in-tree version. Non-blocking for this PR but easy to forget come PR #1251.
Three real fixes from independent review: 1. **Cargo.lock dedup** — bump dogstatsd and datadog-fips from `aaac1a5d` to `bb4dedee` so they share the same serverless-components SHA as datadog-agent-config. Otherwise cargo resolves two distinct `dogstatsd` source entries (the dep transitively from agent-config pulled in a second copy at the newer rev). Source trees for both crates are byte-identical between the two SHAs; only #136 changed between them. PR description matches reality now. 2. **`capture_lambda_payload_max_depth` graceful deserializer** — the field was missing `#[serde(deserialize_with = ...)]`, violating the upstream `ConfigExtension::Source` contract. Without it, a malformed env value would silently reset *all* extension fields to their defaults (api_key_secret_arn, kms_api_key, …) and emit only a `tracing::warn!`. Adds `deser_opt_lossless` which returns `None` on bad input so the rest of the extension keeps its values. 3. **Test coverage gaps**: - YAML round-trip for the two source-to-config renames (`org_uuid → dd_org_uuid` and `lambda_customer_metrics_exclude_tags → custom_metrics_exclude_tags`). These were the exact cases where YAML behavior matters most and they only had env tests. - `FlushStrategy::EndPeriodically` (`"end,N"`) and `FlushStrategy::Continuously` (`"continuously,N"`) — the upstream enum has all four variants and bottlecap will rely on them behaviorally; previously only `End` and `Periodically` had coverage. Test count: 33 → 37.
…aConfig>
Completes the migration started in the previous PR onto the shared
datadog-agent-config crate. bottlecap::config::Config is now a type
alias for datadog_agent_config::Config<LambdaConfig>; Lambda-specific
fields live under .ext (per the upstream ConfigExtension trait), and
the 10 in-tree config submodules that duplicated upstream
implementations are removed entirely.
What changes structurally:
- bottlecap/src/config/mod.rs shrinks from 2243 lines to ~600. The
legacy Config struct, ConfigBuilder, ConfigSource, ConfigError,
the #[macro_export] merge_* macros, all the per-field deserializer
helpers, and the entire test module that mirrored upstream's
behavior are gone. What remains: a `type Config` alias, a
`get_config(path)` wrapper, the LambdaConfig extension itself, and
re-exports of upstream modules under the same `crate::config::*`
paths so existing imports keep working.
- New module bottlecap/src/config/propagation_wrapper.rs holds a
newtype `PropConfig(Arc<Config>)` so we can implement the foreign
PropagationConfig trait on the foreign Config<E> type without
running afoul of Rust's orphan rule. The wrapper is scoped to the
dd-trace-rs propagator boundary; nothing else uses it.
- bottlecap/src/traces/propagation/mod.rs wraps the inner propagator
in PropConfig instead of passing Config directly. All call sites
that previously handed `Arc<Config>` to the propagator are
unchanged - the wrapping happens inside DatadogCompositePropagator::new.
- Deleted files (each redundant with upstream):
additional_endpoints.rs, apm_replace_rule.rs, env.rs,
flush_strategy.rs, log_level.rs, logs_additional_endpoints.rs,
processing_rule.rs, service_mapping.rs,
trace_propagation_style.rs, yaml.rs
- All ~70 field-access sites referencing Lambda-specific fields
(config.api_key_secret_arn, config.serverless_logs_enabled,
config.enhanced_metrics, etc.) updated to read through
config.ext.X. Test struct literals that construct Config { ... }
with Lambda fields now nest them under
`ext: LambdaConfig { ..., ..Default::default() }`.
What stays the same:
- LambdaConfig itself (and its 33 tests) - already shipped in the
parent PR; no behavior change in this commit.
- All other tests pass: 501 lib + 5 integration tests green.
- The .ext indirection is invisible to callers that hold an
`Arc<Config>` thanks to Rust's field-access auto-deref - they just
go from `config.foo` to `config.ext.foo` for the 19 Lambda fields.
Stacked on top of the LambdaConfig foundation PR (#1249).
There was a problem hiding this comment.
Pull request overview
Introduces a Lambda-specific configuration extension (LambdaConfig) compatible with the shared datadog-agent-config crate, establishing the foundation for migrating bottlecap’s in-tree config to the upstream config system without yet changing bottlecap’s legacy Config consumption.
Changes:
- Added
LambdaConfigimplementingdatadog_agent_config::ConfigExtension, plus a sharedLambdaConfigSourcedeserialization shape for env/YAML dual extraction and merge semantics. - Added a focused test module validating defaults, env/YAML parsing, precedence, alias OR-merge behavior, and forgiving deserialization fallbacks.
- Added the
datadog-agent-configgit dependency (and related features), and updatedCargo.lockand the 3rd-party license manifest accordingly.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
bottlecap/src/config/mod.rs |
Adds LambdaConfig/LambdaConfigSource, merge logic, and extension-focused tests to support the upcoming config migration. |
bottlecap/LICENSE-3rdparty.csv |
Registers datadog-agent-config in the third-party license list. |
bottlecap/Cargo.toml |
Adds datadog-agent-config dependency pinned to the serverless-components SHA and wires feature flags (https/fips). |
bottlecap/Cargo.lock |
Locks the new datadog-agent-config dependency and updates related serverless-components git sources. |
Per Copilot review on #1249. The existing `tests` module above already has `#[cfg_attr(coverage_nightly, coverage(off))]` to keep test code out of coverage metrics on nightly coverage builds; the new `lambda_config_tests` module should match.
…aConfig>
Completes the migration started in the previous PR onto the shared
datadog-agent-config crate. bottlecap::config::Config is now a type
alias for datadog_agent_config::Config<LambdaConfig>; Lambda-specific
fields live under .ext (per the upstream ConfigExtension trait), and
the 10 in-tree config submodules that duplicated upstream
implementations are removed entirely.
What changes structurally:
- bottlecap/src/config/mod.rs shrinks from 2243 lines to ~600. The
legacy Config struct, ConfigBuilder, ConfigSource, ConfigError,
the #[macro_export] merge_* macros, all the per-field deserializer
helpers, and the entire test module that mirrored upstream's
behavior are gone. What remains: a `type Config` alias, a
`get_config(path)` wrapper, the LambdaConfig extension itself, and
re-exports of upstream modules under the same `crate::config::*`
paths so existing imports keep working.
- New module bottlecap/src/config/propagation_wrapper.rs holds a
newtype `PropConfig(Arc<Config>)` so we can implement the foreign
PropagationConfig trait on the foreign Config<E> type without
running afoul of Rust's orphan rule. The wrapper is scoped to the
dd-trace-rs propagator boundary; nothing else uses it.
- bottlecap/src/traces/propagation/mod.rs wraps the inner propagator
in PropConfig instead of passing Config directly. All call sites
that previously handed `Arc<Config>` to the propagator are
unchanged - the wrapping happens inside DatadogCompositePropagator::new.
- Deleted files (each redundant with upstream):
additional_endpoints.rs, apm_replace_rule.rs, env.rs,
flush_strategy.rs, log_level.rs, logs_additional_endpoints.rs,
processing_rule.rs, service_mapping.rs,
trace_propagation_style.rs, yaml.rs
- All ~70 field-access sites referencing Lambda-specific fields
(config.api_key_secret_arn, config.serverless_logs_enabled,
config.enhanced_metrics, etc.) updated to read through
config.ext.X. Test struct literals that construct Config { ... }
with Lambda fields now nest them under
`ext: LambdaConfig { ..., ..Default::default() }`.
What stays the same:
- LambdaConfig itself (and its 33 tests) - already shipped in the
parent PR; no behavior change in this commit.
- All other tests pass: 501 lib + 5 integration tests green.
- The .ext indirection is invisible to callers that hold an
`Arc<Config>` thanks to Rust's field-access auto-deref - they just
go from `config.foo` to `config.ext.foo` for the 19 Lambda fields.
Stacked on top of the LambdaConfig foundation PR (#1249).
Overview
First step of migrating bottlecap's in-tree configuration module onto the shared
datadog-agent-configcrate (lives in DataDog/serverless-components).This PR adds the foundation only — it introduces a
LambdaConfigextension struct (inbottlecap/src/config/mod.rs, alongside the existing legacy code) that implements the upstreamConfigExtensiontrait, plus aLambdaConfigSourcedeserialization shape that figment uses for both env-var and YAML loading (dual extraction).Nothing in bottlecap consumes the extension yet — the existing
bottlecap::config::Configstruct is untouched. The follow-up #1251 (stacked on this) replaces it withConfig<LambdaConfig>, deletes the duplicated env.rs / yaml.rs / deserializer modules, and removes the legacy#[macro_export]merge_*macros at the top ofmod.rs(LambdaConfig::merge_fromdeliberately 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_intervalserverless_logs_enabled(OR-merged with theDD_LOGS_ENABLEDalias)serverless_flush_strategy(customFlushStrategydeserializer for"end" | "end,N" | "periodically,N" | "continuously,N")enhanced_metrics,lambda_proc_enhanced_metricscapture_lambda_payload,capture_lambda_payload_max_depthcompute_trace_stats_on_extension,span_dedup_timeoutdd_org_uuid(sourced fromDD_ORG_UUID, source fieldorg_uuid→ config fielddd_org_uuid)serverless_appsec_enabled,appsec_rules,appsec_waf_timeoutapi_security_enabled,api_security_sample_delaycustom_metrics_exclude_tags(sourced fromDD_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_tagsis 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
0a3304cto48da0d8to match bottlecap.datadog-agent-config's libdd transitive deps todefault-features = falseand exposeshttps/fipsopt-in features. Without this, the FIPS dep tree was poisoned by an implicithttps(ring) path.Bottlecap pins all three serverless-components crates (
dogstatsd,datadog-fips,datadog-agent-config) to the merge SHAbb4dedeeso cargo deduplicatesdogstatsd.Testing
37 new tests in
bottlecap/src/config/mod.rs::lambda_config_testscover each extension field from both env vars and YAML where applicable, plus:api_key_secret_arn,kms_api_key,api_key_ssm_arn, …)org_uuid→dd_org_uuidandlambda_customer_metrics_exclude_tags→custom_metrics_exclude_tagsDD_LOGS_ENABLED↔DD_SERVERLESS_LOGS_ENABLEDOR-merge semanticsFlushStrategy—"end","end,N"(EndPeriodically),"periodically,N","continuously,N", invalid →Default_ignore_zerovariantExisting bottlecap config behavior is unchanged (this PR adds; it doesn't replace yet).
Follow-up
bottlecap::config::ConfigwithConfig<LambdaConfig>, deletes the duplicated config files, and removes the legacy#[macro_export]macros at the top ofmod.rs.