Skip to content

Implement connection metrics diagram and drop guard#208

Merged
leynos merged 3 commits intomainfrom
pseqvu-codex/implement-pervasive-observability-in-wireframe-1.0
Jul 15, 2025
Merged

Implement connection metrics diagram and drop guard#208
leynos merged 3 commits intomainfrom
pseqvu-codex/implement-pervasive-observability-in-wireframe-1.0

Conversation

@leynos
Copy link
Copy Markdown
Owner

@leynos leynos commented Jul 13, 2025

Summary

  • decrement active connections gauge on ConnectionActor drop
  • document ACTIVE_CONNECTIONS metric with a sequence diagram
  • convert remaining log crate calls to tracing
  • introduce ActiveConnection RAII guard for connection counting
  • upgrade tracing dependency and refine push queue logging

Testing

  • make fmt
  • make lint
  • make test
  • markdownlint docs/wireframe-1-0-detailed-development-roadmap.md
  • nixie docs/wireframe-1-0-detailed-development-roadmap.md

https://chatgpt.com/codex/tasks/task_e_68718f6e22708322bf47d3a827e0b6b8

Summary by Sourcery

Implement a new ActiveConnection guard to maintain the active connections gauge, ensure proper decrement on actor drop, convert logging to tracing with enhanced push queue diagnostics, upgrade the tracing dependency, and update documentation and tests accordingly

New Features:

  • Introduce ActiveConnection RAII guard for automatic tracking of active connections

Bug Fixes:

  • Decrement the ACTIVE_CONNECTIONS gauge when ConnectionActor instances are dropped

Enhancements:

  • Migrate all remaining log crate calls to tracing macros and refine push queue logging with debug, warn, and error events
  • Instrument ConnectionActor with a tracing span and add debug logs when frames are pushed

Build:

  • Upgrade tracing dependency to >=0.1.40 with log integration features

Documentation:

  • Add a sequence diagram illustrating the ACTIVE_CONNECTIONS metric lifecycle in documentation

Tests:

  • Update connection actor tests to robustly detect warning logs in the new tracing setup

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented Jul 13, 2025

Reviewer's Guide

This PR adds an RAII guard to track active connections and ties gauge updates to its lifetime, enriches documentation with a sequence diagram for the ACTIVE_CONNECTIONS metric, migrates remaining log calls to tracing, refines push-queue logging with structured debug/warning statements, and adjusts tests to reflect these changes.

Sequence diagram for ACTIVE_CONNECTIONS metric lifecycle

sequenceDiagram
    participant Client
    participant Connection
    participant Tracing

    Client->>Connection: Open connection
    Connection->>Tracing: info!(wireframe_active_connections, "connection opened")
    Note right of Connection: ACTIVE_CONNECTIONS += 1

    Client->>Connection: Start run()
    Connection->>Tracing: info_span!("connection_actor")

    alt Shutdown before start
        Connection->>Tracing: info!("connection aborted before start")
        Note right of Connection: ACTIVE_CONNECTIONS -= 1
    else Normal close
        Connection->>Tracing: info!("connection closed")
        Note right of Connection: ACTIVE_CONNECTIONS -= 1
    end
Loading

Class diagram for ActiveConnection RAII guard and ConnectionActor changes

classDiagram
    class ActiveConnection {
        +new() ActiveConnection
        -drops: decrements ACTIVE_CONNECTIONS
    }

    class ConnectionActor {
        -counter: Option~ActiveConnection~
        +new(...): ConnectionActor
        +run(...): Result
    }

    ConnectionActor "1" *-- "1" ActiveConnection : holds
Loading

File-Level Changes

Change Details Files
Introduce ActiveConnection RAII guard for connection counting and gauge updates
  • Added ActiveConnection field to ConnectionActor
  • Initialize guard on actor creation and log connection open
  • Trigger gauge decrement via guard drop on abort and normal close
src/connection.rs
Document ACTIVE_CONNECTIONS metric with sequence diagram
  • Inserted Mermaid sequence diagram illustrating connection open, shutdown, and close events
  • Updated related markdown sections for clarity
docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md
Migrate remaining log crate calls to tracing
  • Replaced log::warn and log::error calls with tracing::warn/error
  • Upgraded tracing dependency in Cargo.toml
