feat(obs): unify metrics initialization and fix exporter move error #851
Conversation
- Fix Rust E0382 (use after move) by removing duplicate MetricExporter consumption. - Consolidate MeterProvider construction into single Recorder builder path. - Remove redundant Recorder::builder(...).install_global() call. - Ensure PeriodicReader setup is performed only once (HTTP + optional stdout). - Set global meter provider and metrics recorder exactly once. - Preserve existing behavior for stdout/file vs HTTP modes. - Minor cleanup: consistent resource reuse and interval handling.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned Files
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the observability crate to fix a Rust move error (E0382) in metrics initialization by replacing the external metrics-exporter-opentelemetry dependency with a custom Recorder implementation. The refactoring consolidates metric initialization into a single builder pattern, eliminating duplicate global recorder installations.
Key changes:
- Introduced a custom
Recorderimplementation with a builder pattern for unified metrics setup - Refactored metrics initialization to use
Recorder::builder().with_meter_provider()closure, preventing exporter double-consumption - Renamed
IS_OBSERVABILITY_ENABLEDtoOBSERVABILITY_METRIC_ENABLEDfor better clarity
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/obs/src/recorder.rs | New custom Recorder implementation with builder pattern for OpenTelemetry metrics integration |
| crates/obs/src/telemetry.rs | Refactored metrics initialization to use new Recorder builder, preventing exporter move error; simplified logging configuration |
| crates/obs/src/error.rs | Moved GlobalError enum from global.rs to dedicated error module with new SetRecorder variant |
| crates/obs/src/global.rs | Renamed observability flag and function; moved error types to separate module |
| crates/obs/src/system/mod.rs | Updated function call to use renamed observability_metric_enabled() |
| crates/obs/src/lib.rs | Added exports for new recorder and error modules |
| crates/obs/Cargo.toml | Removed metrics-exporter-opentelemetry dependency |
| Cargo.toml | Removed metrics-exporter-opentelemetry from workspace dependencies |
| Cargo.lock | Updated lock file to reflect dependency removal and cleanup unused transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
Type of Change
Related Issues
Summary of Changes
Summary
This PR refactors
telemetry.rsto eliminate a double-consumption of the OTLPMetricExporterthat triggered Rust error E0382 (value used after move). Metrics initialization is now performed through a singleRecorder::builderpath, avoiding redundant global installs.Key Changes
MeterProviderBuilderpath that consumed the exporter prematurely.PeriodicReaderinside theRecorder::builder(...).with_meter_providerclosure.Recorder::builder(...).install_global()invocation.global::set_meter_providerandmetrics::set_global_recorderare called exactly once.Resourceinstance across tracing, metrics, and logging.Fixes
Files Affected
crates/obs/src/telemetry.rsNo Behavior Regression
Startup counters and feature flags (
OBSERVABILITY_METRIC_ENABLED) remain intact.Testing
Existing tests pass (
recorder.rsand telemetry config tests). No new tests required as logic consolidation is internal.Next Steps
Optional: add integration test asserting single metric export pipeline creation.
Checklist
make pre-commitImpact
Additional Notes
Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.