feat: send over TCP instead of UDP - #36
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSyslog delivery changes from UDP to lwIP TCP with collector port ChangesTCP syslog transport
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
15-17: 🔒 Security & Privacy | 🔵 TrivialDocument that this stage uses plaintext TCP.
TCP improves stream delivery but does not provide confidentiality or authentication. Add a note that sensitive logs still require TLS/mTLS or another authenticated transport; the upstream library describes RFC 6587 plain TCP as a legacy/interoperability option. (github.com)
🤖 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 `@README.md` around lines 15 - 17, Update the README section describing the TCP sender to explicitly state that it uses plaintext RFC 6587 TCP without confidentiality or authentication. Add guidance that sensitive logs require TLS/mTLS or another authenticated transport, while retaining the existing delivery and framing description.
🤖 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 `@README.md`:
- Around line 15-20: Update the README section describing TCP delivery to remove
claims that failed records are retained or retried, and avoid equating TCP
retransmission with collector-level acceptance. Keep only guarantees supported
by the current SolidSyslogNullStore-based sender configuration, unless the
implementation is changed to provide actual store-and-forward behavior.
---
Nitpick comments:
In `@README.md`:
- Around line 15-17: Update the README section describing the TCP sender to
explicitly state that it uses plaintext RFC 6587 TCP without confidentiality or
authentication. Add guidance that sensitive logs require TLS/mTLS or another
authenticated transport, while retaining the existing delivery and framing
description.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a38459c2-70e1-452f-b8fa-3e903a1d81fc
⛔ Files ignored due to path filters (2)
measurements/stages.tsvis excluded by!**/*.tsvmeasurements/tcp.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
README.mdapp/syslog/Syslog.crun-report.md
A StreamSender over an lwIP TCP stream replaces the UDP sender, and the collector moves to 5601. Records are framed by octet count (RFC 6587), which is what a receiver expects on a stream transport. Flash +7,320 B (+532 on the previous stage) RAM +5,856 B (+180) Log stack +448 B (unchanged) Service +672 B (-56) TCP before TLS is deliberate. It is the smaller step — a stream, a connect and a framing rule, with no certificates in the picture — and it gives store-and-forward somewhere to spool before crypto arrives. It also completes what the sequenceId started: the transport now detects loss where it happens, while the sequence only reveals it afterwards. The flash is small because the baseline already holds a real mTLS session, so lwIP's TCP code was in the image before this stage asked for it. What is charged here is the stream and the sender in front of it, and the 180 bytes of RAM are those two objects in SolidSyslog's static pool. The log stack does not move at all: a task that calls Log is unaffected by the transport underneath it. The service seam's high-water actually fell 56 bytes — this stream's send path measures shallower than the datagram's — but the seam stays 2 KiB, so nothing is given back. The stream takes a Sleep callback because a connect is not instantaneous and the library will not pick a blocking primitive on your behalf; one vTaskDelay is the whole of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pull request
What this stage adds
A StreamSender over an lwIP TCP stream replaces the UDP sender, and the collector
moves to 5601. Records are framed by octet count (RFC 6587), which is what a
receiver expects on a stream transport.
Flash +7,320 B (+532 on the previous stage)
RAM +5,856 B (+180)
Log stack +448 B (unchanged)
Service +672 B (-56)
TCP before TLS is deliberate. It is the smaller step — a stream, a connect and a
framing rule, with no certificates in the picture — and it gives store-and-forward
somewhere to spool before crypto arrives. It also completes what the sequenceId
started: the transport now detects loss where it happens, while the sequence only
reveals it afterwards.
The flash is small because the baseline already holds a real mTLS session, so
lwIP's TCP code was in the image before this stage asked for it. What is charged
here is the stream and the sender in front of it, and the 180 bytes of RAM are
those two objects in SolidSyslog's static pool.
The log stack does not move at all: a task that calls Log is unaffected by the
transport underneath it. The service seam's high-water actually fell 56 bytes —
this stream's send path measures shallower than the datagram's — but the seam
stays 2 KiB, so nothing is given back. The stream takes a Sleep callback because
a connect is not instantaneous and the library will not pick a blocking primitive
on your behalf; one vTaskDelay is the whole of it.
Files
Checklist
measurements/tcp.csvcommitted.measurements/stages.tsv.python3 scripts/gen-cost-table.py../run.shgreen (build + QEMU + baseline self-check) —run-report.mdcommitted.Context for review: 9 of 21 sequential single-commit PRs replaying the integration on top of the Baseline root. Each lands green and reviewed before the next is built on it, because a change to an early commit would force every commit above it to be re-run and re-measured.
Summary by CodeRabbit
New Features
Documentation