fix: cleanup metrics and observability setup and communication with the TUI - #1164
fix: cleanup metrics and observability setup and communication with the TUI#1164KtorZ wants to merge 1 commit into
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (65)
WalkthroughThe change refactors observability around a shared meter and direct TUI callbacks, expands system metrics, replaces TUI-local sampling with exported metrics, adds bounded and smoothed model state, moves era-history loading onto ChangesObservability and TUI metrics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Main
participant Session
participant Observability
participant Meter
participant Model
Main->>Session: create embedded TUI session
Main->>Observability: configure session tracing and metrics adapters
Observability->>Meter: create resource-backed OTLP providers
Meter->>Session: send MetricsEvent through local observer
Session->>Model: enqueue telemetry or metrics message
Model->>Model: update logs, rates, and system state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru-tui/src/model.rs (1)
223-242: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove stale
global_sectionsinitializers.Line 240 initializes
StartupContext::global_sections, but the suppliedStartupContextdefinition has no such field. This prevents the test module from compiling. Remove this initializer and the same stale initializer at Line 685.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-tui/src/model.rs` around lines 223 - 242, Remove the obsolete global_sections initializer from fixture_startup_context and the corresponding StartupContext construction around the other referenced location. Keep all valid StartupContext fields unchanged so the test module matches the current struct definition and compiles.Source: Coding guidelines
🧹 Nitpick comments (2)
crates/amaru-kernel/src/cardano/era_history.rs (1)
428-452: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the legacy era history loader for this non-major release.
For
0.1.2, keepload_era_history_from_fileas a deprecated forwarding function that callsEraHistory::load, and keep its crate-root re-export until a semver-major release. Add a regression test for the forwarding API so this compatibility shim is not forgotten.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-kernel/src/cardano/era_history.rs` around lines 428 - 452, Retain the legacy load_era_history_from_file function as a deprecated forwarding shim that delegates to EraHistory::load, and preserve its crate-root re-export for this non-major release. Add a regression test covering the legacy forwarding API and its successful file-loading behavior.crates/amaru/src/lifecycle.rs (1)
86-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the numbered execution procedure.
Describe the lifecycle contract without numbered steps. Keep the API references if they help callers.
As per coding guidelines, “Do not reference implementation plans, step numbers, ticket IDs, or in-progress refactor names in code comments.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/lifecycle.rs` around lines 86 - 91, Update the documentation comment for Runnable to remove the numbered execution procedure and describe the lifecycle contract in concise, non-sequenced prose. Retain the Runnable::soft, Runnable::exit_on_signal, build_runtime, and run_on API references where useful, without describing implementation steps or ordering.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/amaru-tui/README.md`:
- Around line 18-19: Update the descriptions for telemetry_update.rs and
metrics_update.rs in the README to state that they process in-process tracing
records and local MetricsEvent callbacks, respectively, matching the terminology
used on lines 38-40; remove the incorrect OTLP-derived wording.
In `@crates/amaru-tui/src/startup.rs`:
- Around line 35-36: Update every StartupContext struct literal, especially
fixture_startup_context in model.rs, to remove the obsolete global_sections
field and retain only the current runtime_sections and protocol_sections fields
so amaru-tui compiles.
---
Outside diff comments:
In `@crates/amaru-tui/src/model.rs`:
- Around line 223-242: Remove the obsolete global_sections initializer from
fixture_startup_context and the corresponding StartupContext construction around
the other referenced location. Keep all valid StartupContext fields unchanged so
the test module matches the current struct definition and compiles.
---
Nitpick comments:
In `@crates/amaru-kernel/src/cardano/era_history.rs`:
- Around line 428-452: Retain the legacy load_era_history_from_file function as
a deprecated forwarding shim that delegates to EraHistory::load, and preserve
its crate-root re-export for this non-major release. Add a regression test
covering the legacy forwarding API and its successful file-loading behavior.
In `@crates/amaru/src/lifecycle.rs`:
- Around line 86-91: Update the documentation comment for Runnable to remove the
numbered execution procedure and describe the lifecycle contract in concise,
non-sequenced prose. Retain the Runnable::soft, Runnable::exit_on_signal,
build_runtime, and run_on API references where useful, without describing
implementation steps or ordering.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 047cc0d4-8e69-4e3c-a0fb-fd13c9135143
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.github/workflows/nightly-tool-integrations.yml.github/workflows/template-ledger-epoch-snapshots.ymlCHANGELOG.mdCargo.tomlcrates/amaru-kernel/src/cardano/era_history.rscrates/amaru-kernel/src/lib.rscrates/amaru-kernel/src/utils.rscrates/amaru-kernel/src/utils/process.rscrates/amaru-metrics/src/consensus.rscrates/amaru-metrics/src/ledger.rscrates/amaru-metrics/src/lib.rscrates/amaru-metrics/src/mempool.rscrates/amaru-metrics/src/metrics.rscrates/amaru-metrics/src/protocol.rscrates/amaru-metrics/src/system.rscrates/amaru-node/Cargo.tomlcrates/amaru-node/src/stages/build_node.rscrates/amaru-node/src/tests/setup.rscrates/amaru-observability/macros/src/define_schemas.rscrates/amaru-observability/macros/src/traces.rscrates/amaru-observability/tests/schema_access.rscrates/amaru-protocols/src/metrics_effects.rscrates/amaru-tui/AGENTS.mdcrates/amaru-tui/Cargo.tomlcrates/amaru-tui/README.mdcrates/amaru-tui/src/capture.rscrates/amaru-tui/src/events.rscrates/amaru-tui/src/events/field_value.rscrates/amaru-tui/src/events/host_sample.rscrates/amaru-tui/src/events/message.rscrates/amaru-tui/src/events/telemetry_kind.rscrates/amaru-tui/src/events/telemetry_record.rscrates/amaru-tui/src/host_metrics.rscrates/amaru-tui/src/lib.rscrates/amaru-tui/src/metrics.rscrates/amaru-tui/src/model.rscrates/amaru-tui/src/model/interaction.rscrates/amaru-tui/src/model/metrics_update.rscrates/amaru-tui/src/model/telemetry_update.rscrates/amaru-tui/src/model/tip_state.rscrates/amaru-tui/src/session.rscrates/amaru-tui/src/startup.rscrates/amaru-tui/src/ui/components/logs.rscrates/amaru-tui/src/ui/screens/amaru.rscrates/amaru-tui/src/ui/screens/config.rscrates/amaru/src/bin/amaru/cmd/node/run.rscrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/amaru/pid.rscrates/amaru/src/lifecycle.rscrates/amaru/src/metrics.rscrates/amaru/src/observability.rsengineering-decision-records/030-embedded-terminal-observability-ui.mdmonitoring/Makefilemonitoring/README.mdmonitoring/docker-compose.ymlmonitoring/otlp-collector.yml
💤 Files with no reviewable changes (10)
- monitoring/otlp-collector.yml
- monitoring/docker-compose.yml
- crates/amaru-tui/src/metrics.rs
- crates/amaru-tui/src/lib.rs
- monitoring/Makefile
- crates/amaru-tui/src/events/telemetry_kind.rs
- crates/amaru-tui/src/host_metrics.rs
- crates/amaru-tui/Cargo.toml
- crates/amaru-tui/src/events/host_sample.rs
- .github/workflows/template-ledger-epoch-snapshots.yml
| - `telemetry_update.rs`: folds OTLP-derived telemetry into model state | ||
| - `metrics_update.rs`: folds OTLP-derived metrics into UI state |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the metric-source description.
telemetry_update.rs receives in-process tracing records. metrics_update.rs receives local MetricsEvent callbacks. Neither reducer consumes OTLP-derived data in this embedded path. Use the same terms as lines 38-40.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/amaru-tui/README.md` around lines 18 - 19, Update the descriptions for
telemetry_update.rs and metrics_update.rs in the README to state that they
process in-process tracing records and local MetricsEvent callbacks,
respectively, matching the terminology used on lines 38-40; remove the incorrect
OTLP-derived wording.
1117eff to
9e62067
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…he TUI No-changelog Signed-off-by: KtorZ <matthias.benkort@gmail.com>
9e62067 to
a033158
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
crates/amaru-ledger/src/store/columns/proposals.rs (1)
55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReorder the test imports by crate group.
proptestis an external crate.amaru_kernelis a workspace crate. Placeproptestbeforeamaru_kernel, then keepsuper::*after both groups.Suggested change
- use amaru_kernel::{any_proposal, any_proposal_pointer}; use proptest::{prelude::*, prop_compose}; + use amaru_kernel::{any_proposal, any_proposal_pointer}; use super::*;As per coding guidelines: order imports as standard library, external crates, workspace/project crates, then self/super/crate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-ledger/src/store/columns/proposals.rs` around lines 55 - 58, Reorder the imports in the test module so the external proptest import appears before the workspace amaru_kernel import, with super::* remaining after both crate groups.Source: Coding guidelines
crates/amaru-tui/src/model/exponential_moving_average.rs (1)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a short doc comment for the
smoothingunit.
smoothingis a span-style parameter that maps toalpha = 2 / (N + 1), and1means "follow the newest sample". That is not obvious from the call sites, where it arrives aspeer_timing_capacityorblock_sample_capacity, which read like buffer sizes. A one-line///would save the next reader a trip through the source, and the project guidelines do ask for doc comments on non-trivial APIs and units.As per coding guidelines: "Use doc comments for non-trivial APIs, invariants, units, outcomes, and side effects."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-tui/src/model/exponential_moving_average.rs` around lines 25 - 31, Add a concise doc comment to the public ExponentialMovingAverage::record method documenting that smoothing is the span used to calculate alpha via 2/(N+1), with 1 meaning the newest sample is followed directly.Source: Coding guidelines
crates/amaru/src/bin/amaru/cmd/node/run.rs (1)
284-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe era history file gets read twice on startup.
tui_settingscallsEraHistory::load(path)andparse_args(Line 513-518) loads the same file again. The silent.ok()here is safe, becauseparse_argsstill fails loudly on a bad file. It is just a second disk read plus a second parse of the same JSON, a bit like buying two tickets for the one movie. If you want it tighter, load once and share the value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bin/amaru/cmd/node/run.rs` around lines 284 - 294, Update the node startup flow around the displayed protocol-parameter construction and parse_args/tui_settings handling so EraHistory::load is performed once and the resulting value is reused for both consumers. Preserve the existing failure behavior from parse_args for invalid files while eliminating the duplicate disk read and JSON parse.crates/amaru-tui/src/ui/screens/amaru.rs (2)
260-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese helpers are now pure pass-throughs.
blocks_per_secondandtransactions_per_seconddo nothing except forward to the model methods of the same name. You could callmodel.blocks_per_second()at Line 96-97 and drop both. Keep them if the tests find the free functions handier, no drama either way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-tui/src/ui/screens/amaru.rs` around lines 260 - 266, Remove the redundant blocks_per_second and transactions_per_second free-function wrappers, and update their call sites around the referenced UI code to invoke model.blocks_per_second() and model.transactions_per_second() directly. Retain the wrappers only if existing tests require the free-function API.
290-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test only covers the first EMA sample.
Both assertions land on the seeding path, where
ExponentialMovingAverage::recordtakes theNonebranch and stores the sample verbatim. The smoothing maths atalpha * sample + (1 - alpha) * valuenever runs, so the exact-equality asserts pass for a reason that has nothing to do with the EMA. A thirdrecordcall with a known alpha would actually put the moving average through its paces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-tui/src/ui/screens/amaru.rs` around lines 290 - 311, Add a third timestamped sample to the EMA setup in throughput_uses_exponential_moving_average for both block_rate and transaction_rate, then update the expected blocks_per_second and transactions_per_second assertions to the values produced by the smoothing formula. Ensure the added samples exercise ExponentialMovingAverage::record after initialization rather than the initial seeding path.crates/amaru-tui/src/model/log_buffer.rs (1)
50-52: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the eviction loop against a counter/storage mismatch.
pushloopswhile self.count(level) >= capacity, andevict_oldestreturns early without decrementing when it finds no record of that level inall. Today the invariant holds, because every counted record is also inall, so the early return cannot fire while the count is positive. But if a future change ever breaks that pairing, this becomes an infinite loop on the render thread, and the TUI freezes like a bad save state.Decrementing on the early-return path makes the loop terminate regardless.
🛡️ Proposed defensive fix
fn evict_oldest(&mut self, level: Level) { let Some(position) = self.all.iter().position(|record| record.level == level) else { + self.decrement(level); return; };Also applies to: 72-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-tui/src/model/log_buffer.rs` around lines 50 - 52, Update evict_oldest so its early-return path decrements the per-level count when no matching record exists in all, ensuring the while loop in push can always make progress despite a counter/storage mismatch. Preserve the existing eviction behavior when a record is found.crates/amaru-tui/src/model/peer_state.rs (1)
112-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
smoothingargument still comes from a field named*_capacity.The parameter rename to
smoothingis the right call, but the caller incrates/amaru-tui/src/model/telemetry_update.rsbindslet capacity = self.config.peer_timing_capacity;and hands that straight in. Same story forblock_sample_capacityandtransaction_sample_capacityfeedingRateCounter::new. Nothing is buffered anymore, so "capacity" is now a bit of a ghost in the machine.Renaming the
Configfields to*_smoothingwould make the whole chain read honestly. It touches a few files, so it is fine to defer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-tui/src/model/peer_state.rs` around lines 112 - 120, Rename the configuration fields for peer timing, block sample, and transaction sample from *_capacity to *_smoothing, then update all references—including telemetry_update callers and RateCounter::new arguments—to use the new names consistently. Preserve the existing configured smoothing values and behavior.crates/amaru-tui/src/model/telemetry_update.rs (1)
265-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the generated
outcomeaccessor for header lifecycle checks.The generated schema defines
outcomeas an optional field, soconsensus::perf::header::LIFECYCLE::outcome(record)can replacerecord.str(consensus::perf::header::LIFECYCLE::FIELD_OUTCOME). Keep the"valid"comparison, but stop going raw-string and drop theRecordFieldsimport if this is the only remaining direct accessor. Like a clean cutscene, no extra frames needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-tui/src/model/telemetry_update.rs` around lines 265 - 267, In the header lifecycle validation, replace the raw FIELD_OUTCOME lookup in the surrounding telemetry update logic with the generated consensus::perf::header::LIFECYCLE::outcome accessor, preserving the optional-value comparison against "valid". Remove the RecordFields import if no other direct record access remains.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/amaru-tui/src/config.rs`:
- Around line 58-67: Update Config::log_capacity_for to add a catch-all Level(_)
match arm, preserving the existing capacity selection for known TRACE, DEBUG,
INFO, WARN, and ERROR levels. Apply the same exhaustive-match fix in the
corresponding log-level match within the log buffer implementation.
In `@crates/amaru-tui/src/session.rs`:
- Around line 143-153: Update the telemetry receive loop around recv_timeout so
Err(RecvTimeoutError::Disconnected) exits the loop immediately, while preserving
the existing timeout behavior and message handling for connected receivers.
---
Nitpick comments:
In `@crates/amaru-ledger/src/store/columns/proposals.rs`:
- Around line 55-58: Reorder the imports in the test module so the external
proptest import appears before the workspace amaru_kernel import, with super::*
remaining after both crate groups.
In `@crates/amaru-tui/src/model/exponential_moving_average.rs`:
- Around line 25-31: Add a concise doc comment to the public
ExponentialMovingAverage::record method documenting that smoothing is the span
used to calculate alpha via 2/(N+1), with 1 meaning the newest sample is
followed directly.
In `@crates/amaru-tui/src/model/log_buffer.rs`:
- Around line 50-52: Update evict_oldest so its early-return path decrements the
per-level count when no matching record exists in all, ensuring the while loop
in push can always make progress despite a counter/storage mismatch. Preserve
the existing eviction behavior when a record is found.
In `@crates/amaru-tui/src/model/peer_state.rs`:
- Around line 112-120: Rename the configuration fields for peer timing, block
sample, and transaction sample from *_capacity to *_smoothing, then update all
references—including telemetry_update callers and RateCounter::new arguments—to
use the new names consistently. Preserve the existing configured smoothing
values and behavior.
In `@crates/amaru-tui/src/model/telemetry_update.rs`:
- Around line 265-267: In the header lifecycle validation, replace the raw
FIELD_OUTCOME lookup in the surrounding telemetry update logic with the
generated consensus::perf::header::LIFECYCLE::outcome accessor, preserving the
optional-value comparison against "valid". Remove the RecordFields import if no
other direct record access remains.
In `@crates/amaru-tui/src/ui/screens/amaru.rs`:
- Around line 260-266: Remove the redundant blocks_per_second and
transactions_per_second free-function wrappers, and update their call sites
around the referenced UI code to invoke model.blocks_per_second() and
model.transactions_per_second() directly. Retain the wrappers only if existing
tests require the free-function API.
- Around line 290-311: Add a third timestamped sample to the EMA setup in
throughput_uses_exponential_moving_average for both block_rate and
transaction_rate, then update the expected blocks_per_second and
transactions_per_second assertions to the values produced by the smoothing
formula. Ensure the added samples exercise ExponentialMovingAverage::record
after initialization rather than the initial seeding path.
In `@crates/amaru/src/bin/amaru/cmd/node/run.rs`:
- Around line 284-294: Update the node startup flow around the displayed
protocol-parameter construction and parse_args/tui_settings handling so
EraHistory::load is performed once and the resulting value is reused for both
consumers. Preserve the existing failure behavior from parse_args for invalid
files while eliminating the duplicate disk read and JSON parse.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb400a3b-ef68-4d13-8647-17d0474c2394
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (65)
.github/workflows/nightly-tool-integrations.yml.github/workflows/template-ledger-epoch-snapshots.ymlCHANGELOG.mdCargo.tomlcrates/amaru-kernel/src/cardano/era_history.rscrates/amaru-kernel/src/lib.rscrates/amaru-kernel/src/utils.rscrates/amaru-kernel/src/utils/process.rscrates/amaru-ledger/src/store/columns/proposals.rscrates/amaru-metrics/src/consensus.rscrates/amaru-metrics/src/ledger.rscrates/amaru-metrics/src/lib.rscrates/amaru-metrics/src/mempool.rscrates/amaru-metrics/src/metrics.rscrates/amaru-metrics/src/protocol.rscrates/amaru-metrics/src/system.rscrates/amaru-node/Cargo.tomlcrates/amaru-node/src/stages/build_node.rscrates/amaru-node/src/tests/setup.rscrates/amaru-observability/macros/src/define_schemas.rscrates/amaru-observability/macros/src/traces.rscrates/amaru-observability/tests/schema_access.rscrates/amaru-protocols/src/metrics_effects.rscrates/amaru-stores/src/lib.rscrates/amaru-stores/src/rocksdb/consensus/tests.rscrates/amaru-tui/AGENTS.mdcrates/amaru-tui/Cargo.tomlcrates/amaru-tui/README.mdcrates/amaru-tui/src/capture.rscrates/amaru-tui/src/config.rscrates/amaru-tui/src/events.rscrates/amaru-tui/src/events/field_value.rscrates/amaru-tui/src/events/host_sample.rscrates/amaru-tui/src/events/message.rscrates/amaru-tui/src/events/telemetry_record.rscrates/amaru-tui/src/host_metrics.rscrates/amaru-tui/src/lib.rscrates/amaru-tui/src/metrics.rscrates/amaru-tui/src/model.rscrates/amaru-tui/src/model/exponential_moving_average.rscrates/amaru-tui/src/model/interaction.rscrates/amaru-tui/src/model/log_buffer.rscrates/amaru-tui/src/model/metrics_update.rscrates/amaru-tui/src/model/peer_state.rscrates/amaru-tui/src/model/queries.rscrates/amaru-tui/src/model/rate_counter.rscrates/amaru-tui/src/model/telemetry_event.rscrates/amaru-tui/src/model/telemetry_update.rscrates/amaru-tui/src/model/tip_state.rscrates/amaru-tui/src/session.rscrates/amaru-tui/src/startup.rscrates/amaru-tui/src/ui/components/logs.rscrates/amaru-tui/src/ui/screens/amaru.rscrates/amaru-tui/src/ui/screens/config.rscrates/amaru/src/bin/amaru/cmd/node/run.rscrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/amaru/pid.rscrates/amaru/src/lifecycle.rscrates/amaru/src/metrics.rscrates/amaru/src/observability.rsengineering-decision-records/030-embedded-terminal-observability-ui.mdmonitoring/Makefilemonitoring/README.mdmonitoring/docker-compose.ymlmonitoring/otlp-collector.yml
💤 Files with no reviewable changes (9)
- .github/workflows/template-ledger-epoch-snapshots.yml
- crates/amaru-tui/Cargo.toml
- monitoring/Makefile
- crates/amaru-tui/src/lib.rs
- crates/amaru-tui/src/events/host_sample.rs
- crates/amaru-tui/src/metrics.rs
- monitoring/docker-compose.yml
- monitoring/otlp-collector.yml
- crates/amaru-tui/src/host_metrics.rs
🚧 Files skipped from review as they are similar to previous changes (41)
- .github/workflows/nightly-tool-integrations.yml
- CHANGELOG.md
- crates/amaru-kernel/src/lib.rs
- Cargo.toml
- crates/amaru-metrics/src/ledger.rs
- crates/amaru-metrics/src/mempool.rs
- crates/amaru-observability/macros/src/traces.rs
- crates/amaru-node/src/tests/setup.rs
- crates/amaru-protocols/src/metrics_effects.rs
- crates/amaru-observability/tests/schema_access.rs
- crates/amaru-node/Cargo.toml
- crates/amaru-tui/src/events.rs
- crates/amaru-tui/src/ui/components/logs.rs
- engineering-decision-records/030-embedded-terminal-observability-ui.md
- crates/amaru-tui/src/events/telemetry_record.rs
- crates/amaru-metrics/src/protocol.rs
- crates/amaru-metrics/src/consensus.rs
- crates/amaru-tui/README.md
- monitoring/README.md
- crates/amaru-tui/src/events/message.rs
- crates/amaru-kernel/src/utils.rs
- crates/amaru-tui/src/model/interaction.rs
- crates/amaru/src/bin/amaru/main.rs
- crates/amaru-tui/src/ui/screens/config.rs
- crates/amaru/src/bin/amaru/pid.rs
- crates/amaru-tui/AGENTS.md
- crates/amaru-tui/src/capture.rs
- crates/amaru-observability/macros/src/define_schemas.rs
- crates/amaru-metrics/src/system.rs
- crates/amaru-metrics/src/lib.rs
- crates/amaru-tui/src/model/tip_state.rs
- crates/amaru-kernel/src/utils/process.rs
- crates/amaru/src/metrics.rs
- crates/amaru-kernel/src/cardano/era_history.rs
- crates/amaru/src/lifecycle.rs
- crates/amaru-node/src/stages/build_node.rs
- crates/amaru-metrics/src/metrics.rs
- crates/amaru-tui/src/startup.rs
- crates/amaru-tui/src/events/field_value.rs
- crates/amaru/src/observability.rs
- crates/amaru-tui/src/model.rs
| impl Config { | ||
| pub fn log_capacity_for(&self, level: Level) -> usize { | ||
| match level { | ||
| Level::TRACE | Level::DEBUG => self.debug_log_capacity, | ||
| Level::INFO => self.info_log_capacity, | ||
| Level::WARN => self.warn_log_capacity, | ||
| Level::ERROR => self.error_log_capacity, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how tracing::Level is defined and whether the repo matches on it elsewhere.
fd -t f 'Cargo.toml' -x rg -n 'tracing\s*=' {} \;
rg -n -C3 'Level::TRACE\s*\|\s*Level::DEBUG' --type=rustRepository: pragma-org/amaru
Length of output: 519
🌐 Web query:
Is tracing::Level an enum or a struct with associated constants, and can a Rust match on Level::TRACE be exhaustive?
💡 Result:
tracing::Level is a struct, not an enum [1][2]. It provides five associated constants: ERROR, WARN, INFO, DEBUG, and TRACE [1][3]. Because it is a struct, you cannot perform an exhaustive match on its variants as you would with an enum [1][4]. If you write a match statement that only covers the five associated constants (e.g., matching on Level::TRACE, Level::DEBUG, etc.), the Rust compiler will require a catch-all arm (usually represented by an underscore _ or a wildcard pattern) because it does not treat the struct as a closed set of variants [4].
Citations:
- 1: https://docs.rs/tracing/latest/tracing/struct.Level.html
- 2: https://docs.rs/tracing/0.1.16/tracing/struct.Level.html
- 3: https://prisma.github.io/prisma-engines/doc/tracing/struct.Level.html
- 4: Non-const event level tokio-rs/tracing#2730
🏁 Script executed:
#!/bin/bash
set -e
printf '--- candidate file snippets ---\n'
sed -n '1,120p' crates/amaru-tui/src/config.rs
printf '\n--- log_buffer relevant matches ---\n'
rg -n -C5 'match level|fn count|fn increment|fn decrement' crates/amaru-tui/src/model/log_buffer.rs
printf '\n--- package manifest excerpt ---\n'
rg -n -C3 '^\[package\]|name = "amaru-tui"|tracing = ' crates/amaru-tui/Cargo.toml Cargo.toml
printf '\n--- check cargo availability and build metadata for tracing Level, if present ---\n'
command -v cargo || true
cargo metadata --format-version 1 --no-deps 2>/dev/null | jq -r '.packages[] | select(.name=="tracing") | {name, version, source, manifest_path}' || true
printf '\n--- grep exact tracing Level matches ---\n'
rg -n 'match\s+\w+::Level|\bLevel::(TRACE|DEBUG|INFO|WARN|ERROR)\b' --type rRepository: pragma-org/amaru
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate file snippets ---'
sed -n '1,120p' crates/amaru-tui/src/config.rs
printf '%s\n' ''
printf '%s\n' '--- log_buffer relevant matches ---'
rg -n -C5 'match level|fn count|fn increment|fn decrement' crates/amaru-tui/src/model/log_buffer.rs
printf '%s\n' ''
printf '%s\n' '--- package manifest excerpt ---'
rg -n -C3 '^\[package\]|name = "amaru-tui"|tracing = ' crates/amaru-tui/Cargo.toml Cargo.toml
printf '%s\n' ''
printf '%s\n' '--- check cargo availability and tracing package metadata ---'
command -v cargo || true
cargo metadata --format-version 1 --no-deps 2>/dev/null | jq -r '.packages[] | select(.name=="tracing") | {name, version, source, manifest_path}' || true
printf '%s\n' ''
printf '%s\n' '--- grep exact tracing Level matches ---'
rg -n 'match\s+\w+::Level|\bLevel::(TRACE|DEBUG|INFO|WARN|ERROR)\b' --type rust || trueRepository: pragma-org/amaru
Length of output: 5188
🌐 Web query:
tracing 0.1.40 Level repr core::level Level enum debug
💡 Result:
In the tracing crate (including version 0.1.40), the Level enum is defined within tracing_core and re-exported by the tracing crate to represent the verbosity level of a span or event [1][2]. The Level::DEBUG variant designates lower-priority diagnostic information [3][4]. The Level type implements the Debug trait, which provides its string representation [5]. When formatted, it typically produces the name of the level (e.g., "DEBUG") [5]. The Level enum is designed for use in filtering, and it implements PartialOrd and Ord, where Level::TRACE is the most verbose (highest) and Level::ERROR is the least verbose (lowest) [3][6]. For further programmatic access to the string representation, the Level type provides an as_str method that returns the static string representation of the level (e.g., "DEBUG") [6].
Citations:
- 1: https://tikv.github.io/doc/tracing_core/struct.Level.html
- 2: https://github.com/tokio-rs/tracing
- 3: https://docs.rs/tracing/latest/tracing/struct.Level.html
- 4: https://docs.rs/tokio-trace-core/latest/tokio_trace_core/metadata/struct.Level.html
- 5: https://tikv.github.io/doc/tracing/struct.Level.html
- 6: https://docs.ct2.io/peridot/dev/tracing/struct.Level.html
🌐 Web query:
cargo tree tracing 0.1.40 tracing_core source
💡 Result:
For the tracing crate version 0.1.40, the tracing-core dependency is version 0.1.32 [1][2][3]. If you are trying to understand why tracing-core is included in your project or where it originates, you can use the cargo tree command [4]. Because tracing depends on tracing-core [1], tracing-core will appear as a sub-dependency of tracing. To inspect the dependency graph and identify which crates are pulling in tracing-core, you can use the following commands: 1. To view the dependency tree for tracing and its child dependencies: cargo tree -p tracing@0.1.40 2. To see the reverse dependency graph (to find out which crates depend on tracing-core): cargo tree -i tracing-core 3. To see why tracing-core is included, including feature flags: cargo tree -e features -i tracing-core The tracing-core crate is part of the tokio-rs/tracing repository on GitHub [5]. While tracing provides high-level APIs for instrumentation, tracing-core contains the fundamental primitives (such as Subscriber and Metadata) and is intended to be stable with infrequent API changes [5]. If you need to view the specific source code for a dependency currently in your project, you can use cargo vendor to download the source locally or navigate to the location specified by cargo metadata [4].
Citations:
- 1: https://crates.io/crates/tracing/0.1.40
- 2: https://deps.rs/crate/tracing/0.1.40
- 3: https://android.googlesource.com/platform/external/rust/crates/tracing/+/b4cf1d391b50a17f362c99730e6536a2b095f526/CHANGELOG.md
- 4: https://doc.rust-lang.org/cargo/commands/cargo-tree.html
- 5: https://github.com/tokio-rs/tracing/tree/main/tracing-core
🌐 Web query:
github tokio-rs tracing tracing-core 0.1.32 src/level.rs
💡 Result:
In the tracing-core crate version 0.1.32, the file src/level.rs does not exist [1]. The Level struct and related functionality are defined within the src/metadata.rs file [2][3][4]. The Level struct in tracing-core 0.1.32 describes the verbosity of a span or event and implements the PartialOrd and Ord traits [2][5]. It defines the following verbosity levels, ordered from lowest (least verbose) to highest (most verbose) [2][6][5]: 1. ERROR 2. WARN 3. INFO 4. DEBUG 5. TRACE These levels allow for comparison (e.g., Level::TRACE > Level::DEBUG) to facilitate filtering of trace data [2][6][5]. The crate provides these core primitives to enable structured, event-based diagnostic information in Rust programs [7].
Citations:
- 1: https://docs.rs/crate/tracing-core/0.1.32/source/
- 2: https://codebrowser.dev/tokio/crates/tracing-core-0.1.32/src/metadata.rs.html
- 3: https://github.com/tokio-rs/tracing/blob/master/tracing-core/src/metadata.rs
- 4: https://docs.rs/tracing-core/latest/tracing_core/metadata/index.html
- 5: https://doc.servo.org/tracing_core/struct.Level.html
- 6: https://tracing.rs/tracing_core/struct.level
- 7: https://crates.io/crates/tracing-core/0.1.32
🌐 Web query:
docs.rs tracing-core 0.1.32 source metadata.rs struct Level enum Visibility private
💡 Result:
In tracing-core 0.1.32, the Level struct is a wrapper around a private enum named LevelInner [1]. The definition in metadata.rs is: pub struct Level(LevelInner); And the underlying enum is defined as: #[repr(usize)] #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)] enum LevelInner { Trace = 0, Debug = 1, Info = 2, Warn = 3, Error = 4, } Because LevelInner is not declared with the pub keyword, it is private to the metadata module, meaning it cannot be accessed or constructed directly from outside that module [1]. This design pattern allows the Level struct to encapsulate the verbosity levels while exposing a public API (such as the constants ERROR, WARN, INFO, DEBUG, and TRACE) for users of the library [2][1].
Citations:
- 1: https://codebrowser.dev/tokio/crates/tracing-core-0.1.32/src/metadata.rs.html
- 2: https://tracing.rs/tracing_core/metadata/struct.level
🌐 Web query:
Rust 2024 exhaustive match non-exhaustive enum struct pattern ..``
💡 Result:
In Rust, when a type (struct, enum, or enum variant) is marked with the #[non_exhaustive] attribute, the compiler prevents downstream crates from exhaustively matching it [1][2]. This is designed to allow the defining crate to add new fields to the struct or new variants to the enum in the future without causing breaking changes for consumers [2]. When pattern matching against a non-exhaustive type outside of the defining crate, you must include a wildcard or rest pattern to account for potential future additions [1][3]. For structs, this requirement means you must use the .. (rest) pattern in your struct pattern [1][4]. If you omit the .. pattern, the compiler will error because the struct pattern must specify all fields, and non-exhaustive structs effectively hide the guarantee that the fields you see are the only fields that exist [1][3]. Example of matching a non-exhaustive struct: // Assuming Config is #[non_exhaustive] if let Config { field1,.. } = config { // Correct usage with .. } If you attempt to match without .., such as Config { field1 }, the compiler will require you to include .. to indicate that you are intentionally ignoring other (potentially future) fields [1][3]. Similarly, for non-exhaustive enums, matching on a variant does not contribute to the exhaustiveness of the match statement, so you must always include a catch-all arm (like _ => {}) to handle any variants that might be added later [1][3]. These rules apply consistently across Rust editions, including Rust 2024. While Rust 2024 introduced changes to match ergonomics [5][6], the fundamental requirements for #[non_exhaustive] pattern matching remain unchanged from their original design [2].
Citations:
- 1: https://dev-doc.rust-lang.org/beta/reference/attributes/type_system.html
- 2: https://rust-lang.github.io/rfcs/2008-non-exhaustive.html
- 3: https://doc.rust-lang.org/reference/attributes/type_system.html
- 4: https://doc.rust-lang.org/reference/patterns.html?highlight=range
- 5: https://doc.rust-lang.org/edition-guide/rust-2024/match-ergonomics.html
- 6: Tracking issue for Rust 2024: Match ergonomics rules 1C/2C rust-lang/rust#131414
🏁 Script executed:
#!/bin/bash
set -e
cargo --version
rustc --version
cargo metadata --format-version 1 --locked --no-deps 2>/dev/null | jq -r '.workspace_members[]' | while read -r member; do echo "$member"; done | head -20 || trueRepository: pragma-org/amaru
Length of output: 1466
Add a default arm for these tracing::Level matches.
tracing::Level is a wrapper around a private level enum, so matching Level::TRACE | Level::DEBUG ... but not _, ... leaves these non-exhaustive matches without a catch-all. Add Level(_) => ... in crates/amaru-tui/src/config.rs:58-67 and the same spot in crates/amaru-tui/src/model/log_buffer.rs, or use PartialEq/level < Level::INFO instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/amaru-tui/src/config.rs` around lines 58 - 67, Update
Config::log_capacity_for to add a catch-all Level(_) match arm, preserving the
existing capacity selection for known TRACE, DEBUG, INFO, WARN, and ERROR
levels. Apply the same exhaustive-match fix in the corresponding log-level match
within the log buffer implementation.
| let timeout = next_draw_at.saturating_duration_since(Instant::now()); | ||
| match telemetry_rx.recv_timeout(timeout) { | ||
| Ok(message) => { | ||
| model.handle_message(message); | ||
| while let Ok(message) = telemetry_rx.try_recv() { | ||
| model.handle_message(message); | ||
| } | ||
| } | ||
| Err(RecvTimeoutError::Timeout) => {} | ||
| Err(RecvTimeoutError::Disconnected) => {} | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle Disconnected instead of ignoring it, or the loop spins hot.
When every sender is dropped, recv_timeout returns Disconnected immediately, every call. The loop then becomes a busy spin until next_draw_at, and it repeats forever after that. One core pinned at 100%, like leaving the handbrake on the whole drive home.
Normally Control::Shutdown arrives first, so this stays hidden. It bites when the Session is dropped without shutdown(), for example during an unwind on the main thread.
Return from the loop on Disconnected.
🔧 Proposed fix
Err(RecvTimeoutError::Timeout) => {}
- Err(RecvTimeoutError::Disconnected) => {}
+ Err(RecvTimeoutError::Disconnected) => return Ok(()),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let timeout = next_draw_at.saturating_duration_since(Instant::now()); | |
| match telemetry_rx.recv_timeout(timeout) { | |
| Ok(message) => { | |
| model.handle_message(message); | |
| while let Ok(message) = telemetry_rx.try_recv() { | |
| model.handle_message(message); | |
| } | |
| } | |
| Err(RecvTimeoutError::Timeout) => {} | |
| Err(RecvTimeoutError::Disconnected) => {} | |
| } | |
| let timeout = next_draw_at.saturating_duration_since(Instant::now()); | |
| match telemetry_rx.recv_timeout(timeout) { | |
| Ok(message) => { | |
| model.handle_message(message); | |
| while let Ok(message) = telemetry_rx.try_recv() { | |
| model.handle_message(message); | |
| } | |
| } | |
| Err(RecvTimeoutError::Timeout) => {} | |
| Err(RecvTimeoutError::Disconnected) => return Ok(()), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/amaru-tui/src/session.rs` around lines 143 - 153, Update the telemetry
receive loop around recv_timeout so Err(RecvTimeoutError::Disconnected) exits
the loop immediately, while preserving the existing timeout behavior and message
handling for connected receivers.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation