-
Notifications
You must be signed in to change notification settings - Fork 495
Give the four log-entry structs a shared LogEntry interface #52107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e35a373
a15204d
8053a6e
f22320f
fa8a121
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # ADR-52107: LogEntry Interface for Heterogeneous Log-Entry Structs | ||
|
|
||
| **Date**: 2026-08-11 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| `pkg/cli` contains four independently-defined structs — `AccessLogEntry`, `FirewallLogEntry`, `AuditLogEntry`, and `GatewayLogEntry` — each modelling a parsed log line from a different source. They share no common type, so any code that wants to handle "a log entry" generically (formatters, filters, report generators) must special-case every concrete type. The structs also have structurally incompatible fields: `Timestamp` is a `string` in three types but a `float64` in `AuditLogEntry`, and only `GatewayLogEntry` carries a `Level` field. These differences make a shared embedded base struct impractical without changing wire formats. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will define a `LogEntry` interface in `pkg/cli/log_entry.go` with four accessor methods — `EntryTimestamp()`, `EntrySource()`, `EntryLevel()`, and `EntryMessage()` — and implement it on all four existing log-entry types using value receivers. Timestamp normalisation (epoch seconds → RFC3339 UTC) is handled inside the implementations so callers see a uniform string regardless of the underlying field type. Compile-time conformance is enforced with blank-identifier assertions (`var _ LogEntry = AccessLogEntry{}`). A `FormatLogEntry` function serves as the first generic consumer. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Embedded Base Struct | ||
|
|
||
| Define a shared `BaseLogEntry` struct and embed it in the four types. This would promote common fields directly and avoid the interface layer. | ||
|
|
||
| Rejected because the four types have incompatible field layouts: `AuditLogEntry.Timestamp` is `float64` while the others are `string`, and only `GatewayLogEntry` has `Level`. Adding these fields to a base struct would require changing the JSON tags or adding duplicate fields, breaking existing serialisation and parse call sites. | ||
|
|
||
| #### Alternative 2: Type Switch / Ad-Hoc Polymorphism | ||
|
|
||
| Continue the current pattern: any code that needs to act on "any log entry" performs an explicit type switch over all four concrete types. | ||
|
|
||
| Rejected because it duplicates the dispatch logic in every consumer, makes adding a fifth log-entry type a multi-site change, and provides no compile-time guarantee that all types are handled. The very motivation of the linked issue (#52091) was to eliminate this duplication. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Formatting, filtering, and reporting code can operate uniformly over any `[]LogEntry` without knowing the underlying type. | ||
| - Adding a fifth log-entry type in the future requires only implementing four methods, with no changes to existing consumers. | ||
| - Compile-time `var _ LogEntry = ...` assertions catch interface drift immediately at build time. | ||
| - No existing struct fields, JSON tags, or parsers are touched, so serialisation and all current call sites are unaffected. | ||
|
|
||
| #### Negative | ||
| - Epoch-to-RFC3339 timestamp normalisation is now encapsulated inside each implementation, making the per-source conversion logic less visible to callers who might expect raw values. | ||
| - All four implementations use value receivers; callers passing large `[]LogEntry` slices by value incur copying overhead that would not exist with pointer receivers or a concrete slice type. | ||
|
|
||
| #### Neutral | ||
| - The `LogEntry` interface is defined in the same `cli` package as the concrete types, so there is no cross-package dependency change. | ||
| - Test coverage is added in `pkg/cli/log_entry_test.go` using table-driven tests; the interface itself is not exported beyond the `cli` package boundary at this point. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
| ) | ||
|
|
||
| // LogEntrySource identifies the log stream a LogEntry was parsed from. | ||
| type LogEntrySource string | ||
|
|
||
| const ( | ||
| // LogSourceAccess identifies squid access log entries. | ||
| LogSourceAccess LogEntrySource = "access" | ||
| // LogSourceFirewall identifies firewall log entries. | ||
| LogSourceFirewall LogEntrySource = "firewall" | ||
| // LogSourceAudit identifies audit.jsonl entries. | ||
| LogSourceAudit LogEntrySource = "audit" | ||
| // LogSourceGateway identifies MCP gateway.jsonl entries. | ||
| LogSourceGateway LogEntrySource = "gateway" | ||
| ) | ||
|
|
||
| // Shared severity levels reported by LogEntry.EntryLevel. | ||
| const ( | ||
| LogLevelInfo = "info" | ||
| LogLevelError = "error" | ||
| ) | ||
|
|
||
| // LogEntry is the shared shape implemented by every parsed log-line type | ||
| // (AccessLogEntry, FirewallLogEntry, AuditLogEntry and GatewayLogEntry). | ||
| // It lets formatting, filtering and reporting code operate on any log entry | ||
| // without special-casing each concrete type. | ||
| type LogEntry interface { | ||
| // EntryTimestamp returns the entry timestamp, normalized to RFC3339 (UTC) | ||
| // for sources that record epoch timestamps. Unparseable timestamps are | ||
| // returned unchanged. | ||
| EntryTimestamp() string | ||
| // EntrySource returns the log stream the entry was parsed from. | ||
| EntrySource() LogEntrySource | ||
| // EntryLevel returns the entry severity: LogLevelInfo or LogLevelError. | ||
| EntryLevel() string | ||
| // EntryMessage returns a short human-readable description of the entry. | ||
| EntryMessage() string | ||
| } | ||
|
|
||
| // Compile-time checks that all four log-entry types share the LogEntry shape. | ||
| var ( | ||
| _ LogEntry = AccessLogEntry{} | ||
| _ LogEntry = FirewallLogEntry{} | ||
| _ LogEntry = AuditLogEntry{} | ||
| _ LogEntry = GatewayLogEntry{} | ||
| ) | ||
|
|
||
| // FormatLogEntry renders any log entry in a uniform "timestamp [source] level: message" form. | ||
| func FormatLogEntry(entry LogEntry) string { | ||
| return fmt.Sprintf("%s [%s] %s: %s", entry.EntryTimestamp(), entry.EntrySource(), entry.EntryLevel(), entry.EntryMessage()) | ||
| } | ||
|
|
||
| // formatEpochSeconds converts fractional epoch seconds to RFC3339 in UTC. | ||
| func formatEpochSeconds(seconds float64) string { | ||
| return time.Unix(0, int64(seconds*float64(time.Second))).UTC().Format(time.RFC3339) | ||
| } | ||
|
|
||
| // formatEpochTimestamp converts a textual epoch-seconds timestamp to RFC3339 in UTC, | ||
| // returning the input unchanged when it is not a numeric epoch value. | ||
| func formatEpochTimestamp(timestamp string) string { | ||
| seconds, err := strconv.ParseFloat(strings.TrimSpace(timestamp), 64) | ||
| if err != nil { | ||
| return timestamp | ||
| } | ||
| return formatEpochSeconds(seconds) | ||
| } | ||
|
|
||
| // levelFromAllowed maps an allow/deny outcome to a shared severity level. | ||
| func levelFromAllowed(allowed bool) string { | ||
| if allowed { | ||
| return LogLevelInfo | ||
| } | ||
| return LogLevelError | ||
| } | ||
|
|
||
| // joinEntryMessage builds a message from the first non-placeholder parts available. | ||
| func joinEntryMessage(parts ...string) string { | ||
| kept := make([]string, 0, len(parts)) | ||
| for _, part := range parts { | ||
| part = strings.TrimSpace(part) | ||
| if part == "" || part == "-" { | ||
| continue | ||
| } | ||
| kept = append(kept, part) | ||
| } | ||
| return strings.Join(kept, " ") | ||
| } | ||
|
|
||
| // EntryTimestamp implements LogEntry. | ||
| func (a AccessLogEntry) EntryTimestamp() string { return formatEpochTimestamp(a.Timestamp) } | ||
|
|
||
| // EntrySource implements LogEntry. | ||
| func (a AccessLogEntry) EntrySource() LogEntrySource { return LogSourceAccess } | ||
|
|
||
| // EntryLevel implements LogEntry. | ||
| func (a AccessLogEntry) EntryLevel() string { return levelFromAllowed(isAllowedSquidStatus(a.Status)) } | ||
|
|
||
| // EntryMessage implements LogEntry. | ||
| func (a AccessLogEntry) EntryMessage() string { return joinEntryMessage(a.Method, a.URL, a.Status) } | ||
|
|
||
| // EntryTimestamp implements LogEntry. | ||
| func (f FirewallLogEntry) EntryTimestamp() string { return formatEpochTimestamp(f.Timestamp) } | ||
|
|
||
| // EntrySource implements LogEntry. | ||
| func (f FirewallLogEntry) EntrySource() LogEntrySource { return LogSourceFirewall } | ||
|
|
||
| // EntryLevel implements LogEntry. | ||
| func (f FirewallLogEntry) EntryLevel() string { | ||
| return levelFromAllowed(isRequestAllowed(f.Decision, f.Status)) | ||
| } | ||
|
|
||
| // EntryMessage implements LogEntry. | ||
| func (f FirewallLogEntry) EntryMessage() string { | ||
| target := f.URL | ||
| if strings.TrimSpace(target) == "" || target == "-" { | ||
| target = f.Domain | ||
| } | ||
| return joinEntryMessage(f.Method, target, f.Decision) | ||
| } | ||
|
|
||
| // EntryTimestamp implements LogEntry. | ||
| func (a AuditLogEntry) EntryTimestamp() string { return formatEpochSeconds(a.Timestamp) } | ||
|
|
||
| // EntrySource implements LogEntry. | ||
| func (a AuditLogEntry) EntrySource() LogEntrySource { return LogSourceAudit } | ||
|
|
||
| // EntryLevel implements LogEntry. | ||
| func (a AuditLogEntry) EntryLevel() string { return levelFromAllowed(isEntryAllowed(a)) } | ||
|
|
||
| // EntryMessage implements LogEntry. | ||
| func (a AuditLogEntry) EntryMessage() string { | ||
| target := a.URL | ||
| if strings.TrimSpace(target) == "" || target == "-" { | ||
| target = a.Host | ||
| } | ||
| return joinEntryMessage(a.Method, target, a.Decision) | ||
| } | ||
|
|
||
| // EntryTimestamp implements LogEntry. | ||
| func (g GatewayLogEntry) EntryTimestamp() string { return g.Timestamp } | ||
|
|
||
| // EntrySource implements LogEntry. | ||
| func (g GatewayLogEntry) EntrySource() LogEntrySource { return LogSourceGateway } | ||
|
|
||
| // EntryLevel implements LogEntry. It mirrors the error classification used | ||
| // by gateway log metrics processing (see processGatewayLogEntry), treating | ||
| // Status == "error", a non-empty Error, or Level == "error" as failure | ||
| // signals, and normalizes the result to the interface's two documented | ||
| // levels (LogLevelInfo / LogLevelError). | ||
| func (g GatewayLogEntry) EntryLevel() string { | ||
| return levelFromAllowed(g.Status != "error" && g.Error == "" && g.Level != "error") | ||
| } | ||
|
|
||
| // EntryMessage implements LogEntry. | ||
| func (g GatewayLogEntry) EntryMessage() string { | ||
| for _, candidate := range []string{g.Message, g.Error, g.Event, g.Type} { | ||
| if strings.TrimSpace(candidate) != "" { | ||
| return candidate | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L1-168: yagni: entire file (LogEntry interface, 4 impls, FormatLogEntry, constants) has zero call sites anywhere in the repo. Delete until a real consumer needs generic log-entry handling.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] 💡 Suggested test{
name: "gateway log entry with all-blank fields returns empty message",
entry: GatewayLogEntry{Timestamp: "2024-01-12T10:00:00Z", Level: LogLevelInfo},
expectedMessage: "",
},Consider whether an empty message should be surfaced as a structured warning. @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a test case for all-blank message fields in f22320f. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential logic issue:
Status != LogLevelErrorconflates two separate domainsWhen
g.Levelis empty, the fallback is:LogLevelErroris"error"andStatususes the same string for a status outcome ("success","error","unknown"). AStatusof"unknown"with noErrorstring returnsLogLevelInfo, which is likely incorrect — unknown status should not be treated as a success.Consider being explicit:
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f22320f:
EntryLevelnow returns error wheneverStatus == "error",Erroris non-empty, orLevel == "error", matchingprocessGatewayLogEntry's OR-based classification. This also removes the deadStatus != LogLevelErrorcomparison.