add new Json logs #3
Conversation
WalkthroughAdds a journald-based structured event system and emits versioned KNOCKER_* events across service lifecycle, knock operations, and whitelist management. Introduces a platform-agnostic journald wrapper, integrates event emission into Service and CLI manual-knock paths, updates service lifecycle synchronization, and expands documentation for the event contract. Changes
Sequence Diagram(s)sequenceDiagram
actor CLI as CLI/User
participant prog as program
participant svc as Service
participant journald as journald.Emit
CLI->>prog: knocker knock (manual)
prog->>svc: Start / NewService(version)
svc->>journald: Emit(ServiceState="started")
alt manual knock
CLI->>svc: trigger manual knock
svc->>journald: Emit(KnockTriggered, TriggerSource="cli")
alt success
svc->>journald: Emit(WhitelistApplied)
else failure
svc->>journald: Emit(Error)
end
end
rect rgb(230,245,255)
Note over svc: scheduled loop
svc->>svc: maybe fetch public IP
alt IP changed
svc->>svc: health check -> performKnock()
svc->>journald: Emit(KnockTriggered / WhitelistApplied or Error)
else unchanged
svc->>journald: Emit(StatusSnapshot)
end
end
CLI->>prog: Stop
svc->>journald: Emit(ServiceState="stopping")
svc->>journald: Emit(ServiceState="stopped")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 1
🧹 Nitpick comments (7)
docs/architecture.md (1)
69-72: Fix list indentation for markdown style consistency.The nested list items use 4-space indentation instead of the expected 2 spaces.
Apply this diff to fix the indentation:
-- The core service emits: - - `ServiceState` when entering `started`, `stopping`, or `stopped` transitions (including `KNOCKER_VERSION`). - - `StatusSnapshot` whenever material state changes (whitelist, TTL, next knock timestamp) so consumers can seed their UI. - - `WhitelistApplied`, `WhitelistExpired`, `NextKnockUpdated`, and `KnockTriggered` as the whitelist lifecycle evolves. - - `Error` whenever a problem (IP lookup, health check, knock) should surface in the UI, tagged with `KNOCKER_ERROR_CODE`. +- The core service emits: + - `ServiceState` when entering `started`, `stopping`, or `stopped` transitions (including `KNOCKER_VERSION`). + - `StatusSnapshot` whenever material state changes (whitelist, TTL, next knock timestamp) so consumers can seed their UI. + - `WhitelistApplied`, `WhitelistExpired`, `NextKnockUpdated`, and `KnockTriggered` as the whitelist lifecycle evolves. + - `Error` whenever a problem (IP lookup, health check, knock) should surface in the UI, tagged with `KNOCKER_ERROR_CODE`.go.mod (1)
6-6: Update go-systemd dependency to v22.6.0.v22.5.0 is outdated; v22.6.0 is the current stable release (released August 20, 2025). No security vulnerabilities were detected in the go-systemd repository. v22.6.0 includes bug fixes (thread-safe error handling in dlopen, C wrapper fixes), a new dbus method, and bumps the Go requirement to 1.23—verify this aligns with your project's Go version support before updating.
cmd/knocker/program.go (1)
64-71: Remove redundant NotifyStopping; Stop() already emits it.Service.Stop calls NotifyStopping internally. Calling both is harmless due to sync.Once but redundant.
- if svc != nil { - svc.NotifyStopping() - svc.Stop() - } + if svc != nil { + svc.Stop() + }internal/service/service.go (1)
72-89: Deduplicate NotifyStopping calls to reduce noise.You call NotifyStopping in:
- Run quit case,
- Run s.stop case,
- Service.Stop.
sync.Once protects against duplicate emission, but you can remove one set (e.g., from Run) to simplify reasoning. Not required, just cleaner.
If Run must support external quit without invoking Service.Stop, keep the NotifyStopping in Run and rely on Stop’s call only when Stop() is used.
internal/service/event_logging.go (2)
51-55: Auto-attach KNOCKER_VERSION in emit() so all events carry version.Currently only ServiceState includes version. Centralizing avoids drift.
func (s *Service) emit(eventType, message string, priority journald.Priority, fields journald.Fields) { - if err := journald.Emit(eventType, message, priority, fields); err != nil && s.Logger != nil { + // Copy to avoid mutating caller’s map. + if fields == nil { + fields = journald.Fields{} + } else { + dup := make(journald.Fields, len(fields)+1) + for k, v := range fields { dup[k] = v } + fields = dup + } + if s.version != "" { + fields["KNOCKER_VERSION"] = s.version + } + if err := journald.Emit(eventType, message, priority, fields); err != nil && s.Logger != nil { s.Logger.Printf("Failed to emit journald event %s: %v", eventType, err) } }
91-105: Unify source field naming across events.Whitelist uses KNOCKER_SOURCE while Knock uses KNOCKER_TRIGGER_SOURCE. Consider standardizing to a single key to simplify consumers (before locking schema v1).
Does docs/logging.md recommend one of these? Align code with docs if already defined.
Also applies to: 154-161
internal/journald/journald.go (1)
36-41: Short-circuit Emit when journald isn’t available.Avoid calling emit() on platforms where Enabled() is false.
func Emit(eventType, message string, priority Priority, fields Fields) error { - if journaldDisabled.Load() { + if journaldDisabled.Load() || !Enabled() { return nil }Also applies to: 44-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
README.md(3 hunks)cmd/knocker/knock.go(3 hunks)cmd/knocker/program.go(3 hunks)docs/architecture.md(1 hunks)docs/logging.md(1 hunks)go.mod(1 hunks)internal/journald/journald.go(1 hunks)internal/journald/journald_linux.go(1 hunks)internal/journald/journald_stub.go(1 hunks)internal/service/event_logging.go(1 hunks)internal/service/service.go(4 hunks)internal/service/service_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
internal/service/service.go (3)
internal/api/client.go (2)
Client(11-15)KnockResponse(17-21)internal/util/ip.go (1)
IPGetter(9-11)internal/service/event_logging.go (9)
ServiceStateStarted(22-22)ServiceStateStopped(24-24)ServiceStateStopping(23-23)TriggerSourceSchedule(29-29)ErrorCodeIPLookup(39-39)ErrorCodeHealthCheck(40-40)ResultFailure(35-35)ErrorCodeKnockFailed(41-41)ResultSuccess(34-34)
cmd/knocker/knock.go (3)
internal/journald/journald.go (4)
Emit(47-76)PriErr(18-18)Fields(27-27)PriInfo(21-21)internal/service/event_logging.go (7)
EventKnockTriggered(17-17)TriggerSourceCLI(28-28)ResultFailure(35-35)EventError(18-18)ErrorCodeKnockFailed(41-41)ResultSuccess(34-34)EventWhitelistApplied(14-14)internal/api/client.go (1)
KnockResponse(17-21)
internal/journald/journald_stub.go (1)
internal/journald/journald.go (2)
Priority(11-11)Fields(27-27)
internal/service/service_test.go (2)
internal/service/service.go (1)
NewService(34-45)internal/api/client.go (1)
NewClient(23-29)
internal/journald/journald_linux.go (1)
internal/journald/journald.go (3)
Enabled(38-40)Priority(11-11)Fields(27-27)
internal/service/event_logging.go (2)
internal/service/service.go (1)
Service(16-32)internal/journald/journald.go (6)
Priority(11-11)Fields(27-27)Emit(47-76)PriInfo(21-21)PriNotice(20-20)PriErr(18-18)
cmd/knocker/program.go (1)
internal/service/service.go (2)
Service(16-32)NewService(34-45)
🪛 LanguageTool
README.md
[grammar] ~12-~12: Ensure spelling is correct
Context: ...ist request at any time. - Structured journald events: Emits machine-readable journa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~12-~12: Ensure spelling is correct
Context: ...urnald events:** Emits machine-readable journald events alongside human logs for desktop...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~89-~89: Ensure spelling is correct
Context: ...the configured interval. ## Structured journald events When the service runs under sys...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~100-~100: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...cli) and scheduled (schedule) knocks with success/failure result. - Error` — surfaced ...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
docs/architecture.md
[grammar] ~63-~63: Ensure spelling is correct
Context: ...for easy deployment. ### 8. Structured Journald Events Linux deployments run the servi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture.md
69-69: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (15)
internal/journald/journald_stub.go (1)
1-11: LGTM! Clean cross-platform stub implementation.The build-tagged stub correctly provides no-op behavior for non-Linux platforms, ensuring the codebase remains portable.
README.md (2)
12-12: Excellent addition of structured journald events feature.The new feature documentation clearly explains the machine-readable event integration for desktop environments.
89-103: Well-documented journald integration with practical examples.The structured events section provides clear guidance on schema versioning, event types, and consumption patterns with
journalctl. The note about manual invocations producing the same events ensures consistency for external consumers.docs/architecture.md (1)
63-76: Comprehensive documentation of the journald integration architecture.The new section clearly explains the journald abstraction, event schema, and cross-platform handling. The description of event types and consumption patterns provides valuable context for understanding the implementation.
cmd/knocker/knock.go (3)
33-33: Good integration of structured event emission into the knock flow.The manual knock command now properly emits
KnockTriggeredandWhitelistAppliedevents, maintaining consistency with the background service's event stream.Also applies to: 37-37
48-59: Well-structured failure event emission.The function correctly emits both
KnockTriggered(with failure result) andErrorevents, providing comprehensive observability for manual knock failures. Ignoring journald errors is appropriate since event emission is best-effort and shouldn't block the main error path.
61-105: Thorough success event emission with proper nil handling.The function safely handles nil
knockResponseand conditionally populates fields only when values are available. The structured event emission for bothKnockTriggeredandWhitelistAppliedensures external consumers receive complete state updates.internal/service/service_test.go (1)
43-51: Excellent refactoring to use public API instead of internal details.The test now uses
NewServiceconstructor andStop()method rather than directly manipulating internal fields and channels. This improves test maintainability and respects the service's encapsulation.Also applies to: 63-63
internal/journald/journald_linux.go (2)
9-11: Clean delegation to the systemd journal library.The
enabled()function correctly delegates tojournal.Enabled()for Linux platforms.
13-20: Efficient and correct journald emission implementation.The function pre-allocates the payload map for efficiency, safely copies all fields, and properly type-casts the priority before calling
journal.Send. Error handling is appropriate with direct propagation.docs/logging.md (1)
1-118: Outstanding structured logging documentation.This comprehensive guide covers the complete journald event contract including schema versioning, all event types with detailed field descriptions, practical JSON examples, and parser recommendations. The documentation will be invaluable for developers building integrations with Knocker.
cmd/knocker/program.go (1)
44-47: Confirm fatal-on-healthcheck is intended for service mode.logger.Fatalf will terminate the whole process from the run goroutine. If you prefer graceful startup failure (so the service manager can retry), return an error instead and let the supervisor handle restarts.
internal/service/service.go (3)
47-65: Run lifecycle sequencing looks solid.Startup emits (state → nextKnock → snapshot) and deferred teardown (clear next → snapshot → stopped) are correct and will keep consumers consistent.
145-162: Good centralization of knock side effects.performKnock encapsulates API call, success/failure emits, and whitelist handling. Clear and testable.
180-208: Expiry handling is correct and observable.Edge cases (missing IP/expiredUnix) are handled, and both human logs and structured events are emitted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/knocker/program.go (2)
47-51: Avoid Fatalf in goroutine; prefer graceful start failure.logger.Fatalf will exit the process from a worker goroutine. Return an error from Start (after doing the health check there) or bail out of run cleanly without terminating the whole service.
Example (minimal) inside run to avoid process exit:
- if err := apiClient.HealthCheck(); err != nil { - logger.Fatalf("Initial health check failed: %v. Please check your API URL and key.", err) - } + if err := apiClient.HealthCheck(); err != nil { + logger.Printf("Initial health check failed: %v. Please check your API URL and key.", err) + return + }Better: move the health check into Start and return the error so the service manager handles it.
68-81: Close channel outside the lock to minimize lock hold time.Closing while holding the lock is safe, but swapping then closing after unlock avoids holding the lock during the close path.
Apply:
- p.mu.Lock() - if p.quit != nil { - close(p.quit) - p.quit = nil - } - p.mu.Unlock() + p.mu.Lock() + quit := p.quit + p.quit = nil + p.mu.Unlock() + if quit != nil { + close(quit) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/knocker/program.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/knocker/program.go (1)
internal/service/service.go (2)
Service(16-32)NewService(34-45)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (5)
cmd/knocker/program.go (5)
15-17: Good state encapsulation and locking primitives.Adding mu (RWMutex) and service pointer is appropriate for coordinating lifecycle access.
22-28: Race on p.quit resolved; launch pattern looks correct.Initializing p.quit under Lock and passing a captured quit into run removes the read/write race noted earlier. Nice.
55-62: Service pointer lifecycle guarded correctly.Assigning and clearing p.service under the mutex is sound; Stop reads it under RLock, which pairs well with this.
64-64: Run(quit) handoff is clean.Passing quit explicitly avoids future accidental unsynchronized reads of p.quit.
53-54: Version parameter is already properly wired—no action needed.The
versionconstant is already defined at the package level incmd/knocker/main.go:17asversion = "dev", and the code at line 53 correctly passes it toNewService. The original concern about missing the version symbol is unfounded; compilation will succeed without any changes.Likely an incorrect or invalid review comment.
This PR adds JSON logs to make integrating knocker-cli with other wrappers easier like a gnome extension.
Summary by CodeRabbit
New Features
Documentation
Tests