Improve logging performance and separation (#421)#428
Conversation
Replace container/ring.Ring with a custom circularBuffer that uses a single contiguous []byte slice. This fixes the original implementation which created 10,240 ring elements instead of 10KB of storage. GetHistory is now 139x faster (145μs → 1μs) and uses 117x less memory (1.2MB → 10KB). Allocations reduced from 2 to 1 per write operation. ref #421
WalkthroughReplaces LogMonitor's container/ring buffering with a new unexported circularBuffer type (pre-allocated []byte, head and size) and integrates it into LogMonitor. Adds unit tests and benchmarks, updates CLI README examples, and adjusts process lifecycle to clear per-process log buffers and expose a Logger accessor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 0
🧹 Nitpick comments (1)
proxy/logMonitor.go (1)
37-37: Consider renaming local variable to avoid shadowing built-in.The variable name
capshadows Go's built-incap()function. While this works correctly, consider usingcapacityfor clarity.- cap := len(cb.data) + capacity := len(cb.data)Note: This same pattern appears at line 75 in GetHistory().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ai-plans/2025-12-14-efficient-ring-buffer.md(1 hunks)proxy/logMonitor.go(4 hunks)proxy/logMonitor_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
proxy/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Run
make test-devwhen making iterative changes to code under theproxy/directory - this runsgo testandstaticcheck, and all static checking errors must be fixed
Files:
proxy/logMonitor_test.goproxy/logMonitor.go
ai-plans/**
📄 CodeRabbit inference engine (CLAUDE.md)
When improving plans, identify inconsistencies, expand plans to be detailed specifications, and ensure plans include Title, Overview, Design Requirements, Testing Plan, and Checklist sections
Files:
ai-plans/2025-12-14-efficient-ring-buffer.md
🧠 Learnings (1)
📚 Learning: 2025-11-25T05:40:06.444Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:40:06.444Z
Learning: Applies to proxy/**/*.go : Run `make test-dev` when making iterative changes to code under the `proxy/` directory - this runs `go test` and `staticcheck`, and all static checking errors must be fixed
Applied to files:
proxy/logMonitor_test.go
🧬 Code graph analysis (1)
proxy/logMonitor_test.go (1)
proxy/logMonitor.go (1)
NewLogMonitorWriter(123-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (7)
proxy/logMonitor.go (3)
14-91: Excellent circular buffer implementation!The circularBuffer implementation is correct and efficient:
- Properly handles wrap-around logic in Write()
- Correctly calculates start position in GetHistory() using
(head - size + cap) % cap- Handles edge cases (empty input, oversized writes, exact capacity)
- Maintains O(1) write and O(n) read complexity as designed
- Preserves immutability by returning copies from GetHistory()
The algorithm correctly manages the circular nature: when the buffer is full,
headpoints to both the next write position and the oldest byte (which will be overwritten).
148-151: Good practice: preserving immutability.Creating a copy for broadcast correctly ensures that subsequent modifications to the input slice don't affect subscribers, maintaining the immutability guarantee tested in TestWrite_ImmutableBuffer.
1-12: Confirm import cleanup.The
container/ringimport has been successfully removed fromproxy/logMonitor.go. Verify locally thatmake test-devpasses to ensure all tests and static checks are compliant with coding guidelines.proxy/logMonitor_test.go (3)
117-144: Comprehensive wrap-around testing.TestCircularBuffer_WrapAround provides excellent coverage:
- Sequential writes filling the buffer
- Wrap-around overwrites ("world12345" correctly keeps last 10 bytes)
- Oversized write handling (16 bytes → last 10 bytes kept)
The test validates that the circular buffer correctly maintains FIFO ordering when overwriting old data.
146-166: Good boundary condition coverage.TestCircularBuffer_BoundaryConditions correctly validates:
- Empty buffer returns nil (line 149)
- Exact capacity writes (10 bytes in 10-byte buffer)
- Split writes at capacity boundary (5+5 bytes)
These edge cases ensure the buffer handles capacity limits correctly.
168-248: Excellent benchmark suite with impressive results.The benchmarks comprehensively measure:
- Various message sizes (small, medium, large)
- Impact of subscribers
- GetHistory performance
The documented results show significant improvements:
- GetHistory: 139x faster, 117x less memory
- Consistent 2→1 allocation reduction across all operations
- Small/medium writes: 1.1-1.6x faster
These dramatic improvements, especially for GetHistory, validate the design decision to replace ring.Ring with the custom circular buffer.
ai-plans/2025-12-14-efficient-ring-buffer.md (1)
1-85: Well-structured plan document with clear specifications.The plan document follows the required format and includes all necessary sections:
- ✓ Title and Overview clearly state the objective
- ✓ Design Requirements provide detailed implementation guidance (struct fields, API methods, complexity guarantees)
- ✓ Implementation Details include concrete code examples
- ✓ Testing Plan covers existing and new test scenarios
- ✓ Checklist provides actionable steps
The plan correctly identifies the issues with
ring.Ring(10,240 elements ≠ 10KB storage, poor cache locality, excessive allocations) and the implementation successfully addresses all these concerns as evidenced by the benchmark results.As per coding guidelines for ai-plans files, this plan meets all requirements for detailed specifications.
This update creates a LogMonitor per proxy.Process, replacing the usage
of a shared one. The buffer in LogMonitor is lazy allocated on the first
call to Write and freed when the Process is stopped. This reduces
unncessary memory usage when a model is not active.
The /logs/stream/{model_id} endpoint was added to stream logs from a
specific process.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)proxy/logMonitor.go(3 hunks)proxy/logMonitor_test.go(1 hunks)proxy/process.go(2 hunks)proxy/processgroup.go(2 hunks)proxy/proxymanager_loghandlers.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- proxy/logMonitor_test.go
- proxy/logMonitor.go
🧰 Additional context used
📓 Path-based instructions (1)
proxy/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Run
make test-devwhen making iterative changes to code under theproxy/directory - this runsgo testandstaticcheck, and all static checking errors must be fixed
Files:
proxy/processgroup.goproxy/process.goproxy/proxymanager_loghandlers.go
🧠 Learnings (1)
📚 Learning: 2025-06-05T22:57:53.793Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 155
File: proxy/process.go:359-372
Timestamp: 2025-06-05T22:57:53.793Z
Learning: In Go, cmdWaitChan in proxy/process.go is a signal channel (chan struct{}) that follows the pattern where waitForCmd() closes the channel when cmd.Wait() returns, and stopCommand() waits on it. Reading from a closed channel returns immediately, so <-cmdWaitChan won't block indefinitely.
Applied to files:
proxy/process.go
🧬 Code graph analysis (2)
proxy/processgroup.go (2)
proxy/logMonitor.go (1)
NewLogMonitorWriter(125-134)proxy/process.go (2)
NewProcess(84-128)Process(43-82)
proxy/process.go (1)
proxy/logMonitor.go (1)
LogMonitor(104-119)
🔇 Additional comments (4)
proxy/proxymanager_loghandlers.go (1)
86-93: LGTM! Switch statement improves readability.The refactoring from an if-else chain to a switch statement with early returns is clean and improves code readability. The handling of the default, proxy, and upstream loggers is correct.
proxy/process.go (2)
417-420: LGTM! Memory cleanup on process stop.The addition of
processLogger.Clear()in the defer ensures that per-process log buffers are freed when a process stops, which aligns with the PR's goal of improving logging performance and memory management. The comment clearly explains the intent.
652-655: LGTM! Clean public API for logger access.The
Logger()method provides clean public access to the process-specific logger, enabling the model-specific logger resolution inproxymanager_loghandlers.go. The implementation is straightforward and correct.proxy/processgroup.go (1)
49-50: LGTM! Per-process logger creation improves separation.The change to create a per-process logger via
NewLogMonitorWriter(upstreamLogger)for each group member improves logging separation and aligns with the PR's objectives. Each process now has its own log buffer that can be independently managed and cleared, as evidenced by theClear()call added inprocess.go:419.
| func (pg *ProcessGroup) GetMember(modelName string) (*Process, bool) { | ||
| if pg.HasMember(modelName) { | ||
| return pg.processes[modelName], true | ||
| } | ||
| return nil, false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ProcessGroup struct definition
fd -e go proxy/ | xargs grep -l "type ProcessGroup"Repository: mostlygeek/llama-swap
Length of output: 396
🏁 Script executed:
#!/bin/bash
# Examine the ProcessGroup struct and its methods
rg -nP --type=go "type ProcessGroup|func \(pg" proxy/processgroup.go | head -50Repository: mostlygeek/llama-swap
Length of output: 539
🏁 Script executed:
#!/bin/bash
# Search for modifications to pg.processes map
rg -nP --type=go -C3 'pg\.processes\[.*\]\s*=' proxy/
# Search for delete operations on processes map
rg -nP --type=go -C3 'delete\s*\(\s*pg\.processes' proxy/
# Search for any assignments or operations on .processes field
rg -nP --type=go -C2 'processes\s*=' proxy/Repository: mostlygeek/llama-swap
Length of output: 509
🏁 Script executed:
#!/bin/bash
# Read the first 150 lines of processgroup.go to see struct and initialization
head -150 proxy/processgroup.go | cat -nRepository: mostlygeek/llama-swap
Length of output: 4492
🏁 Script executed:
#!/bin/bash
# Get all occurrences of pg.processes access and Lock/Unlock calls
rg -nP --type=go "pg\.processes|pg\.Lock|pg\.Unlock|HasMember" proxy/processgroup.goRepository: mostlygeek/llama-swap
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Run make test-dev to check if data race is detected
cd . && make test-dev 2>&1 | head -200Repository: mostlygeek/llama-swap
Length of output: 107
Add mutex protection to GetMember and other map accesses.
The GetMember method accesses pg.processes without acquiring the ProcessGroup mutex, creating a data race. While HasMember safely reads the config, the map access at line 94 is unprotected. Additionally, ProxyRequest (line 84) accesses the map without the lock when pg.swap is false, and Shutdown iterates over processes without synchronization. Acquire pg.Lock() before all map accesses to match the pattern used in StopProcess and StopProcesses.
🤖 Prompt for AI Agents
In proxy/processgroup.go around lines 92 to 97 (and also check ProxyRequest at
~line 84 and Shutdown), the code accesses the pg.processes map without holding
the ProcessGroup mutex which causes data races; wrap all map accesses with
pg.Lock() and defer pg.Unlock() (or use the same locking pattern used in
StopProcess/StopProcesses) — specifically, acquire pg.Lock() before
checking/returning pg.processes[modelName] in GetMember, before reading
pg.processes in ProxyRequest when pg.swap is false, and around the iteration in
Shutdown, then release the lock with defer pg.Unlock() to ensure safe concurrent
access.
There was a problem hiding this comment.
This is not necessary because pg.processes is static and never changes. The code only needs the reference to the Process.
There was a problem hiding this comment.
@mostlygeek, thank you for the clarification! You're absolutely right—if pg.processes is immutable after initialization in NewProcessGroup, then concurrent reads are safe without locking since Go maps support concurrent reads when there are no concurrent writes. The reference retrieval is indeed safe in this case.
I see now that the locking in methods like StopProcess and StopProcesses is protecting other mutable state (like lastUsedProcess) rather than the map itself. This is a helpful design invariant to know!
✏️ Learnings added
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 428
File: proxy/processgroup.go:92-97
Timestamp: 2025-12-19T05:58:38.415Z
Learning: In proxy/processgroup.go, the pg.processes map is immutable after initialization in NewProcessGroup—it's populated once with all group members and never modified afterward (no additions or deletions). Therefore, concurrent reads from pg.processes (such as in GetMember) are safe without mutex protection.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 371
File: proxy/process.go:0-0
Timestamp: 2025-10-29T05:26:34.964Z
Learning: In proxy/process.go, the loading message "llama-swap loading model: {name}" intentionally uses p.ID (Process.ID) rather than the realModelName from the request context. This is the correct design choice.
| default: | ||
| // search for a models specific logger | ||
| if name, found := pm.config.RealModelName(logMonitorId); found { | ||
| for _, group := range pm.processGroups { | ||
| if process, found := group.GetMember(name); found { | ||
| return process.Logger(), nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("invalid logger. Use 'proxy' or 'upstream'") |
There was a problem hiding this comment.
Update the error message to reflect model-specific logger support.
The default case now correctly supports model-specific loggers through RealModelName lookup and GetMember, but the error message on Line 104 still only mentions 'proxy' or 'upstream'. Users might be confused when they try to use a valid model name and receive an error message that doesn't mention this option.
🔎 Proposed fix
- return nil, fmt.Errorf("invalid logger. Use 'proxy' or 'upstream'")
+ return nil, fmt.Errorf("invalid logger. Use 'proxy', 'upstream', or a valid model name")📝 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.
| default: | |
| // search for a models specific logger | |
| if name, found := pm.config.RealModelName(logMonitorId); found { | |
| for _, group := range pm.processGroups { | |
| if process, found := group.GetMember(name); found { | |
| return process.Logger(), nil | |
| } | |
| } | |
| } | |
| return nil, fmt.Errorf("invalid logger. Use 'proxy' or 'upstream'") | |
| default: | |
| // search for a models specific logger | |
| if name, found := pm.config.RealModelName(logMonitorId); found { | |
| for _, group := range pm.processGroups { | |
| if process, found := group.GetMember(name); found { | |
| return process.Logger(), nil | |
| } | |
| } | |
| } | |
| return nil, fmt.Errorf("invalid logger. Use 'proxy', 'upstream', or a valid model name") |
🤖 Prompt for AI Agents
In proxy/proxymanager_loghandlers.go around lines 94 to 104, the error returned
in the default branch still only mentions "proxy" or "upstream" even though the
code now supports model-specific loggers via RealModelName/GetMember; update the
fmt.Errorf to include model names as valid options (e.g., mention model names or
"model-specific logger") so the message reflects that users can request 'proxy',
'upstream', or a model-specific logger; keep the message concise and
user-friendly.
1 participant
Performance improvements using MBP M1 Pro
Before (ring.Ring):
After (circularBuffer):
Summary:
Summary by CodeRabbit
New Features
/logs/stream,/logs/stream/proxy,/logs/stream/upstream,/logs/stream/{model_id}) for enhanced log monitoring on the CLI.Documentation
?no-historyparameter details.Tests
✏️ Tip: You can customize this high-level summary in your review settings.