refactor(config): replace bottlecap Config with upstream Config<LambdaConfig>#1251
Open
duncanista wants to merge 2 commits into
Open
Conversation
duncanista
commented
Jun 10, 2026
duncanista
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed the migration diff (feat/use-datadog-agent-config-crate...feat/migrate-bottlecap-to-upstream-config). This is a mechanical refactor — Config → Config<LambdaConfig> with .ext. indirection on 19 Lambda fields, plus a PropConfig newtype to ferry the foreign Config<E> past the orphan rule. No correctness bugs found. The structural pieces (PropConfig impl, mod.rs rewrite, ~70 read-site rewrites, ~14 test struct-literal rewrites) all check out.
What I verified
- Field migration completeness. Grepped every one of the 19 Lambda field names across
bottlecap/src/andbottlecap/tests/. The only remaining bareconfig.<lambda_field>(no.ext.) accesses are insideLambdaConfig/LambdaConfigSourcestruct literals or comments. Nothing missed. - Test struct-literal rewrites. Every
Config { lambda_field: …, ..Default::default() }is correctly rewritten to nest the Lambda fields underext: LambdaConfig { …, ..Default::default() }with the outer..Config::default()(or..no_config.as_ref().clone()) preserved. Test intent is preserved in the two cases where a Lambda field was bundled with core fields (e.g.lifecycle/invocation/processor.rstest at the new line 2407 hasservice: Some("test-service".to_string())at the top level ANDserverless_appsec_enabled: truecorrectly placed underext:). metrics/enhanced/lambda.rstest_disabled / test_snapstart_restore_duration_metric_disabled. These overrideenhanced_metricsover a non-defaultno_configbase. The rewrite correctly uses..no_config.ext.clone()for the extension and..no_config.as_ref().clone()for the rest — both layers of "keep everything else from no_config" are preserved.mod.rsrewrite (2243 → 602 lines). Verified that nothing outside the deleted files references the droppedConfigBuilder/ConfigSource/ConfigError/merge_*macros. The re-exports cover every existingcrate::config::{env, flush_strategy, processing_rule, log_level, …}::Ximport I could find (lifecycle/invocation_times.rs,lifecycle/flush_control.rs,logs/processor.rs, etc.).PropConfigimpl parity. The newPropagationConfig for PropConfigimpl is a verbatim forward of the oldimpl PropagationConfig for Config(same emptiness checks, sameNonefor inject, same hard-coded512fordatadog_tags_max_length). Theself.0.fooderefs throughArc<Config>to the same fields as the priorself.fooaccess.- Stacked-PR base. Re-targeting onto main after #1249 lands should be clean — this PR's diff touches only files modified in this branch's HEAD commit (
e5cebae0), and #1249 doesn't touch the same lines in those files.
Findings are non-blocking nits — see inline comments.
duncanista
added a commit
that referenced
this pull request
Jun 10, 2026
Three non-blocking nits from the independent review: 1. PropConfig's inner Arc<Config> is no longer pub — keeps the wrapper boundary tight; the new() constructor is sufficient and no callers outside the module reach for the inner field directly. 2. PropConfig::new returns Arc<Self> instead of Self. Drops the redundant `Arc::new(...)` wrap at the single call site in traces/propagation/mod.rs. 3. Documents the hard-coded 512 in datadog_tags_max_length — it matches dd-trace-rs's default; bottlecap intentionally doesn't expose DD_TRACE_X_DATADOG_TAGS_MAX_LENGTH. Saves the next reader a round-trip through upstream to confirm parity. No behavior change. All 505 lib tests still pass.
e5cebae to
561096a
Compare
…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).
Three non-blocking nits from the independent review: 1. PropConfig's inner Arc<Config> is no longer pub — keeps the wrapper boundary tight; the new() constructor is sufficient and no callers outside the module reach for the inner field directly. 2. PropConfig::new returns Arc<Self> instead of Self. Drops the redundant `Arc::new(...)` wrap at the single call site in traces/propagation/mod.rs. 3. Documents the hard-coded 512 in datadog_tags_max_length — it matches dd-trace-rs's default; bottlecap intentionally doesn't expose DD_TRACE_X_DATADOG_TAGS_MAX_LENGTH. Saves the next reader a round-trip through upstream to confirm parity. No behavior change. All 505 lib tests still pass.
561096a to
a223aee
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Stacked on top of #1249. Completes the migration to the shared
datadog-agent-configcrate:bottlecap::config::Configis now a type alias fordatadog_agent_config::Config<LambdaConfig>, Lambda-specific fields live under.ext, and the 10 in-tree config submodules that duplicated upstream implementations are removed.Structural changes
bottlecap/src/config/mod.rsshrinks from 2243 → 602 lines. The legacyConfigstruct,ConfigBuilder,ConfigSource,ConfigError, the#[macro_export]merge_*macros, all per-field deserializer helpers, and the test module that mirrored upstream's behavior — all gone. What remains: atype Configalias, aget_config(path)wrapper, theLambdaConfigextension itself (from the parent PR), and re-exports of upstream modules under the samecrate::config::*paths so existing imports keep working.New module
bottlecap/src/config/propagation_wrapper.rsholds a newtypePropConfig(Arc<Config>)so we can implement the foreignPropagationConfigtrait on the foreignConfig<E>type without tripping Rust's orphan rule. The wrapper is scoped to the dd-trace-rs propagator boundary — nothing else uses it.bottlecap/src/traces/propagation/mod.rswraps the inner propagator inPropConfiginstead of passingConfigdirectly. All call sites that previously handedArc<Config>to the propagator are unchanged — the wrapping happens insideDatadogCompositePropagator::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~70 field-access sites referencing Lambda-specific fields (
config.api_key_secret_arn,config.serverless_logs_enabled,config.enhanced_metrics, etc.) updated to read throughconfig.ext.X. Test struct literals that constructedConfig { ... }with Lambda fields now nest them underext: LambdaConfig { ..., ..Default::default() }.What stays the same
LambdaConfigitself (and its 33 tests) — already shipped in the parent PR; no behavior change in this commit..extindirection is invisible to callers that hold anArc<Config>thanks to Rust's field-access auto-deref — they just go fromconfig.footoconfig.ext.foofor the 19 Lambda fields.Testing
Follow-ups
feat/use-datadog-agent-config-cratetomainonce feat(config): introduce extension config on top of shared datadog-agent-config #1249 merges.custom_metrics_exclude_tagsupstream so it becomes part of coreConfigrather than living in the Lambda extension (discussed in feat(config): introduce extension config on top of shared datadog-agent-config #1249).