src/app.rs
src/push.rs
Cargo.toml
Refine push queue logging
  • Added debug! after successful frame send
  • Structured warn! with priority, policy, and DLQ status
  • Removed unstructured log statements
src/push.rs
Adjust connection actor tests for logging changes
  • Updated test to scan logger records for a warning containing the expected message
  • Removed direct level/assertion in favor of iterative search
tests/connection_actor.rs

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jul 13, 2025

Summary by CodeRabbit

  • New Features

    • Introduced global tracking of active connections, with public access to the current connection count.
    • Enhanced structured logging across the application using the tracing system, providing richer context for connection and push events.
  • Bug Fixes

    • Improved test reliability by ensuring serial execution of asynchronous tests to avoid concurrency issues.
  • Documentation

    • Added a sequence diagram and improved table formatting in documentation for better clarity and compactness.
  • Tests

    • Added tests to verify that the active connection count increments and decrements correctly on connection open and close.

Summary by CodeRabbit

  • New Features

    • Added global tracking of active connections, including a method to retrieve the current count.
    • Enhanced structured logging for connection lifecycle events and push operations using the tracing system.
  • Bug Fixes

    • Improved test reliability by ensuring active connection counts are accurately incremented and decremented on connection abort and closure.
  • Documentation

    • Added a sequence diagram illustrating connection lifecycle with tracing.
    • Reformatted and improved the readability of development roadmap tables.
  • Tests

    • Introduced new tests to verify correct active connection counting.
    • Improved log assertion in protocol error warning tests.

Walkthrough

Update the project to use the tracing crate for structured logging throughout the codebase, replacing previous log macros. Introduce global active connection counting in the connection actor, with atomic tracking and new tests to verify correct increment and decrement. Refine documentation formatting and add a sequence diagram for tracing instrumentation.

Changes

File(s) Change Summary
Cargo.toml Add tracing dependency with "log" and "log-always" features.
src/app.rs, src/push.rs Replace log macros with tracing macros for structured logging; add contextual fields in logs.
src/connection.rs Add atomic active connection counter, RAII guard, and structured connection lifecycle logging; update actor.
tests/connection_actor.rs Add tests for connection count increment/decrement; update warning log assertion logic.
docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md Add Mermaid sequence diagram for connection lifecycle with tracing; compact table formatting.
docs/wireframe-1-0-detailed-development-roadmap.md Reformat markdown tables for clarity; remove extraneous whitespace.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Connection
    participant Tracing

    Client->>Connection: Open connection
    Connection->>Tracing: Log "connection opened" with context
    Connection->>Tracing: Increment active connection count
    Connection->>Connection: Start run loop
    alt Shutdown token cancelled
        Connection->>Tracing: Log "connection aborted before run"
        Connection->>Tracing: Decrement active connection count
    else Normal closure
        Connection->>Tracing: Log "connection closed"
        Connection->>Tracing: Decrement active connection count
    end
Loading

Possibly related PRs

Poem

Connections counted, logs anew,
With tracing fields in every view.
RAII guards watch numbers rise and fall,
Structured warnings answer every call.
Tables neat and diagrams bright—
Wireframe’s future shining light!
🚦📈


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f1d7641 and c062508.

