Logging overhaul

[spuun]

WHAT is this pull request doing?

An overhaul of our logging code to enable more logging formats and make it configurable.

Depends somewhat on crystal-lang/crystal#16098, but this has been monkey patched for now.

Logging

Before this PR we used Log mixed with @log (instance of Logger). This PR will add macros so L.<level> (alias Logging) always is used. If it's used in the context of an instance including Loggable (e.g. Queue), the metadata for the instance will be added to the log.

To set an instance's metadata L.context(meta: "data", foo: "bar") is used, preferrably in the constructor.

The macros will remove the need of using block and metadata is just key-value args to the method

L.info "my message", meta: "data", foo: "bar"

Exceptions are passed as a key value arg:

begin
# …
rescue ex
L.error "Failure", exception: ex, other: "data"
end

Formats

This PR introduces three different logging formats:

• stdout (what's before this PR)
• json
• logfmt

No matter which format that's used the logging will respect the environment variable JOURNAL_STREAM. If it's set timestamp will be omitted (except for json) and the log level will be logged as journal priority.

Configuration

Three new configuration options are added:

• log_format = { stdout, json, logfmt }
• log_single_line = { true , false }
• log_backtrace_separator = string

log_backtrace_separator is only used if log_single_line is true.

HOW can this pull request be tested?

No specs, run lavin and look at output...

[claude]

Claude finished @spuun's task —— View job

---

Code Review for Logging Overhaul

I've conducted a comprehensive review of this significant logging overhaul PR. Here's my detailed feedback:

✅ Strengths & Good Design Decisions

Architecture

• Clean abstraction: The Loggable module pattern with L.context() provides excellent context management
• Unified API: Replacing mixed Log/@log usage with consistent L.<level> macros is a major improvement
• Format flexibility: Supporting JSON, logfmt, and stdout formats covers diverse deployment needs
• Systemd integration: Proper JOURNAL_STREAM handling with priority mapping shows production awareness

Implementation Quality

• Memory efficiency: The monkey-patched Log::Metadata optimizations look well-designed for the upstream PR
• Configuration consistency: Clean integration into existing config system with sensible defaults
• Context inheritance: The metadata extension pattern allows proper hierarchical logging context

🔍 Issues & Concerns

Critical Issues

1. Memory Management in LogfmtFormat (src/lavinmq/logging/format.cr:148)

@io << ' ' << before_backtrace << '"' << ex.backtrace.join(@io, Config.instance.log_backtrace_separator) << '"'

• join(@io, separator) appears to be incorrect usage - join typically returns a string
• Should likely be: ex.backtrace.join(Config.instance.log_backtrace_separator)
• This could cause runtime errors in exception logging

2. Commented Dead Code (src/lavinmq/logging/format.cr:145)

# @io << before << '"' << ex.class.name << ": " << ex.message << '"'

• Commented code suggests incomplete exception message formatting in logfmt
• Exception class and message aren't being logged, reducing debugging utility

3. Monkey Patching Risk (src/lavinmq/logging.cr:5-79)

• Heavy reliance on monkey-patching Log::Metadata creates fragile dependency on Crystal internals
• The @overridden_size hack may break with Crystal updates
• Consider feature flagging or fallback mechanisms

Performance Concerns

4. Macro Overhead (src/lavinmq/logging.cr:115-137)

• The logging macros generate significant code at each call site
• Block-based approach may have allocation overhead compared to direct calls
• Consider benchmarking against the previous implementation

5. Metadata Building (src/lavinmq/logging.cr:131-134)

• @log_context.extend(metadata) creates new metadata objects on each call
• High-frequency logging could cause GC pressure
• Previous @log instance approach may have been more efficient

Code Quality Issues

6. Magic Number Usage (src/lavinmq/logging/format.cr:150-152)

backtrace_id = Time.monotonic.nanoseconds
backtrace_id.to_s(@io, 32)

• Base-32 encoding without explanation
• Consider using a more readable identifier generation approach

7. Inconsistent Error Handling

• Some old Log. calls remain in codebase (found in vhost_store.cr, mqtt/session.cr)
• Migration appears incomplete, creating inconsistent logging patterns

Configuration & Security

8. Missing Validation (src/lavinmq/config.cr)

• No validation for log_backtrace_separator content (could contain injection chars)
• Format parsing should handle invalid enum values gracefully

📊 Test Coverage Assessment

Critical Gap: The PR description explicitly states "No specs" for this massive change. For a core infrastructure component like logging:

• Risk: Silent failures in production environments
• Recommendation: Add at least basic format output tests and error handling specs
• Suggestion: Test each format with various metadata combinations and exception scenarios

🚀 Performance Recommendations

1. Benchmark the macro approach against direct method calls
2. Profile metadata allocation in high-throughput scenarios
3. Consider lazy evaluation for expensive formatting operations
4. Cache format instances rather than recreating per log entry

🔒 Security Considerations

• Configurable log formats could potentially be exploited for log injection
• Backtrace separator should be sanitized to prevent control character injection
• JSON format appears safe from injection attacks

🎯 Migration Completeness

The migration appears incomplete - found remaining Log. usage in several files. A complete migration would:

1. Use tooling to find all Log. references
2. Update remaining instances to use L. pattern
3. Ensure consistent context setup across all components

Overall Assessment

This is an ambitious and well-architected overhaul that addresses real needs for structured logging. However, it has several critical implementation issues and lacks adequate testing. The monkey-patching approach, while clever, introduces significant maintenance risk.

Recommendation: Address the critical issues (especially the join bug) and add basic test coverage before merging. Consider a phased rollout approach given the scope of changes.

---