fix: enhance OpenTelemetry configuration with vendor support and refactor tracing settings#29
Conversation
…actor tracing settings Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
📝 WalkthroughWalkthroughThe changes involve exposing a helper function for directory path validation and restructuring the CLI configuration system to separate OpenTelemetry concerns (resource, exporter, tracing, metrics) into a dedicated config struct while introducing vendor-specific configuration support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/cli/src/common.rs (1)
26-37: Document the CWD side effect now that this helper is public.Now that
parse_and_chdiris part of a broader API surface, add a rustdoc note that it mutates process-global state (set_current_dir) at parse time.Based on learnings: “`-p/--path` uses a custom `parse_and_chdir` parser that intentionally changes process CWD at argument-parse time.”Suggested patch
-pub fn parse_and_chdir(s: &str) -> Result<PathBuf, String> { +/// Parses a path and changes the process current working directory to it. +/// +/// # Side effects +/// This mutates process-global state via `std::env::set_current_dir`, which +/// can affect subsequent relative-path operations. +pub fn parse_and_chdir(s: &str) -> Result<PathBuf, String> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 26 - 37, Add a rustdoc note to the public function parse_and_chdir that clearly states it mutates process-global state by calling env::set_current_dir during argument parsing; update the documentation comment above the parse_and_chdir fn to mention that this parser will change the current working directory at parse time (used by -p/--path) so callers are aware of the side effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/config/app_config.rs`:
- Around line 21-23: AppConfig currently drops legacy top-level "tracing" data
because the new field is opentelemetry; implement a compatibility path in
AppConfig deserialization: detect if the incoming map contains a "tracing" key
and either migrate its fields into the new opentelemetry: OpenTelemetryConfig
structure or return a clear error; update the Deserialize impl for AppConfig (or
add a custom deserialize function used via #[serde(deserialize_with = "...")])
to inspect the raw map, perform the migration (mapping service_name, resource
attributes, metrics settings into opentelemetry), and ensure save_config writes
the migrated config, and add a unit test that loads a pre-refactor config
containing "tracing" to validate migration/error behavior.
- Around line 374-387: Change MetricsConfig.exporter from a concrete Exporter
with #[serde(default)] to Option<Exporter> (mirroring TracingConfig.exporter) so
metrics can distinguish "unset" vs "explicit default"; update
deserialization/merge logic in OpenTelemetryConfig (or wherever effective
exporter is resolved) to fall back to OpenTelemetryConfig.exporter when
metrics.exporter is None; remove the inappropriate #[serde(default)] on the
metrics exporter field and add a unit/config test that only sets
opentelemetry.exporter.kind to verify metrics inherits the shared exporter.
---
Nitpick comments:
In `@crates/cli/src/common.rs`:
- Around line 26-37: Add a rustdoc note to the public function parse_and_chdir
that clearly states it mutates process-global state by calling
env::set_current_dir during argument parsing; update the documentation comment
above the parse_and_chdir fn to mention that this parser will change the current
working directory at parse time (used by -p/--path) so callers are aware of the
side effect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89c2ede3-c83a-4523-9bc7-f211cd02dc53
📒 Files selected for processing (2)
crates/cli/src/common.rscrates/cli/src/config/app_config.rs
| /// Top-level OpenTelemetry configuration grouping resource identity, | ||
| /// a shared default exporter, tracing settings and metrics settings. | ||
| #[derive(Clone, Deserialize, Serialize, Default)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct OpenTelemetryConfig { | ||
| #[serde(default)] | ||
| pub resource: OpenTelemetryResource, | ||
| /// Default exporter shared by tracing and metrics. Per-signal `exporter` | ||
| /// fields override this when present. | ||
| pub exporter: Option<Exporter>, | ||
| #[serde(default)] | ||
| pub tracing: TracingConfig, | ||
| #[serde(default)] | ||
| pub metrics: MetricsConfig, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/cli/src/config/app_config.rs | sed -n '360,410p'Repository: cyberfabric/cf-cli
Length of output: 2211
🏁 Script executed:
# Find MetricsConfig definition
rg "struct MetricsConfig" -A 20Repository: cyberfabric/cf-cli
Length of output: 1331
🏁 Script executed:
# Find TracingConfig definition to see if it has the same pattern
rg "struct TracingConfig" -A 20Repository: cyberfabric/cf-cli
Length of output: 1610
🏁 Script executed:
# Check for any existing fallback/inheritance logic related to exporter
rg "exporter" crates/cli/src/config/app_config.rs -B 2 -A 2Repository: cyberfabric/cf-cli
Length of output: 962
🏁 Script executed:
# Look for any implementation code that handles exporter fallback
rg "OpenTelemetryConfig" -A 5 -B 5 | head -100Repository: cyberfabric/cf-cli
Length of output: 2579
🏁 Script executed:
# Search for tests related to OpenTelemetry config
fd "test" --type f | xargs rg -l "OpenTelemetryConfig|MetricsConfig" 2>/dev/null | head -5Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check if there are any impl blocks for OpenTelemetryConfig
ast-grep --pattern $'impl OpenTelemetryConfig {
$$$
}'Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Look for where these configs are actually used/instantiated
rg "metrics\.exporter|tracing\.exporter" -B 2 -A 2Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Look for all test files
find . -name "*test*" -type f | grep -E "\.(rs|toml)$" | head -20Repository: cyberfabric/cf-cli
Length of output: 102
🏁 Script executed:
# Search for test modules in the config crate
rg "#\[cfg\(test\)\]|#\[test\]" crates/cli/src/config/ -A 3Repository: cyberfabric/cf-cli
Length of output: 1871
🏁 Script executed:
# Look for where OpenTelemetry setup happens (likely in a separate init/setup file)
fd -e rs | xargs rg -l "TracingConfig|MetricsConfig" | grep -v app_configRepository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check the actual usage of these configs - look for where metrics exporter is extracted/used
rg "\.metrics" crates/cli/src/ -B 2 -A 2 | head -60Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Search for where telemetry is actually initialized or used
rg "otel|telemetry|instrumentation" crates/cli/src/ -l | head -10Repository: cyberfabric/cf-cli
Length of output: 207
🏁 Script executed:
# Look for any mention of metrics or tracing initialization
rg "metrics_config|tracing_config|otel_config" crates/cli/src/ -B 2 -A 2 | head -80Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check if there's a separate module for telemetry setup
find crates/cli/src -name "*.rs" | xargs grep -l "metrics\|tracing" | grep -v ".git"Repository: cyberfabric/cf-cli
Length of output: 122
🏁 Script executed:
# Search more broadly for where the config fields are accessed
rg "\.exporter" crates/cli/src/ -B 3 -A 3Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Look at run_loop.rs to see how telemetry config is used
cat -n crates/cli/src/run/run_loop.rs | head -150Repository: cyberfabric/cf-cli
Length of output: 7688
🏁 Script executed:
# Check for any exporter resolution or fallback logic
rg "opentelemetry.exporter|otel.*exporter" crates/ -iRepository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Look at how metrics config is actually used in run_loop
rg "metrics" crates/cli/src/run/run_loop.rs -B 3 -A 3Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Search for any function that might handle the exporter inheritance
rg "fn.*exporter|exporter.*fn" crates/cli/src/ -i -B 1 -A 5Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check common.rs for telemetry/config setup
cat -n crates/cli/src/common.rs | head -200Repository: cyberfabric/cf-cli
Length of output: 7135
🏁 Script executed:
# Search for where metrics exporter is actually instantiated or used
rg "MetricsConfig" crates/cli/src/ -B 5 -A 5Repository: cyberfabric/cf-cli
Length of output: 1544
🏁 Script executed:
# Look for any builder or post-processing after deserialization
rg "from_config|with_exporter|exporter.*=|metrics.*=" crates/cli/src/ -iRepository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Look for modkit usage - this is where the config is actually applied
rg "run_server|AppConfig" crates/cli/src/ -B 2 -A 5Repository: cyberfabric/cf-cli
Length of output: 7888
🏁 Script executed:
# Check if there's a specific handler for exporter in metrics setup
rg "impl.*MetricsConfig|fn.*metrics" crates/cli/src/ -B 2 -A 8Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Look for where Exporter::default is used or referenced
rg "Exporter::default|Default for.*Exporter" crates/cli/src/Repository: cyberfabric/cf-cli
Length of output: 44
🏁 Script executed:
# Check the Exporter struct more carefully
rg "struct Exporter" -A 15 crates/cli/src/config/Repository: cyberfabric/cf-cli
Length of output: 1172
Change MetricsConfig.exporter to Option<Exporter> to inherit the shared default exporter.
MetricsConfig.exporter is currently a concrete Exporter type with #[serde(default)], which prevents the field from distinguishing "unset, inherit the shared exporter" from "explicitly configured as default". After deserialization, metrics always has Exporter::default(), breaking the documented design that per-signal fields can override opentelemetry.exporter when present.
This differs from TracingConfig.exporter, which correctly uses Option<Exporter> to support inheritance. A config that only sets opentelemetry.exporter.kind will therefore lose that shared kind for metrics.
Shape change
pub struct MetricsConfig {
#[serde(default)]
pub enabled: bool,
- #[serde(default)]
- pub exporter: Exporter,
+ #[serde(default, skip_serializing_if = "Option::is_none")]
+ pub exporter: Option<Exporter>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub cardinality_limit: Option<usize>,
}Resolve the effective exporter after deserialization by falling back to OpenTelemetryConfig.exporter when metrics.exporter is None. Add a config test with only opentelemetry.exporter populated to prevent regression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cli/src/config/app_config.rs` around lines 374 - 387, Change
MetricsConfig.exporter from a concrete Exporter with #[serde(default)] to
Option<Exporter> (mirroring TracingConfig.exporter) so metrics can distinguish
"unset" vs "explicit default"; update deserialization/merge logic in
OpenTelemetryConfig (or wherever effective exporter is resolved) to fall back to
OpenTelemetryConfig.exporter when metrics.exporter is None; remove the
inappropriate #[serde(default)] on the metrics exporter field and add a
unit/config test that only sets opentelemetry.exporter.kind to verify metrics
inherits the shared exporter.
1 participant
Summary by CodeRabbit