Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if sink != nil { | ||
| sink(line) |
There was a problem hiding this comment.
🟡 Split structured logs reach sink malformed
When structured output spans multiple Write calls, sink receives only the fragment ending in }. The complete handler record is lost.
Learn more
A command's stdout and stderr are streams. One logical line can be split across multiple Write calls, especially when a record exceeds the copy buffer. This logger classifies each incoming chunk immediately and does not retain text before the last newline. A split JSON record therefore sends its first fragment through the ordinary error logger and passes only its final fragment to sink.
Example: A record {"level":"info","msg":"..."} arrives as {"level":"info", and "msg":"..."}\n. The first write is logged as an error. The sink receives only "msg":"..."}, which is invalid JSON instead of the expected complete record.
Recommended fix: Buffer incomplete text across Write calls and classify only newline-terminated records. Protect the buffer if the writer can receive concurrent calls, and define how any final unterminated record is flushed.
Was this helpful? React with 👍 or 👎 to provide feedback.
… lines A deployment that ships ingress's logs somewhere has nowhere to install a tee: InitLogger builds the zap logger itself, and its variadic slot is already spoken for by the logger values. LoggerTee carries one in on the config instead. The zero value builds no core, so existing callers are unaffected. The handler subprocess is the other half. Its own structured lines are passed straight through to the parent's stdout rather than its zap core, so a tee on the parent never sees them. NewHandlerLoggerWithSink hands them to a caller-supplied sink instead, which keeps the decision about where they go with the parent that owns the transport. Requires protocol at or past logger.WithTee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
17a9315 to
de7a640
Compare
3 participants
A deployment that ships ingress's logs somewhere has nowhere to install a tee:
InitLoggerbuilds the zap logger itself, and its variadic slot is already spoken for by the logger values.LoggerTeecarries one in on the config instead. The zero value builds no core, so existing callers are unaffected.The handler subprocess is the other half. Its own structured lines are passed straight through to the parent's stdout rather than its zap core, so a tee on the parent never sees them.
NewHandlerLoggerWithSinkhands them to a caller-supplied sink instead, which keeps the decision about where they go with the parent that owns the transport.Requires protocol at or past
logger.WithTee, hence the bump.Consumed by livekit/cloud-ingress, which uses both halves to publish its logs to the observability warehouse.