Consolidate logging to single .nats scope #67
Conversation
- Create shared log.zig module with single .nats scope - Replace all module-specific scoped loggers with shared logger - Update tests to use std.log.default for test-specific logging - Remove redundant std.debug.print in jetstream.zig This allows clients to easily filter or disable all NATS library logs using the single .nats scope while keeping test logs separate.
WalkthroughCentralizes logging by adding Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dispatcher.zig (1)
260-271: Guard against ref_count underflow in releaseGlobalPool
global_pool_ref_countisu32. Calling release more times than acquire will underflow (debug builds trap; release builds wrap), leaking the pool and threads.Apply:
pub fn releaseGlobalPool() void { global_pool_mutex.lock(); defer global_pool_mutex.unlock(); - global_pool_ref_count -= 1; + if (global_pool_ref_count == 0) { + log.err("releaseGlobalPool called with ref_count=0; ignoring", .{}); + return; + } + global_pool_ref_count -= 1; if (global_pool_ref_count == 0) { log.debug("Last reference released, shutting down global dispatcher pool", .{}); if (global_pool) |pool| { pool.deinit(); global_pool = null; } } else { log.debug("Global dispatcher pool released, ref count: {}", .{global_pool_ref_count}); } }
🧹 Nitpick comments (8)
tests/jetstream_stream_test.zig (1)
6-6: Drop unused logger alias in this test file
logisn’t used here. Remove it to avoid dead code.-const log = std.log.default;tests/minimal_test.zig (1)
5-5: Remove unusedlogaliasThis test doesn’t call
log.*; drop the alias.-const log = std.log.default;src/log.zig (1)
3-3: Optional: add a short doc comment.A brief note on why .nats exists (consumer filtering) would help future readers.
- pub const log = std.log.scoped(.nats); + /// Central logging scope for the NATS library. + /// Consumers can filter/disable all library logs via this scope. + pub const log = std.log.scoped(.nats);src/socket.zig (1)
17-17: Drop unused logger importThis file doesn’t log anything. Keeping an unused top-level const is noise and risks future “unused” checks failing.
Apply:
-const log = @import("log.zig").log;src/dispatcher.zig (2)
216-234: Cap thread pool size from env to avoid accidental fork bombsA fat-fingered NATS_THREAD_POOL_MAX can spawn hundreds of threads. Soft-cap it and log.
Example:
fn getThreadPoolSize(allocator: Allocator) usize { + const max_threads: usize = 64; if (std.process.getEnvVarOwned(allocator, "NATS_THREAD_POOL_MAX")) |env_value| { defer allocator.free(env_value); if (std.fmt.parseInt(usize, env_value, 10)) |size| { if (size > 0) { - log.debug("Using NATS_THREAD_POOL_MAX={}", .{size}); - return size; + const capped = if (size > max_threads) blk: { + log.warn("NATS_THREAD_POOL_MAX={} exceeds cap {}; using cap", .{ size, max_threads }); + break :blk max_threads; + } else size; + log.debug("Using NATS_THREAD_POOL_MAX={}", .{capped}); + return capped; } else { log.warn("NATS_THREAD_POOL_MAX must be > 0, using default", .{}); } } else |_| { log.warn("Invalid NATS_THREAD_POOL_MAX value '{s}', using default", .{env_value}); } } else |_| { // Environment variable not set, use default } return 1; // Default size }
239-247: Consider spawning outside the global lockMinor: starting threads while holding
global_pool_mutexincreases contention. You can create and assign under lock, then start outside. If you keep it as-is for simplicity, fine.If you want to check how often acquire/release are called (to justify the change), we can add temporary counters with logs; say the word and I’ll sketch it.
src/parser.zig (1)
749-749: Downgrade chatty per-iteration logs to debugThese run inside tight loops over chunk sizes and spam CI logs at info level.
Apply:
- log.info("chunk_size: {}", .{chunk_size}); + log.debug("chunk_size: {}", .{chunk_size});Also applies to: 769-769, 793-793, 811-811, 835-835
src/jetstream.zig (1)
751-757: Tone down or enrich the “adding consumer” log
infowithout context is noisy in apps using this lib. Either make itdebug, or include stream/consumer for signal.Option A (debug):
- log.info("adding consumer", .{}); + log.debug("adding consumer", .{});Option B (keep info, add context):
- log.info("adding consumer", .{}); + const cname = if (config.name) |n| n else (config.durable_name orelse "ephemeral"); + log.info("adding consumer: stream={s}, consumer={s}", .{ stream_name, cname });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
src/connection.zig(1 hunks)src/dispatcher.zig(1 hunks)src/jetstream.zig(1 hunks)src/log.zig(1 hunks)src/parser.zig(6 hunks)src/response_manager.zig(1 hunks)src/socket.zig(1 hunks)src/subscription.zig(1 hunks)tests/core_request_reply_test.zig(1 hunks)tests/headers_test.zig(1 hunks)tests/jetstream_duplicate_ack_test.zig(1 hunks)tests/jetstream_large_list_test.zig(1 hunks)tests/jetstream_nak_test.zig(1 hunks)tests/jetstream_push_test.zig(1 hunks)tests/jetstream_stream_test.zig(1 hunks)tests/jetstream_sync_test.zig(1 hunks)tests/jetstream_test.zig(1 hunks)tests/minimal_test.zig(1 hunks)tests/reconnection_test.zig(1 hunks)tests/subscribe_test.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
Format code using
zig fmtbefore commit
Files:
tests/headers_test.zigtests/jetstream_stream_test.zigtests/jetstream_push_test.zigsrc/connection.zigsrc/dispatcher.zigtests/jetstream_large_list_test.zigtests/reconnection_test.zigtests/minimal_test.zigtests/jetstream_test.zigsrc/socket.zigtests/core_request_reply_test.zigsrc/response_manager.zigsrc/parser.zigtests/jetstream_duplicate_ack_test.zigtests/subscribe_test.zigsrc/subscription.zigsrc/log.zigtests/jetstream_nak_test.zigsrc/jetstream.zigtests/jetstream_sync_test.zig
🧠 Learnings (5)
📓 Common learnings
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.
📚 Learning: 2025-08-29T10:22:22.247Z
Learnt from: CR
PR: lalinsky/nats.zig#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T10:22:22.247Z
Learning: Run `zig build test` to confirm your changes are working
Applied to files:
tests/headers_test.zigtests/jetstream_test.zig
📚 Learning: 2025-08-25T14:51:33.570Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.
Applied to files:
tests/jetstream_stream_test.zigtests/jetstream_push_test.zigtests/jetstream_large_list_test.zigtests/minimal_test.zigtests/jetstream_test.zigtests/jetstream_duplicate_ack_test.zigtests/jetstream_nak_test.zigtests/jetstream_sync_test.zig
📚 Learning: 2025-08-25T02:49:59.119Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#28
File: src/response_manager.zig:89-105
Timestamp: 2025-08-25T02:49:59.119Z
Learning: In this NATS Zig codebase, subscriptions are reference counted, so calling deinit() on a subscription from multiple locations (like both Connection and ResponseManager) is safe and doesn't cause double-free issues.
Applied to files:
tests/jetstream_push_test.zig
📚 Learning: 2025-08-30T18:48:28.310Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#66
File: src/connection.zig:317-323
Timestamp: 2025-08-30T18:48:28.310Z
Learning: In src/connection.zig, the establishConnection() call in connectToServer() is during initial connection setup where no race conditions exist - the connection starts disconnected and other threads would see the connecting status and wait appropriately.
Applied to files:
src/response_manager.zig
🔇 Additional comments (19)
tests/subscribe_test.zig (1)
6-6: Using std.log.default in tests is the right call.Keeps test logs separate from the library’s .nats scope as intended. LGTM.
tests/core_request_reply_test.zig (1)
5-5: Tests bound to std.log.default look good.Aligns with the PR goal of isolating library logs under .nats while letting tests use the default logger.
tests/reconnection_test.zig (1)
5-5: Good switch to std.log.default for test logging.Matches the unified logging approach.
tests/jetstream_test.zig (1)
6-6: Test logger moved to std.log.default — approved.Consistent with a single .nats scope for the library.
tests/jetstream_push_test.zig (1)
6-6: Switch to std.log.default is consistent and correct.Keeps push tests’ output independent from the library scope.
tests/jetstream_large_list_test.zig (1)
6-6: LGTM: switch to std.log.default for testsMatches the PR goal of isolating test logs from the shared
.natsscope. No functional impact.tests/headers_test.zig (1)
5-5: LGTM: test logging moved to default loggerKeeps test output separate from
.nats-scoped library logs. All usages compile againststd.log.default.tests/jetstream_nak_test.zig (1)
6-6: LGTM: use default logger for testsConsistent with the unified
.natsscope for library code and separate test logging.tests/jetstream_sync_test.zig (1)
6-6: LGTM: tests now use std.log.default.This cleanly separates test logs from the library’s .nats scope as intended.
src/log.zig (2)
3-3: Single .nats scope looks good.Central export keeps callsites simple and consistent.
3-3: All log scope and debug-print checks passed: nostd.log.scopedcalls outsidesrc/log.zig, nostd.debug.printinsrc/, no scoped logs in tests, and all modules import the central logger consistently.src/response_manager.zig (1)
25-25: LGTM: switched to central .nats logger.Consistent with the PR goal; usage sites already call log.debug/warn here.
src/connection.zig (1)
39-39: LGTM: centralized logger import.Matches the new logging approach; no behavior change.
src/dispatcher.zig (2)
20-20: Centralized logger import looks goodAligned with the single .nats scope goal.
183-197: Stop/start logs are helpful; keep them at infoThese are the right granularity for ops.
tests/jetstream_duplicate_ack_test.zig (1)
6-6: Switching tests to std.log.default is the right callKeeps library logs filterable via .nats while tests remain visible.
src/parser.zig (1)
18-18: Centralized logger import for test logs: OKNo impact on parser logic; just aligns test logging.
src/jetstream.zig (2)
22-22: Adopting the shared .nats loggerMatches the PR’s objective and keeps JetStream logs under a single scope.
564-569: Good: replaced ad-hoc print with scoped loggingError context is preserved and now filterable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/utils.zig (1)
4-4: Should this be public?If other tests import this binding from utils, make it
pub; otherwise keeping it private is fine.-const log = std.log.default; +pub const log = std.log.default;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/utils.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
Format code using
zig fmtbefore commit
Files:
tests/utils.zig
🧠 Learnings (2)
📓 Common learnings
Learnt from: lalinsky
PR: lalinsky/nats.zig#67
File: src/dispatcher.zig:20-20
Timestamp: 2025-08-30T21:03:43.986Z
Learning: In the nats.zig project, test_runner.zig and files in the examples/ directory are fine to keep their std.debug.print calls - these don't need to be replaced with the centralized logging system.
📚 Learning: 2025-08-30T21:03:43.986Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#67
File: src/dispatcher.zig:20-20
Timestamp: 2025-08-30T21:03:43.986Z
Learning: In the nats.zig project, test_runner.zig and files in the examples/ directory are fine to keep their std.debug.print calls - these don't need to be replaced with the centralized logging system.
Applied to files:
tests/utils.zig
🔇 Additional comments (1)
tests/utils.zig (1)
4-4: Tests: switch to std.log.default — good callThis keeps test logs separate from the library’s
.natsscope, matching the PR goal.
1 participant
This allows clients to easily filter or disable all NATS library logs using the single .nats scope while keeping test logs separate.