[wip]feat(log): implement structured logging module#312
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a robust, structured logging system designed for high-performance and flexibility. By leveraging Zig's comptime capabilities, the system provides a static dispatch pipeline that minimizes runtime overhead while supporting complex logging requirements such as asynchronous file rotation, structured JSON/Logfmt output, and OpenTelemetry integration. The changes include a complete infrastructure for global dispatchers and a bridge to ensure seamless integration with existing Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a structured logging library for the project, featuring a robust pipeline architecture with filters, diagnostic enrichment, and multiple appender types (console, file, and async). The implementation includes support for structured layouts (Text, JSON, Logfmt) and ensures performance through comptime-generic dispatch and small buffer optimization (SBO) for ring buffer messages. My review identified several instances where error handling was insufficient, specifically regarding silent error suppression in formatting and file operations, which violates the project's requirement that all errors must be handled.
| fn formatOtlp(record: *const Record, writer: anytype) void { | ||
| writer.writeAll("{") catch return; | ||
|
|
||
| writer.print("\"timeUnixNano\":{d}", .{@as(i128, record.timestamp_us) * 1_000}) catch return; | ||
|
|
||
| const sev = severityNumber(record.level); | ||
| writer.print(",\"severityNumber\":{d}", .{sev}) catch return; | ||
| writer.writeAll(",\"severityText\":\"") catch return; | ||
| writer.writeAll(severityText(record.level)) catch return; | ||
| writer.writeAll("\"") catch return; | ||
|
|
||
| writer.writeAll(",\"body\":{\"stringValue\":\"") catch return; | ||
| writeOtlpJsonEscaped(writer, record.message); | ||
| writer.writeAll("\"}") catch return; | ||
|
|
||
| writer.writeAll(",\"attributes\":[") catch return; | ||
|
|
||
| writer.writeAll("{\"key\":\"scope\",\"value\":{\"stringValue\":\"") catch return; | ||
| writer.writeAll(record.scope_name) catch return; | ||
| writer.writeAll("\"}}") catch return; | ||
|
|
||
| var iter = record.attrIterator(); | ||
| while (iter.next()) |attr| { | ||
| writer.writeAll(",{\"key\":\"") catch return; | ||
| writer.writeAll(attr.key) catch return; | ||
| writer.writeAll("\",\"value\":") catch return; | ||
| writeOtlpValue(writer, attr.value); | ||
| writer.writeAll("}") catch return; | ||
| } | ||
|
|
||
| writer.writeAll("]") catch return; | ||
| writer.writeAll("}\n") catch return; | ||
| } |
There was a problem hiding this comment.
Throughout formatOtlp, write operations use catch return, which silently ignores errors. This can lead to incomplete or malformed OTLP JSON log entries being sent to the collector, which might reject them or parse them incorrectly. This violates the style guide's rule 160: "All errors must be handled."
Consider changing formatOtlp to return an error (!void) and using try for all write operations. The error can then be handled in the append function, for example by calling reportAppendError.
References
- Rule 160 states that all errors must be handled. The current code uses
catch returnwhich silently ignores potential I/O errors during formatting, leading to potentially corrupt data. (link)
| pub fn format(self: *const TextLayout, record: *const Record, writer: anytype) void { | ||
| formatTimestamp(writer, record.timestamp_us); | ||
| writer.writeByte(' ') catch return; | ||
|
|
||
| if (self.color) { | ||
| writer.writeAll(levelColor(record.level)) catch return; | ||
| writer.writeAll(rec.asText(record.level)) catch return; | ||
| writer.writeAll(ansi_reset) catch return; | ||
| } else { | ||
| writer.writeAll(rec.asText(record.level)) catch return; | ||
| } | ||
| writer.writeByte(' ') catch return; | ||
|
|
||
| if (!std.mem.eql(u8, record.scope_name, "default")) { | ||
| writer.writeByte('(') catch return; | ||
| writer.writeAll(record.scope_name) catch return; | ||
| writer.writeAll("): ") catch return; | ||
| } else { | ||
| writer.writeAll(": ") catch return; | ||
| } | ||
|
|
||
| writer.writeAll(record.message) catch return; | ||
|
|
||
| var iter = record.attrIterator(); | ||
| while (iter.next()) |attr| { | ||
| writer.writeByte(' ') catch return; | ||
| writer.writeAll(attr.key) catch return; | ||
| writer.writeByte('=') catch return; | ||
| attr.formatValue(writer); | ||
| } | ||
|
|
||
| writer.writeByte('\n') catch return; | ||
| } |
There was a problem hiding this comment.
Throughout the layout formatters (TextLayout, JsonLayout, LogfmtLayout), write operations use catch return. This silently ignores I/O errors and can result in partially written, corrupted log lines. This violates the style guide's rule 160: "All errors must be handled."
A better approach would be for format functions to return an error (!void) and use try for all write operations. The caller (the appender) could then handle the error appropriately, for instance by calling reportAppendError. This ensures that formatting failures are not silent.
References
- Rule 160 states that all errors must be handled. The current code uses
catch returnwhich silently ignores potential I/O errors during formatting, leading to potentially corrupt log lines. (link)
| std.fs.cwd().deleteFile("smoke.log") catch {}; | ||
| std.fs.cwd().deleteFile("smoke_combined.log") catch {}; |
There was a problem hiding this comment.
The errors from deleteFile are being ignored with catch {}. While this is an example file, the style guide requires all errors to be handled (Rule 160). Silently ignoring file system errors can hide underlying problems. It would be better to at least print the error to stderr.
std.fs.cwd().deleteFile("smoke.log") catch |err| std.debug.print("Failed to delete smoke.log: {s}\n", .{@errorName(err)});
std.fs.cwd().deleteFile("smoke_combined.log") catch |err| std.debug.print("Failed to delete smoke_combined.log: {s}\n", .{@errorName(err)});
References
- Rule 160 states that all errors must be handled. The current code uses
catch {}which silently ignores potential file system errors during cleanup. (link)
| const written = fbs.getWritten(); | ||
|
|
||
| if (written.len < format_buf_size) { | ||
| self.rolling_writer.writer().writeAll(written) catch return; |
There was a problem hiding this comment.
Errors from rolling_writer.writer().writeAll are being silently ignored with catch return. This violates the style guide's rule 160: "All errors must be handled." These errors should be reported to stderr using the reportAppendError utility function, similar to how other appenders handle write errors.
self.rolling_writer.writer().writeAll(written) catch |err| reportAppendError(err, "FileAppend.writeAll");
References
- Rule 160 states that all errors must be handled. The current code uses
catch returnwhich silently ignores potential I/O errors. (link)
| var list = std.ArrayList(u8).init(self.allocator); | ||
| defer list.deinit(); | ||
| self.layout.format(record, list.writer()); | ||
| self.rolling_writer.writer().writeAll(list.items) catch return; |
There was a problem hiding this comment.
Errors from rolling_writer.writer().writeAll are being silently ignored with catch return. This violates the style guide's rule 160: "All errors must be handled." These errors should be reported to stderr using the reportAppendError utility function, similar to how other appenders handle write errors.
self.rolling_writer.writer().writeAll(list.items) catch |err| reportAppendError(err, "FileAppend.writeAll");
References
- Rule 160 states that all errors must be handled. The current code uses
catch returnwhich silently ignores potential I/O errors. (link)
| } | ||
|
|
||
| pub fn addOverride(self: *ScopeFilter, scope_name: []const u8, min_level: std.log.Level) void { | ||
| self.overrides.append(.{ .scope_name = scope_name, .min_level = min_level }) catch {}; |
There was a problem hiding this comment.
The error from self.overrides.append is silently ignored with catch {}. If the number of overrides exceeds max_scopes (32), append will fail and the override will not be added. This failure is silent, which can be very confusing for a user who expects their configuration to be applied. According to the style guide (Rule 160), all errors must be handled. It would be better to log a warning if the limit is reached.
self.overrides.append(.{ .scope_name = scope_name, .min_level = min_level }) catch |_| {
std.debug.print("log.ScopeFilter: could not add override for '{s}', max_scopes ({d}) reached.\n", .{
scope_name, max_scopes,
});
};
References
- Rule 160 states that all errors must be handled. The current code uses
catch {}which silently ignores the error when the maximum number of scope overrides is exceeded. (link)
| pub fn parseLevel(text: []const u8) ?Level { | ||
| var buf: [8]u8 = undefined; | ||
| if (text.len == 0 or text.len >= buf.len) return null; | ||
| for (text, 0..) |c, i| buf[i] = std.ascii.toLower(c); | ||
| const lower = buf[0..text.len]; | ||
|
|
||
| if (std.mem.eql(u8, lower, "err")) return .err; | ||
| if (std.mem.eql(u8, lower, "error")) return .err; | ||
| if (std.mem.eql(u8, lower, "warn")) return .warn; | ||
| if (std.mem.eql(u8, lower, "warning")) return .warn; | ||
| if (std.mem.eql(u8, lower, "info")) return .info; | ||
| if (std.mem.eql(u8, lower, "debug")) return .debug; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The fixed-size buffer buf: [8]u8 in parseLevel is brittle. While it's large enough for the current standard log levels, it makes the function less robust to future changes, such as the introduction of custom or longer log level names. A more robust approach would be to use std.ascii.eqlIgnoreCase to perform the comparison without creating a lowercased copy.
pub fn parseLevel(text: []const u8) ?Level {
if (std.ascii.eqlIgnoreCase(text, "err")) return .err;
if (std.ascii.eqlIgnoreCase(text, "error")) return .err;
if (std.ascii.eqlIgnoreCase(text, "warn")) return .warn;
if (std.ascii.eqlIgnoreCase(text, "warning")) return .warn;
if (std.ascii.eqlIgnoreCase(text, "info")) return .info;
if (std.ascii.eqlIgnoreCase(text, "debug")) return .debug;
return null;
}
2f474a2 to
416b6fa
Compare
416b6fa to
266f087
Compare
Replace comptime-generic Dispatch/Dispatcher with runtime-configurable structs using BoundedArray, matching logforth's caller-owned pipeline design. - Add AnyFilter, AnyDiagnostic, AnyAppend type-erased interfaces - Add .any() to all concrete filter/diagnostic/appender types - Rewrite Dispatch and Dispatcher as concrete runtime structs - Remove DefaultSync dev default; global dispatcher starts as noop - Callers must explicitly build and set their pipeline - Simplify LevelFilter/parseLevel/FilterResult tests to table-driven
Summary
Add a full-featured structured logging module for lodestar-z
Key features
RUST_LOG-style directives likewarn,fork_choice=debug)std_optionsso existingstd.log.scoped()calls route through the pipelineinitConsoleDispatcher,initFileDispatcher,initCombinedDispatcherfor common setups2024-08-11T22:44:57.172105Z)ring_buffer.zig: Generic lock-free SPSC ring buffer, reusable outside loggingArchitecture
Module structure
Consolidated from 20+ separate files into 6 focused modules:
record.zig— Scope, Level, Attr, Record, comptime interface checksfilter.zig— LevelFilter, ScopeFilter, EnvFilterdiagnostic.zig— StaticDiagnostic, ThreadLocalDiagnosticlayout.zig— TextLayout, JsonLayout, LogfmtLayout, ISO 8601 formattingappend.zig— WriterAppend, AsyncAppend, RollingFileWriter, OpenTelemetryAppenddispatch.zig— Dispatch, Dispatcher, AnyDispatcher, Logger