-
Notifications
You must be signed in to change notification settings - Fork 6
Description
Problem
Internal crates (common, worker-core, monitoring) define configuration types that are directly imported and used by amp-config. This creates two problems:
-
Internal crates must not depend on
schemars— JSON schema generation is anampdconfig concern, not an internal crate concern. Currently,schemarspropagates into internal crates via the feature chain:amp-config[schemars]→amp-worker-core/schemars→common/schemarsandamp-config[schemars]→monitoring/schemars. -
Application-level defaults should be defined at the application level — Internal crates should define their own defaults independently. The
amp-configcrate should be able to define different defaults at the application-specific config level.
Approach
The Rust-idiomatic way to achieve this decoupling:
- Duplicate the config types in
amp-config. Define the defaults there, even if they are the same as those in the internal crate. - Do not use dependency types as config types. Always declare new types that wrap 3rd-party code in the interfaces of your code (App or API). A change in the dependency can cause a catastrophic effect in your app. After upgrading the dependency version, you should control the public API.
- In internal crates, declare function params as
config: impl Into<T>(whereTis the crate's own config type). - In
amp-config, implementimpl From<AmpConfigT> for CrateSpecificT.
Additional requirements
- Improve
amp-configfields' documentation (rustdocs). Make it more customer-facing, more thorough. For example, documenting theCompressionenum possible values and what is the purpose of that config param.
Affected Crates
Each crate below should be decoupled in its own PR, one crate at a time.
1. monitoring (crate: monitoring)
Types used by amp-config:
OpenTelemetryConfig— imported inconfig_file.rsandlib.rs
schemars coupling:
monitoring/Cargo.toml:schemars = { workspace = true, optional = true }monitoring/src/config.rs:#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]onOpenTelemetryConfig- Feature activated by:
amp-config[schemars]→monitoring/schemars
Decoupling work:
- Create a new
OpenTelemetryConfigtype inamp-configwith serde + schemars derives - Implement
impl From<amp_config::OpenTelemetryConfig> for monitoring::config::OpenTelemetryConfig - Remove
schemarsoptional dependency frommonitoring - Remove the
cfg_attr(feature = "schemars", ...)annotations frommonitoring::config
2. worker-core (crate: amp-worker-core)
Types used by amp-config:
ParquetConfig— imported inconfig_file.rs(forConfigFile.writer) andlib.rs(forConfig.parquet)ConfigDuration— imported inconfig_file.rs(forConfigFile.poll_interval_secs) andlib.rs(forWorkerEventsConfig.progress_interval)
schemars coupling:
worker-core/Cargo.toml:schemars = ["dep:schemars", "common/schemars"]— also propagates tocommon!worker-core/src/config.rs:schemarsannotations onParquetConfig,ConfigDuration,CollectorConfig,CompactorConfig,CompactionAlgorithmConfig,SizeLimitConfig- Feature activated by:
amp-config[schemars]→amp-worker-core/schemars→common/schemars
Decoupling work:
- Create duplicate config types in
amp-config:ParquetConfig,ConfigDuration,CollectorConfig,CompactorConfig,CompactionAlgorithmConfig,SizeLimitConfig - The
amp-configversions should have serde + schemars derives and customer-facing documentation - Implement
Fromconversions fromamp-configtypes toworker-coretypes - Make
worker-coreacceptimpl Into<worker_core::ParquetConfig>(and similar) where config is consumed - Remove
schemarsoptional dependency fromworker-core - Remove the
cfg_attr(feature = "schemars", ...)annotations fromworker-core::config - Do NOT use
datafusion::parquet::basic::Compressiondirectly inamp-config's config type — wrap it
3. common (crate: common)
Types used by amp-config (transitively via worker-core):
Overflow— used inworker-core::SizeLimitConfig.overflowfield
schemars coupling:
common/Cargo.toml:schemars = { workspace = true, optional = true }common/src/metadata/size.rs: manualimpl schemars::JsonSchema for Overflow- Feature activated by:
amp-worker-core[schemars]→common/schemars
Decoupling work:
- Remove the
schemarsoptional dependency fromcommon - Remove the
#[cfg(feature = "schemars")] impl schemars::JsonSchema for Overflowfromcommon/src/metadata/size.rs - The
Overflowschema will be defined in theamp-configversion ofSizeLimitConfiginstead (part ofworker-coredecoupling PR)
Note: This work may be absorbed into the
worker-corePR since the onlyschemarsusage incommonis forOverflow, which is transitively used throughworker-core. Alternatively it can be a standalone PR that just removes theschemarsdep fromcommonafterworker-coreno longer propagates the feature.
Dependency Graph (Current)
amp-config ──depends on──► amp-worker-core ──depends on──► common
│ │
│ └─ schemars feature propagates to common
│
└──depends on──► monitoring
amp-config[schemars] feature chain:
amp-config[schemars]
├── dep:schemars
├── amp-worker-core/schemars
│ ├── dep:schemars
│ └── common/schemars
│ └── dep:schemars
└── monitoring/schemars
└── dep:schemars
Dependency Graph (Target)
amp-config ──depends on──► amp-worker-core ──depends on──► common
│ (no schemars) (no schemars)
│
└──depends on──► monitoring
(no schemars)
amp-config[schemars] feature chain (target):
amp-config[schemars]
└── dep:schemars (only amp-config itself needs schemars)
Suggested PR Order
- PR 1:
monitoring— Smallest, simplest decoupling (1 type:OpenTelemetryConfig) - PR 2:
worker-core+common— Larger, handlesParquetConfig,ConfigDuration, and related types. Includes removingschemarsfromcommonsinceOverflowschemars impl is only needed through this chain.
Each PR should be independently reviewable and mergeable.