feat(kernel): forward logs through shared logger - #450
Open
vuanhphung wants to merge 12 commits into
Open
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
vuanhphung
force-pushed
the
vu-phung/kernel-log-file-parity
branch
from
August 21, 2026 18:20
a6fd333 to
e056e93
Compare
Address PR review of the log-forwarding path: - Drop the cgo.Handle->void* coercion; register with NULL user_data and reach the sink via a package global, so no Go pointer is fabricated into a C pointer the GC can fault on. - Forward off the kernel thread: the C callback copies the strings and does a non-blocking enqueue onto a bounded channel drained by one goroutine, with a drop counter and per-record panic recovery. - Use an exact-signature C adapter instead of a function-pointer cast. - Rework logger.sharedOutput into an atomic, LevelWriter-aware proxy: no lock held across the user Write (no self-deadlock, a stuck writer stays replaceable), nil normalized to io.Discard, and WriteLevel preserved. Adds enqueue/drop, drain-panic, nil-output, and LevelWriter tests. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 2 Low
Looks good — solid engineering with strong unit + end-to-end coverage (level mapping, retargeting, panic containment, drop policy). Two Low observations: an error-path close(ch) whose "no producer exists" comment is stronger than the kernel ABI guarantees (contained by the trampoline's recover), and silently-dropped kernel logs whose drop counter is never surfaced in production.
…og bridge Follow-up to review feedback on the async log-forwarding path: - Stamp each kernel record with the emission time captured in the cgo callback instead of the drain time. The sink is now a hook-free zerolog logger built over the shared output proxy (new logger.Output()), so a backed-up drain no longer skews kernel log timestamps. - Add flushKernelLogs(timeout): a FIFO barrier that drains the async hand-off before the writer is closed, retargeted, or the process exits. The end-to-end test flushes instead of polling. - installKernelLogCallback now starts the drain only after a successful install and drops the channel (no close) on failure, so there is no send-on-closed race to reason about on the error path. - Surface dropped kernel records with a one-shot warning from the drain goroutine (never the kernel thread); the running total stays available via kernelLogDropped(). - Document the SetLogOutput concurrent-retarget constraint and add concurrent-retarget, nil-discard, LevelWriter, and flush regression tests. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
…-wrap footgun Follow-up review fixes: - HIGH: the one-shot drop warning wrote through the shared logger outside the drain's per-record recover, so a panicking writer plus a queue overflow could crash the process from the drain goroutine. Both the forwarded record and the drop warning now run inside a shared contain() recover. Adds a regression test that combines a panicking writer with dropped records. - MEDIUM: replace logger.Output() (which returned the live proxy, so SetLogOutput(Output()) — e.g. a save/restore of the current output — wrapped the proxy around itself and deadlocked the next write) with logger.NewForwardingLogger(), which returns a hook-free zerolog.Logger over the shared output and cannot be round-tripped back into SetLogOutput. - Clarify the flushKernelLogs doc: it drains the async hand-off for clean shutdown/retarget, but has no public entry point yet — only the end-to-end test calls it; a supported flush API is a separate change. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — solid, well-tested change. The shared sharedOutput proxy correctly lets derived logger snapshots (and the kernel bridge) follow SetLogOutput via an atomic store, preserves LevelWriter routing, normalizes nil→io.Discard, and the cgo callback path (panic containment, non-blocking bounded hand-off, owned string copies, FIFO flush barrier) is sound and thoroughly covered. One low-severity note on the OFF+sync.Once latching behavior being under-documented; no correctness defects found.
The previous change returned a zerolog.Logger from NewForwardingLogger, but zerolog.Logger implements io.Writer (value-receiver Write), so SetLogOutput(NewForwardingLogger()) still compiled and deadlocked: the forwarding logger's Write routes back through the shared output into the same SyncWriter mutex. Re-entrant custom writers hit the same trap. Replace it with logger.ForwardingSink, a method-only type (Event(level) *zerolog.Event) that is deliberately not an io.Writer, so it cannot be passed to SetLogOutput to form a self-referential, deadlocking sink. The kernel log sink now holds a *ForwardingSink and keeps its level mapping. Adds a regression test pinning that ForwardingSink is not an io.Writer. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Per-destination serialization holds a non-reentrant lock across the user Write/WriteLevel, so a destination that logs through the driver from within its own write (a self-referential sink) deadlocks — and would recurse without bound even if it did not. This is inherent to any logging library; document the constraint on SetLogOutput rather than dropping serialization (which would only re-open the concurrent-corruption case and turn the deadlock into a stack overflow). Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
…ff logger.Logger Two review fixes: - Move the pure-Go async forwarding pipeline (record type, bounded queue, drain, flush barrier, drop accounting, panic containment) out of the cgo-tagged log_callback.go into the untagged logforward_async.go, and its tests into logforward_async_test.go. This matches the repo convention documented in logging_level.go: the FIFO-flush, drop-policy, and panic-containment tests now run in the default CGO_ENABLED=0 lane instead of only the kernel-linked lane. log_callback.go keeps only the cgo trampoline and the one-time install. - Route the drain's one-shot drop warning through the sink's own immutable logger (new logSink.warnDropped) instead of logger.Logger, whose embedded value SetLogLevel reassigns. The long-lived drain goroutine no longer reads a field that races SetLogLevel; drainKernelLogs is now pure Go. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #444. Depends on databricks/databricks-sql-kernel#278.
Routes Rust kernel tracing records through the driver's existing zerolog sink, so
logger.SetLogOutputapplies equally to Thrift, Go kernel-binding, and Rust kernel logs. A synchronized writer proxy lets existing logger snapshots follow later output changes safely.Adds callback, level-mapping, retargeting, panic-containment, and end-to-end same-file coverage. Validated with the full default and kernel-tagged Go suites, focused race tests,
go vet, and golangci-lint.This PR was created with GitHub MCP.