refactor(config): decouple monitoring config from amp-config schemars#1859
Merged
refactor(config): decouple monitoring config from amp-config schemars#1859
Conversation
Remove schemars dependency from the monitoring crate by duplicating OpenTelemetryConfig in amp-config with its own serde + schemars support. This is the first step toward making amp-config the sole owner of JSON schema generation, so internal crates no longer carry application-level concerns. - Add application-level OpenTelemetryConfig in amp-config with From impl - Change monitoring::init to accept impl Into<OpenTelemetryConfig> - Add monitoring::init_logging_only convenience function - Remove schemars feature propagation from amp-config to monitoring Part of #1790 (PR 1 of 2)
mitchhs12
force-pushed
the
mitchhs12/decouple-monitoring-config
branch
from
21b3153 to
8d016c3
Compare
LNSD
approved these changes
Feb 25, 2026
| [features] | ||
| # JSON Schema generation support for configuration validation | ||
| schemars = ["dep:schemars", "amp-worker-core/schemars", "monitoring/schemars"] | ||
| schemars = ["dep:schemars", "amp-worker-core/schemars"] |
Extract OpenTelemetryConfig into its own `monitoring` submodule per review feedback, keeping lib.rs clean. Rename the external monitoring crate dependency to `monitoring-crate` in Cargo.toml to avoid name collision with the local module.
LNSD
reviewed
Feb 25, 2026
crates/config/Cargo.toml
Outdated
| fs-err.workspace = true | ||
| metadata-db = { path = "../core/metadata-db" } | ||
| monitoring = { path = "../core/monitoring" } | ||
| monitoring-crate = { package = "monitoring", path = "../core/monitoring" } |
Contributor
There was a problem hiding this comment.
Suggested change
| monitoring-crate = { package = "monitoring", path = "../core/monitoring" } | |
| amp-monitoring = { package = "monitoring", path = "../core/monitoring" } |
LNSD
reviewed
Feb 25, 2026
crates/config/src/monitoring.rs
Outdated
| .map(|option| option.map(Duration::from_secs_f64)) | ||
| } | ||
|
|
||
| impl From<&OpenTelemetryConfig> for monitoring_crate::config::OpenTelemetryConfig { |
Contributor
There was a problem hiding this comment.
Suggested change
| impl From<&OpenTelemetryConfig> for monitoring_crate::config::OpenTelemetryConfig { | |
| impl From<&OpenTelemetryConfig> for amp_monitoring::config::OpenTelemetryConfig { |
LNSD
reviewed
Feb 25, 2026
crates/config/src/lib.rs
Outdated
| pub server_microbatch_max_interval: u64, | ||
| /// OpenTelemetry observability configuration. | ||
| pub opentelemetry: Option<OpenTelemetryConfig>, | ||
| pub opentelemetry: Option<monitoring::OpenTelemetryConfig>, |
Contributor
There was a problem hiding this comment.
Nit: Can we import it?
Suggested change
| pub opentelemetry: Option<monitoring::OpenTelemetryConfig>, | |
| pub opentelemetry: Option<OpenTelemetryConfig>, |
- Rename dependency alias from monitoring-crate to amp-monitoring - Import OpenTelemetryConfig in lib.rs instead of using qualified path - Update From impl to use amp_monitoring
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
2 participants
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.
Summary
schemarsdependency from themonitoringcrate by duplicatingOpenTelemetryConfiginamp-configwith its own serde + schemars supportmonitoring::initto acceptimpl Into<OpenTelemetryConfig>per the decoupling patternFrom<&_config::OpenTelemetryConfig> for monitoring::config::OpenTelemetryConfigmonitoring/schemarsfrom theamp-configfeature chainPart of #1790 (PR 1 of 2)