📒 Files selected for processing (1)
  • tests/connection_actor.rs (15 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

  • AGENTS.md

⚙️ CodeRabbit Configuration File

🧬 Code Graph Analysis (1)
tests/connection_actor.rs (2)
wireframe_testing/src/logging.rs (2)
  • logger (46-46)
  • new (21-28)
src/connection.rs (5)
  • new (30-33)
  • new (135-148)
  • new (504-513)
  • active_connection_count (42-42)
  • shutdown_token (194-194)
⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-test
🔇 Additional comments (2)
tests/connection_actor.rs (2)

42-42: Excellent addition of serial execution for global state tests.

The #[serial] attributes ensure these tests don't interfere with each other when testing the global active connection count. This prevents race conditions that could cause flaky test failures.

Also applies to: 61-61, 127-127, 152-152, 177-177, 222-222, 239-239, 263-263, 299-299, 324-324, 344-344, 373-373, 396-396, 424-424, 455-455


312-319: Proper migration to handle tracing log record iteration.

The test now correctly iterates through all log records to find the warning, rather than checking only the first record. This change aligns with the migration from the log crate to the tracing crate.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch pseqvu-codex/implement-pervasive-observability-in-wireframe-1.0

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/push.rs:177` </location>
<code_context>
                 Ok(()) => {}
                 Err(mpsc::error::TrySendError::Full(_)) => {
-                    log::error!("push queue and DLQ full; frame lost");
+                    error!("push queue and DLQ full; frame lost");
                 }
                 Err(mpsc::error::TrySendError::Closed(_)) => {
-                    log::error!("DLQ closed; frame lost");
+                    error!("DLQ closed; frame lost");
                 }
             }
</code_context>

<issue_to_address>
Log messages for lost frames could include frame details for better traceability.

Including frame identifiers in these log messages will improve troubleshooting and event correlation.

Suggested implementation:

```rust
                Err(mpsc::error::TrySendError::Full(_)) => {
                    error!(?frame, "push queue and DLQ full; frame lost");
                }
                Err(mpsc::error::TrySendError::Closed(_)) => {
                    error!(?frame, "DLQ closed; frame lost");
                }

```

If `frame` does not implement `Debug`, or if you want to log a specific identifier (e.g., `frame.id`), replace `?frame` with the appropriate field, such as `frame_id = frame.id`. For example:
```rust
error!(frame_id = frame.id, "push queue and DLQ full; frame lost");
```
Make sure the logging macro you use supports structured fields as shown.
</issue_to_address>

### Comment 2
<location> `src/connection.rs:194` </location>
<code_context>
         // will be drained and any streaming response is abandoned. This mirrors
         // a hard shutdown and is required for the tests.
         if self.shutdown.is_cancelled() {
+            info!("connection aborted before start");
+            let _ = self.counter.take();
             return Ok(());
</code_context>

<issue_to_address>
The log message for aborted connections could include connection context.

Adding details like connection ID or remote address will help correlate logs and troubleshoot issues in environments with multiple connections.

Suggested implementation:

```rust
        if self.shutdown.is_cancelled() {
            info!(
                "connection aborted before start: id={:?}, remote_addr={:?}",
                self.connection_id, self.remote_addr
            );
            let _ = self.counter.take();
            return Ok(());
        }

```

This change assumes that `self.connection_id` and `self.remote_addr` are fields on the struct. If they are named differently or not present, replace them with the appropriate identifiers or add them to the struct as needed.
</issue_to_address>

### Comment 3
<location> `src/connection.rs:204` </location>
<code_context>
             self.poll_sources(&mut state, out).await?;
         }
-
+        info!("connection closed");
+        let _ = self.counter.take();
         Ok(())
</code_context>

<issue_to_address>
Closing connection log could benefit from additional context.

Include connection identifiers in the 'connection closed' log message to enhance traceability.

Suggested implementation:

```rust
        info!(
            "connection closed";
            "connection_id" = ?self.id,
            "peer_addr" = ?self.peer_addr
        );

```

If your struct does not have `id` or `peer_addr` fields, replace them with the actual identifiers available in your context. If you use a different logging style (e.g., not structured logging), format the message accordingly, such as:
```rust
info!("connection closed: id={:?}, peer_addr={:?}", self.id, self.peer_addr);
```
</issue_to_address>

### Comment 4
<location> `src/app.rs:534` </location>
<code_context>

         if let Err(e) = self.process_stream(&mut stream, &routes).await {
-            log::warn!("connection terminated with error: {e}");
+            tracing::warn!("connection terminated with error: {e}");
         }

</code_context>

<issue_to_address>
Switching to tracing for warnings is consistent, but consider structured logging.

Consider using structured fields (e.g., error = ?e) in the tracing log for better machine readability and queryability.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
        if let Err(e) = self.process_stream(&mut stream, &routes).await {
            tracing::warn!("connection terminated with error: {e}");
        }
=======
        if let Err(e) = self.process_stream(&mut stream, &routes).await {
            tracing::warn!(error = ?e, "connection terminated with error");
        }
>>>>>>> REPLACE

</suggested_fix>

### Comment 5
<location> `src/app.rs:661` </location>
<code_context>
             Err(e) => {
                 *deser_failures += 1;
-                log::warn!("failed to deserialize message: {e}");
+                tracing::warn!("failed to deserialize message: {e}");
                 if *deser_failures >= MAX_DESER_FAILURES {
                     return Err(io::Error::new(
</code_context>

<issue_to_address>
Warn log for deserialization failure could use structured logging.

Use structured logging (e.g., error = ?e) to make deserialization failure logs more consistent and searchable.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
                tracing::warn!("failed to deserialize message: {e}");
=======
                tracing::warn!(error = ?e, "failed to deserialize message");
>>>>>>> REPLACE

</suggested_fix>

### Comment 6
<location> `src/app.rs:681` </location>
<code_context>
                     };
                     if let Err(e) = self.send_response(stream, &response).await {
-                        log::warn!("failed to send response: {e}");
+                        tracing::warn!("failed to send response: {e}");
                     }
                 }
</code_context>

<issue_to_address>
Failed response send log could be structured for better observability.

Consider logging the error as a structured field to improve log filtering and analysis.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
                        tracing::warn!("failed to send response: {e}");
=======
                        tracing::warn!(error = %e, "failed to send response");
>>>>>>> REPLACE

</suggested_fix>

### Comment 7
<location> `src/app.rs:685` </location>
<code_context>
                 }
                 Err(e) => {
-                    log::warn!("handler error for id {}: {e}", env.id);
+                    tracing::warn!("handler error for id {}: {e}", env.id);
                 }
             }
</code_context>

<issue_to_address>
Handler error log should use structured fields for id and error.

Use structured logging fields (e.g., id = env.id, error = ?e) to improve log consistency and traceability.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
                    tracing::warn!("handler error for id {}: {e}", env.id);
=======
                    tracing::warn!(id = env.id, error = ?e, "handler error");
>>>>>>> REPLACE

</suggested_fix>

### Comment 8
<location> `tests/connection_actor.rs` </location>
<code_context>
@@ async fn protocol_error_logs_warning(
</code_context>

<issue_to_address>
Missing test coverage for connection counter decrement on drop.

Please add a test to verify that the active connections metric is decremented when a connection is dropped or closed, including cases where the connection is aborted before starting or closed normally.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/push.rs Outdated
Comment thread src/connection.rs Outdated
Comment thread src/connection.rs Outdated
Comment thread src/app.rs
Comment thread src/app.rs Outdated
Comment thread src/app.rs Outdated
Comment thread src/app.rs Outdated
Comment thread tests/connection_actor.rs
@leynos
Copy link
Copy Markdown
Owner Author

leynos commented Jul 14, 2025

@sourcery-ai resolve

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (7)
src/app.rs (4)

534-534: LGTM: Structured logging migration addresses previous feedback.

The transition to tracing::warn! with structured error fields improves observability and aligns with the codebase's logging standardisation.


661-661: LGTM: Structured error logging improves diagnostics.

The structured error field enhances machine readability and filtering capabilities for deserialisation failures.


681-681: LGTM: Response send failure logging enhanced.

The structured error logging improves troubleshooting capabilities for response transmission issues.


685-685: LGTM: Handler error logging includes essential context.

The structured fields for both request ID and error details provide excellent traceability for debugging handler failures.

src/push.rs (1)

179-183: LGTM: Enhanced error logging includes frame details.

The structured frame logging improves troubleshooting capabilities by providing specific frame information when DLQ operations fail.

tests/connection_actor.rs (2)

468-487: Well-structured test for connection abort scenario.

This test properly verifies that the active connection count is maintained correctly when a connection is aborted before starting. The use of #[serial] is appropriate for testing global state.


489-508: Comprehensive test for normal connection close.

This test complements the abort test by verifying the connection count is properly maintained during normal connection lifecycle. Together, these tests provide excellent coverage of the connection counting feature.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 039257e and f1d7641.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • Cargo.toml (1 hunks)
  • docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md (2 hunks)
  • docs/wireframe-1-0-detailed-development-roadmap.md (2 hunks)
  • src/app.rs (3 hunks)
  • src/connection.rs (6 hunks)
  • src/push.rs (5 hunks)
  • tests/connection_actor.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

  • AGENTS.md
docs/**/*.md

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

  • AGENTS.md
  • docs/asynchronous-outbound-messaging-roadmap.md
  • docs/contents.md
  • docs/documentation-style-guide.md
  • docs/wireframe-1-0-detailed-development-roadmap.md
**/*.md

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

  • AGENTS.md

⚙️ CodeRabbit Configuration File

**/*.rs

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

  • AGENTS.md

⚙️ CodeRabbit Configuration File

🧬 Code Graph Analysis (1)
tests/connection_actor.rs (2)
wireframe_testing/src/logging.rs (2)
  • logger (46-46)
  • new (21-28)
src/connection.rs (5)
  • new (30-33)
  • new (135-148)
  • new (504-513)
  • active_connection_count (42-42)
  • shutdown_token (194-194)
🪛 LanguageTool
docs/wireframe-1-0-detailed-development-roadmap.md

[misspelling] ~8-~8: This word is normally spelled with a hyphen.
Context: ...our distinct phases, each with a set of well- defined tasks. The dependencies between tasks a...

(EN_COMPOUNDS_WELL_DEFINED)


[grammar] ~23-~23: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ...ilt.* | Item | Name | Details | Size | Depends | | ---- | ----------------------------...

(DEPEND_ON)


[typographical] ~25-~25: To join two clauses or introduce examples, consider using an em dash.
Context: ...tween I/O and protocol errors. | Small | - | | 1.2 | Priority Push Channels | Imple...

(DASH_RULE)


[typographical] ~26-~26: To join two clauses or introduce examples, consider using an em dash.
Context: ...d low-priority pushed frames. | Medium | - | | 1.3 | Connection Actor Write Loop | ...

(DASH_RULE)


[style] ~27-~27: Consider using the typographical ellipsis character here instead.
Context: ...stateful connection actors. Implement a select!(biased; ...) loop that polls for shutdown signals,...

(ELLIPSIS)


[uncategorized] ~27-~27: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...p that polls for shutdown signals, high/low priority pushes and the handler response stream ...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[typographical] ~28-~28: To join two clauses or introduce examples, consider using an em dash.
Context: ..._headerandencode_header`. | Medium | - | | 1.5 | Basic FragmentAdapter | Implem...

(DASH_RULE)


[uncategorized] ~29-~29: Possible missing comma found.
Context: ...for a single, non-multiplexed stream of fragments and the outbound logic for splitting a ...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~73-~73: Do not mix variants of the same word (‘finalise’ and ‘finalize’) within a single text.
Context: ...All | | 4.6 | Changelog & 1.0 Release | Finalise the CHANGELOG.md with a comprehensive...

(EN_WORD_COHERENCY)

⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-test
🔇 Additional comments (15)
Cargo.toml (1)

17-17: LGTM: Dependency addition supports the logging migration.

The explicit version constraint and relevant features properly enable the transition from the log crate to tracing with structured logging capabilities.

docs/wireframe-1-0-detailed-development-roadmap.md (1)

73-73: Fix spelling consistency.

Maintain consistent spelling throughout the document:

-| 4.6 | Changelog & 1.0 Release | Finalise the `CHANGELOG.md` with a comprehensive...
+| 4.6 | Changelog & 1.0 Release | Finalise the `CHANGELOG.md` with a comprehensive...

Use "finalise" consistently as per the British English requirement in the coding guidelines.

Likely an incorrect or invalid review comment.

docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md (2)

292-312: Excellent addition: Connection lifecycle diagram enhances documentation clarity.

The Mermaid sequence diagram effectively illustrates the ACTIVE_CONNECTIONS metric lifecycle and tracing instrumentation mentioned in the PR objectives. The visual representation clearly shows the connection opening, span creation, and proper cleanup scenarios.


345-349: LGTM: Table formatting improvements enhance readability.

The more compact table formatting reduces visual clutter whilst maintaining all essential information.

src/app.rs (1)

689-689: LGTM: Simple warning appropriate for missing handler scenario.

The straightforward message is suitable since there's no error object to structure—the message ID is embedded in the string.

src/push.rs (4)

114-116: LGTM: Debug logging enhances push operation visibility.

The structured priority field provides valuable context for debugging frame push operations.


172-174: LGTM: Debug trait bound enables structured frame logging.

The trait bound supports the enhanced error logging that includes frame details for better traceability.


225-227: LGTM: Debug trait bound supports enhanced warning logs.

The trait bound enables the structured warning logging that includes frame context and policy information.


240-245: LGTM: Comprehensive structured warning with operational context.

The structured fields (priority, policy, DLQ availability) provide excellent operational visibility for queue full scenarios.

tests/connection_actor.rs (1)

304-311: Good improvement to test robustness.

The test now correctly searches through all log records instead of assuming the warning is the first one. This makes the test more resilient to changes in logging order.

src/connection.rs (5)

22-42: Well-implemented RAII guard for connection tracking.

The ActiveConnection guard correctly uses atomic operations to maintain the global connection count. The use of Relaxed ordering is appropriate for a simple counter.


159-183: Good integration of connection tracking in constructor.

The constructor properly creates the ActiveConnection guard and logs the connection opening event with the current active count. The structured logging format is well-designed.


204-230: Excellent lifecycle management with proper cleanup.

The run method correctly handles both early abort and normal close scenarios, ensuring the connection counter is decremented in all cases. The tracing span provides valuable context for debugging.


427-427: Correct migration to tracing with improved structured logging.

The change from log::warn! to tracing::warn! with structured fields improves log analysis capabilities.


112-113: Ignore outdated suggestion about connection_id and peer_addr fields
The ConnectionActor no longer declares connection_id or peer_addr—those were replaced by the single ctx: ConnectionContext field. The original comment about populating those two fields no longer applies.

Likely an incorrect or invalid review comment.

Comment thread docs/wireframe-1-0-detailed-development-roadmap.md
Comment on lines +23 to +30
| Item | Name | Details | Size | Depends |
| ---- | ------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ | ------- |
| 1.1 | Core Response & Error Types | Define the new `Response<F, E>` enum with `Single`, `Vec`, `Stream` and `Empty` variants. Implement the generic `WireframeError<E>` enum to distinguish between I/O and protocol errors. | Small | - |
| 1.2 | Priority Push Channels | Implement the internal dual-channel `mpsc` mechanism within the connection state to handle high-priority and low-priority pushed frames. | Medium | - |
| 1.3 | Connection Actor Write Loop | Convert per-request workers into stateful connection actors. Implement a `select!(biased; ...)` loop that polls for shutdown signals, high/low priority pushes and the handler response stream in that strict order. | Large | #1.2 |
| 1.4 | Initial FragmentStrategy Trait | Define the initial `FragmentStrategy` trait and the `FragmentMeta` struct. Focus on the core methods: `decode_header` and `encode_header`. | Medium | - |
| 1.5 | Basic FragmentAdapter | Implement the `FragmentAdapter` as a `FrameProcessor`. Build the inbound reassembly logic for a single, non-multiplexed stream of fragments and the outbound logic for splitting a single large frame. | Large | #1.4 |
| 1.6 | Internal Hook Plumbing | Add the invocation points for the protocol-specific hooks (`before_send`, `on_command_end`, etc.) within the connection actor, even if the public trait is not yet defined. | Small | #1.3 |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Fix table header and improve typographical elements.

Address the grammar issue in the table header and improve dash usage:

-| Item | Name | Details | Size | Depends |
+| Item | Name | Details | Size | Depends on |

Also replace the plain dashes with em dashes in the "Depends on" column for better typography.

📝 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.

Suggested change
| Item | Name | Details | Size | Depends |
| ---- | ------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ | ------- |
| 1.1 | Core Response & Error Types | Define the new `Response<F, E>` enum with `Single`, `Vec`, `Stream` and `Empty` variants. Implement the generic `WireframeError<E>` enum to distinguish between I/O and protocol errors. | Small | - |
| 1.2 | Priority Push Channels | Implement the internal dual-channel `mpsc` mechanism within the connection state to handle high-priority and low-priority pushed frames. | Medium | - |
| 1.3 | Connection Actor Write Loop | Convert per-request workers into stateful connection actors. Implement a `select!(biased; ...)` loop that polls for shutdown signals, high/low priority pushes and the handler response stream in that strict order. | Large | #1.2 |
| 1.4 | Initial FragmentStrategy Trait | Define the initial `FragmentStrategy` trait and the `FragmentMeta` struct. Focus on the core methods: `decode_header` and `encode_header`. | Medium | - |
| 1.5 | Basic FragmentAdapter | Implement the `FragmentAdapter` as a `FrameProcessor`. Build the inbound reassembly logic for a single, non-multiplexed stream of fragments and the outbound logic for splitting a single large frame. | Large | #1.4 |
| 1.6 | Internal Hook Plumbing | Add the invocation points for the protocol-specific hooks (`before_send`, `on_command_end`, etc.) within the connection actor, even if the public trait is not yet defined. | Small | #1.3 |
| Item | Name | Details | Size | Depends on |
| ---- | ------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ | ------- |
| 1.1 | Core Response & Error Types | Define the new `Response<F, E>` enum with `Single`, `Vec`, `Stream` and `Empty` variants. Implement the generic `WireframeError<E>` enum to distinguish between I/O and protocol errors. | Small | |
| 1.2 | Priority Push Channels | Implement the internal dual-channel `mpsc` mechanism within the connection state to handle high-priority and low-priority pushed frames. | Medium | |
| 1.3 | Connection Actor Write Loop | Convert per-request workers into stateful connection actors. Implement a `select!(biased; ...)` loop that polls for shutdown signals, high/low priority pushes and the handler response stream in that strict order. | Large | #1.2 |
| 1.4 | Initial FragmentStrategy Trait | Define the initial `FragmentStrategy` trait and the `FragmentMeta` struct. Focus on the core methods: `decode_header` and `encode_header`. | Medium | |
| 1.5 | Basic FragmentAdapter | Implement the `FragmentAdapter` as a `FrameProcessor`. Build the inbound reassembly logic for a single, non-multiplexed stream of fragments and the outbound logic for splitting a single large frame. | Large | #1.4 |
| 1.6 | Internal Hook Plumbing | Add the invocation points for the protocol-specific hooks (`before_send`, `on_command_end`, etc.) within the connection actor, even if the public trait is not yet defined. | Small | #1.3 |
🧰 Tools
🪛 LanguageTool

[grammar] ~23-~23: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ...ilt.* | Item | Name | Details | Size | Depends | | ---- | ----------------------------...

(DEPEND_ON)


[typographical] ~25-~25: To join two clauses or introduce examples, consider using an em dash.
Context: ...tween I/O and protocol errors. | Small | - | | 1.2 | Priority Push Channels | Imple...

(DASH_RULE)


[typographical] ~26-~26: To join two clauses or introduce examples, consider using an em dash.
Context: ...d low-priority pushed frames. | Medium | - | | 1.3 | Connection Actor Write Loop | ...

(DASH_RULE)


[style] ~27-~27: Consider using the typographical ellipsis character here instead.
Context: ...stateful connection actors. Implement a select!(biased; ...) loop that polls for shutdown signals,...

(ELLIPSIS)


[uncategorized] ~27-~27: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...p that polls for shutdown signals, high/low priority pushes and the handler response stream ...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[typographical] ~28-~28: To join two clauses or introduce examples, consider using an em dash.
Context: ..._headerandencode_header`. | Medium | - | | 1.5 | Basic FragmentAdapter | Implem...

(DASH_RULE)


[uncategorized] ~29-~29: Possible missing comma found.
Context: ...for a single, non-multiplexed stream of fragments and the outbound logic for splitting a ...

(AI_HYDRA_LEO_MISSING_COMMA)

🤖 Prompt for AI Agents
In docs/wireframe-1-0-detailed-development-roadmap.md around lines 23 to 30, fix
the table header by correcting the grammar in the "Depends" column header to
"Depends on" and replace all plain dashes in the "Depends on" column with em
dashes for improved typography. Ensure the table alignment remains correct after
these changes.

@leynos leynos merged commit 57a8152 into main Jul 15, 2025
5 checks passed
@leynos leynos deleted the pseqvu-codex/implement-pervasive-observability-in-wireframe-1.0 branch July 15, 2025 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant