Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

4 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 21, 2025 2:31am
rivet-inspector Ignored Ignored Nov 21, 2025 2:31am
rivet-site Ignored Ignored Nov 21, 2025 2:31am
rivetkit-serverless Skipped Skipped Nov 21, 2025 2:31am

Copy link
Member Author

NathanFlurry commented Nov 21, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 21, 2025

PR Review: chore(pegboard-runner): log runner messages

Summary

This PR adds debug logging for runner messages in both directions of the WebSocket communication:

  • tunnel_to_ws_task.rs: Logs messages being sent from the tunnel to the client
  • ws_to_tunnel_task.rs: Logs messages received from the client

✅ Positives

  1. Follows logging conventions: Uses structured logging with tracing::debug!(?var, "message") as recommended in CLAUDE.md
  2. Appropriate log level: Uses debug level, which is correct for message tracing in production
  3. Symmetric logging: Adds logging on both message send and receive paths, providing complete visibility
  4. Helpful for debugging: Will aid in troubleshooting WebSocket communication issues

📝 Code Quality Observations

Log message consistency:
The two log messages use slightly different phrasing:

  • Line 121 in tunnel_to_ws_task.rs: "sending runner message to client"
  • Line 68 in ws_to_tunnel_task.rs: "received runner message from client"

Placement difference:

  • In ws_to_tunnel_task.rs (line 68), the log is placed after deserialization, so it logs the parsed message structure
  • In tunnel_to_ws_task.rs (line 121), the log is placed before serialization, so it also logs the parsed message structure

This is actually good - both log the structured message data rather than raw bytes, making debugging easier.

🔍 Potential Considerations

1. Log volume at debug level:
In high-traffic scenarios, these debug logs could generate significant volume. This is acceptable since:

  • Debug logs are typically disabled in production
  • When enabled for troubleshooting, this is exactly the information needed
  • The log level is appropriate for this type of tracing

2. Sensitive data logging:
The logs use ? debug formatting which will dump the entire message structure. Consider:

  • Review if any protocol::ToClient or protocol::ToServer variants contain sensitive data (tokens, credentials, PII)
  • If sensitive data exists, may want to implement custom Debug or use selective field logging

3. Existing log coverage:
I notice there are already logs at different points:

  • Line 41-44 in tunnel_to_ws_task.rs: Logs payload length when receiving from pubsub
  • Line 51-54 in ws_to_tunnel_task.rs: Logs at trace level for binary data

The new logs complement these by showing the parsed protocol messages, which is valuable.

🧪 Test Coverage

Note: No test files were found for the pegboard-runner package. While this PR only adds logging (low risk), consider:

  • Integration tests that verify message flow could benefit from these new log statements
  • The logs could be used in tests to verify correct message handling

🔐 Security Considerations

Low risk: Adding debug logging doesn't introduce security vulnerabilities. However:

  • Ensure debug logs are not exposed in production logging systems that may be accessible to unauthorized parties
  • If messages contain sensitive data, consider adding sanitization or using #[serde(skip_debug)] on sensitive fields

⚡ Performance Considerations

Minimal impact: Debug logging with lazy evaluation means:

  • When debug logging is disabled, only a boolean check is performed
  • Message formatting only occurs if debug level is enabled
  • No performance concerns for this change

📊 Summary

Recommendation: ✅ Approve

This is a straightforward, well-implemented logging improvement that:

  • Follows the project's logging conventions from CLAUDE.md
  • Uses appropriate log levels
  • Provides valuable debugging information
  • Has minimal performance impact
  • Introduces no security vulnerabilities

The only minor suggestion would be to verify that the protocol message types don't contain sensitive data that shouldn't be logged. Otherwise, this is ready to merge.


Review generated via Claude Code

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 2:40 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 2:42 AM UTC: CI is running for this pull request on a draft pull request (#3515) due to your merge queue CI optimization settings.
  • Nov 21, 2:43 AM UTC: Merged by the Graphite merge queue via draft PR: #3515.

@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-chore_pegboard-runner_log_runner_messages branch November 21, 2025 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants