Conversation
Reviewer's GuideThis PR introduces a new Prometheus counter to track connection task panics, wires it into the connection task’s catch_unwind handler, adds unit and integration tests to verify the metric is recorded, and extends the production resilience guide with alerting recommendations. ER diagram for Prometheus connection panic metricerDiagram
CONNECTION_TASK ||--o{ CONNECTION_PANIC_METRIC : triggers
CONNECTION_PANIC_METRIC {
string name
int count
}
Class diagram for new connection panic metric trackingclassDiagram
class Metrics {
+inc_connection_panics()
+CONNECTION_PANICS : &str
}
class ConnectionTask {
+spawn_connection_task()
}
ConnectionTask ..> Metrics : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughIntroduce a new Prometheus-compatible metric, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant MetricsRecorder
Client->>Server: Initiate connection
Server->>Server: spawn_connection_task
Server-->>Server: Connection task panics
Server->>MetricsRecorder: inc_connection_panics(peer_addr)
MetricsRecorder-->>MetricsRecorder: Increment wireframe_connection_panics_total
Server->>Server: Handle panic (log, isolate)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🧬 Code Graph Analysis (1)src/server/connection.rs (2)
🔍 MCP Research (1 server)Deepwiki:
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:171` </location>
<code_context>
+Each occurrence also increments the `wireframe_connection_panics_total`
+counter, enabling alerts on unexpected spikes. Operators can chart
+`rate(wireframe_connection_panics_total[5m])` in Prometheus and create Grafana
+panels to visualise instability. To emit this metric, enable the `metrics`
+Cargo feature and install a recorder such as `metrics_exporter_prometheus`,
+which exposes an HTTP endpoint for scraping.
</code_context>
<issue_to_address>
'Visualise' uses en-gb spelling; en-oxendic prefers 'visualize'.
The word 'visualise' should be spelled 'visualize' to conform to en-oxendic spelling conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/hardening-wireframe-a-guide-to-production-resilience.md (1)
168-174: Use Oxford “-ize” spellingReplace “visualise” with “visualize” to follow en-GB-oxendict conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/hardening-wireframe-a-guide-to-production-resilience.md(1 hunks)src/metrics.rs(3 hunks)src/server/connection.rs(3 hunks)tests/metrics.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/hardening-wireframe-a-guide-to-production-resilience.md
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/server/connection.rstests/metrics.rssrc/metrics.rs
🧬 Code Graph Analysis (1)
src/metrics.rs (1)
src/extractor.rs (1)
peer_addr(362-362)
🪛 LanguageTool
docs/hardening-wireframe-a-guide-to-production-resilience.md
[style] ~171-~171: Would you like to use the Oxford spelling “visualize”? The spelling ‘visualise’ is also correct.
Context: ...Prometheus and create Grafana panels to visualise instability. To emit this metric, enabl...
(OXFORD_SPELLING_Z_NOT_S)
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServerarchitecture uses a factory pattern to spawn worker tasks that accept connections and spawn connection tasks wrapped with panic recovery usingcatch_unwind. When a panic occurs in a connection task, it is caught and logged with the peer address, and now also increments thewireframe_connection_panics_totalmetric. This ensures that panics do not crash the server and are observable via metrics. (docs/hardening-wireframe-a-guide-to-production-resilience.md) -
The connection actor model and asynchronous messaging system use
PushHandleandSessionRegistrywith weak references to manage connection lifecycles and prevent resource leaks. Panic isolation and metric incrementing integrate with this architecture to provide resilience and observability. (docs/hardening-wireframe-a-guide-to-production-resilience.md) -
Observability and monitoring documentation describes structured logging with tracing spans for connections and requests, and metrics collection integrated with Prometheus exporters. The new panic metric
wireframe_connection_panics_totalfits into this observability framework, enabling operators to monitor panic rates over time and create Grafana dashboards for visualization. (docs/hardening-wireframe-a-guide-to-production-resilience.md) -
The production deployment documentation emphasizes the importance of enabling the
metricsCargo feature and installing a recorder likemetrics_exporter_prometheusto expose an HTTP scraping endpoint for Prometheus. It also details how the panic metric counter supports operational monitoring and alerting for connection panics. (docs/hardening-wireframe-a-guide-to-production-resilience.md)
🔇 Additional comments (2)
tests/metrics.rs (1)
65-82: Test looks soundThe test correctly asserts that the counter increments under a local recorder.
src/server/connection.rs (1)
41-41: Instrumentation correctly placedIncrementing the panic counter inside the
catch_unwindbranch is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/hardening-wireframe-a-guide-to-production-resilience.md(1 hunks)src/metrics.rs(2 hunks)src/server/connection.rs(3 hunks)tests/metrics.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit Inference Engine (docs/contents.md)
Conventions for writing project documentation as described in the documentation style guide
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise alt text describing the content.
Add a brief description before each Mermaid diagram in documentation for screen readers.Document examples showing how to deprecate old message versions gracefully
Write the official documentation for the new features. Create separate guides for "Duplex Messaging & Pushes", "Streaming Responses", and "Message Fragmentation". Each guide must include runna...
Files:
docs/hardening-wireframe-a-guide-to-production-resilience.md
docs/**/*.{md,rs}
📄 CodeRabbit Inference Engine (docs/multi-packet-and-streaming-responses-design.md)
docs/**/*.{md,rs}: The official documentation and examples must exclusively use the declarativeResponsemodel for handler responses.
The async-stream pattern must be documented as the canonical approach for dynamic stream generation.
Files:
docs/hardening-wireframe-a-guide-to-production-resilience.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar. (EXCEPTION: the naming of the LICENSE file, which is to be left unchanged for community consistency.)
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown files must be wrapped at 120 columns.
Tables and headings in Markdown files must not be wrapped.
Use dashes (-) for list bullets in Markdown files.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/hardening-wireframe-a-guide-to-production-resilience.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/hardening-wireframe-a-guide-to-production-resilience.md
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status....
Files:
src/metrics.rssrc/server/connection.rstests/metrics.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/metrics.rssrc/server/connection.rstests/metrics.rs
🔍 MCP Research (1 server)
Deepwiki:
-
The panic handling mechanism in connection tasks is robust and tested: panics in connection tasks are caught and logged without crashing the server, allowing continued acceptance of new connections. This is supported by existing tests and documented in the
WireframeServerwiki and test suite. The new metric addition supplements this by providing observability into panic frequency for operational monitoring. (src/server.rs, tests/world.rs, docs/hardening-wireframe-a-guide-to-production-resilience.md) -
The documentation specifies that to emit the panic metric, the
metricsCargo feature must be enabled and a recorder such asmetrics_exporter_prometheusinstalled to expose an HTTP scraping endpoint. This enables operators to monitor panic rates over time and create Grafana dashboards for visualization and alerting. (docs/hardening-wireframe-a-guide-to-production-resilience.md)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (1)
src/server/connection.rs (1)
40-43: Good instrumentation.Incrementing the panic counter here fulfils the monitoring objective without exposing labels.
leynos
deleted the
codex/add-production-monitoring-for-connection-panics
branch
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/server/connection.rs:219` </location>
<code_context>
+ /// Panics increment the connection panic counter.
+ #[rstest]
+ #[tokio::test]
+ async fn connection_panic_metric_increments(
+ factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static,
+ ) {
</code_context>
<issue_to_address>
Consider replacing the complex async integration test with a simple unit test that directly checks the metric increment.
Instead of spinning up a real TCP listener, driver tasks and I/O just to verify that `inc_connection_panics()` was called, you can drop that entire async test and replace it with a tiny unit‐test that directly exercises the metric. For example:
```rust
#[cfg(test)]
mod metrics_tests {
use metrics_util::debugging::{DebugValue, DebuggingRecorder};
use crate::metrics::{self, CONNECTION_PANICS};
#[test]
fn inc_connection_panics_increments_counter() {
// Arrange: install a debugging recorder
let recorder = DebuggingRecorder::new();
let snapshotter = recorder.snapshotter();
recorder.install().expect("failed to install recorder");
// Act: invoke the panic‐increment function directly
metrics::inc_connection_panics();
// Assert: we saw exactly one increment on the CONNECTION_PANICS counter
let metrics = snapshotter.snapshot().into_vec();
assert!(metrics.iter().any(|(key, _, _, value)| {
key.key().name() == CONNECTION_PANICS
&& matches!(value, DebugValue::Counter(c) if *c == 1)
}), "expected CONNECTION_PANICS == 1, got {metrics:#?}");
}
}
```
Steps:
1. Remove the entire `async fn connection_panic_metric_increments` and its TCP boilerplate.
2. Add the above `inc_connection_panics_increments_counter` test.
3. Verify `cargo test` still passes and coverage for `inc_connection_panics()` remains.
</issue_to_address>
### Comment 2
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:168` </location>
<code_context>
Connection tasks are wrapped with `catch_unwind` to log and discard panics.
Each panicking connection is isolated so it cannot terminate the entire server.
+Each occurrence also increments the `wireframe_connection_panics_total`
+counter, enabling alerts on unexpected spikes. The counter intentionally omits
+peer address labels to limit cardinality and protect personally identifiable
</code_context>
<issue_to_address>
This paragraph is not wrapped to 80 columns as required by the style guide.
Please wrap this paragraph so that no line exceeds 80 columns, in accordance with the documentation formatting standards.
</issue_to_address>
### Comment 3
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:169` </location>
<code_context>
Each panicking connection is isolated so it cannot terminate the entire server.
+Each occurrence also increments the `wireframe_connection_panics_total`
+counter, enabling alerts on unexpected spikes. The counter intentionally omits
+peer address labels to limit cardinality and protect personally identifiable
+information. Operators can chart `rate(wireframe_connection_panics_total[5m])`
</code_context>
<issue_to_address>
This line is part of a paragraph that exceeds the 80 column limit.
Ensure all lines in paragraphs and bullet points are wrapped to 80 columns for readability and consistency.
</issue_to_address>
### Comment 4
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:170` </location>
<code_context>
+Each occurrence also increments the `wireframe_connection_panics_total`
+counter, enabling alerts on unexpected spikes. The counter intentionally omits
+peer address labels to limit cardinality and protect personally identifiable
+information. Operators can chart `rate(wireframe_connection_panics_total[5m])`
+in Prometheus and create Grafana panels to visualize instability. To emit this
</code_context>
<issue_to_address>
This line is part of a paragraph that is not wrapped to 80 columns.
Please reformat this paragraph so that each line does not exceed 80 columns.
</issue_to_address>
### Comment 5
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:171` </location>
<code_context>
+Each occurrence also increments the `wireframe_connection_panics_total`
+counter, enabling alerts on unexpected spikes. The counter intentionally omits
+peer address labels to limit cardinality and protect personally identifiable
+information. Operators can chart `rate(wireframe_connection_panics_total[5m])`
+in Prometheus and create Grafana panels to visualize instability. To emit this
+metric, enable the `metrics` Cargo feature and install a recorder such as
</code_context>
<issue_to_address>
This line is part of a paragraph that is not wrapped to 80 columns.
Wrap this paragraph to 80 columns as per documentation standards.
</issue_to_address>
### Comment 6
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:172` </location>
<code_context>
+counter, enabling alerts on unexpected spikes. The counter intentionally omits
+peer address labels to limit cardinality and protect personally identifiable
+information. Operators can chart `rate(wireframe_connection_panics_total[5m])`
+in Prometheus and create Grafana panels to visualize instability. To emit this
+metric, enable the `metrics` Cargo feature and install a recorder such as
+`metrics_exporter_prometheus`, which exposes an HTTP endpoint for scraping.
</code_context>
<issue_to_address>
This line is part of a paragraph that is not wrapped to 80 columns.
Please ensure all paragraph lines are wrapped to 80 columns.
</issue_to_address>
### Comment 7
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:173` </location>
<code_context>
+peer address labels to limit cardinality and protect personally identifiable
+information. Operators can chart `rate(wireframe_connection_panics_total[5m])`
+in Prometheus and create Grafana panels to visualize instability. To emit this
+metric, enable the `metrics` Cargo feature and install a recorder such as
+`metrics_exporter_prometheus`, which exposes an HTTP endpoint for scraping.
+
</code_context>
<issue_to_address>
This line is part of a paragraph that is not wrapped to 80 columns.
Reformat this paragraph so that each line is wrapped to 80 columns.
</issue_to_address>
### Comment 8
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:174` </location>
<code_context>
+information. Operators can chart `rate(wireframe_connection_panics_total[5m])`
+in Prometheus and create Grafana panels to visualize instability. To emit this
+metric, enable the `metrics` Cargo feature and install a recorder such as
+`metrics_exporter_prometheus`, which exposes an HTTP endpoint for scraping.
+
### 3.2 Leak-Proof Registries with `Weak`/`Arc`
</code_context>
<issue_to_address>
This line is part of a paragraph that is not wrapped to 80 columns.
Please wrap this paragraph to 80 columns for consistency with the documentation style guide.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
wireframe_connection_panics_totalcloses #217
Testing
make fmtmake markdownlintmake nixie(fails: too many arguments/failed to link packages)make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68953c8ac6f08322a60414af8a1d1406
Summary by Sourcery
Instrument connection tasks to record panics as a new Prometheus metric and update tests and documentation to use and verify the new counter.
New Features:
wireframe_connection_panics_totalcounter andinc_connection_panicsAPI to record panicking connection tasks.Enhancements:
spawn_connection_taskwhen a task panics.Documentation:
Tests: