Skip to content

Commit ca9234b

Browse files
fix(kernel): harden log-bridge init + address review feedback
Review feedback from PR #824 (gopalldb): - P1: guard the init_logging() call against throwing, not just a missing function. A panic across the PyO3 boundary surfaces as pyo3_runtime.PanicException (a BaseException, not Exception), so a bare call could escape module import and fail every use_kernel=True connection over a non-essential logging feature. Wrap in try/except BaseException, log a debug breadcrumb, continue. This also neutralizes the idempotency concern regardless of the Rust impl. - P2: soften the level-control test docstring to make clear it asserts the effective customer-visible outcome (sub-threshold records don't surface), not source-side suppression — caplog filters after the FFI. - P2: downgrade the databricks.sql.kernel.pyo3 assertion to a soft warning so a benign kernel change to the boundary breadcrumb target doesn't break the connector e2e suite. The core databricks.sql.kernel contract is still hard-asserted. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
1 parent b47e3b5 commit ca9234b

2 files changed

Lines changed: 46 additions & 12 deletions

File tree

src/databricks/sql/backend/kernel/_errors.py

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@
3838

3939
from __future__ import annotations
4040

41+
import logging
42+
4143
from databricks.sql.exc import (
4244
DatabaseError,
4345
Error,
@@ -61,12 +63,26 @@
6163
# the extension loads. The kernel emits under the ``databricks.sql.kernel``
6264
# logger (a child of the connector's ``databricks.sql`` namespace), so a
6365
# customer who configures ``databricks.sql`` logging gets kernel logs for
64-
# free with no extra setup. ``init_logging`` is idempotent on the Rust
65-
# side; ``getattr`` guards against an older kernel wheel that predates the
66-
# function so ``use_kernel=True`` still works (just without kernel logs).
66+
# free with no extra setup.
67+
#
68+
# This is a best-effort, non-essential feature: it must never take down
69+
# ``use_kernel=True`` for a process. ``getattr`` guards against an older
70+
# kernel wheel that predates the function. The ``try`` guards against the
71+
# call itself throwing — note ``except BaseException`` is deliberate: a
72+
# panic raised across the PyO3 boundary surfaces as
73+
# ``pyo3_runtime.PanicException``, which derives from ``BaseException``
74+
# (not ``Exception``), so a narrower clause would let it escape module
75+
# import and fail every kernel-backed connection. The kernel side is
76+
# idempotent and returns rather than panics on a double install, but we
77+
# do not rely on that here — the guard holds regardless of the Rust impl.
6778
_kernel_init_logging = getattr(_kernel, "init_logging", None)
6879
if _kernel_init_logging is not None:
69-
_kernel_init_logging()
80+
try:
81+
_kernel_init_logging()
82+
except BaseException as exc: # noqa: BLE001 - see comment above re: PanicException
83+
logging.getLogger(__name__).debug(
84+
"kernel log bridge init failed; continuing without it: %r", exc
85+
)
7086

7187

7288
# Map a kernel `code` slug to the PEP 249 exception class that best

tests/e2e/test_kernel_backend.py

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -160,20 +160,38 @@ def test_kernel_logs_reach_python_logging(kernel_conn_params, caplog):
160160
"expected log records under the 'databricks.sql.kernel' logger; "
161161
"the kernel tracing -> Python logging bridge did not deliver any"
162162
)
163-
# The core kernel logger (not just the .pyo3 child) must be present:
163+
# The core kernel logger (not just any child) must be present — this
164+
# is the customer-facing contract.
164165
assert any(
165166
r.name == "databricks.sql.kernel" for r in kernel_records
166167
), "expected core kernel records on the exact 'databricks.sql.kernel' logger"
167-
# The pyo3 boundary breadcrumb must surface under the .pyo3 child:
168-
assert any(
169-
r.name == "databricks.sql.kernel.pyo3" for r in kernel_records
170-
), "expected pyo3-boundary records on 'databricks.sql.kernel.pyo3'"
168+
# The pyo3-boundary breadcrumb (`databricks.sql.kernel.pyo3`) is a
169+
# nice-to-have, but the exact sub-target is a kernel-internal naming
170+
# detail — assert softly so a benign kernel change to the boundary
171+
# breadcrumbs doesn't break the connector e2e suite.
172+
if not any(r.name == "databricks.sql.kernel.pyo3" for r in kernel_records):
173+
import warnings
174+
175+
warnings.warn(
176+
"no 'databricks.sql.kernel.pyo3' boundary records seen; "
177+
"the kernel may have changed its pyo3 breadcrumb target",
178+
stacklevel=2,
179+
)
171180

172181

173182
def test_kernel_log_level_is_respected(kernel_conn_params, caplog):
174-
"""At WARNING, the chatty DEBUG kernel records are filtered out
175-
before reaching Python — proving level control works (and that
176-
filtering happens, not that everything is forwarded unconditionally)."""
183+
"""At WARNING on the kernel logger, no DEBUG/INFO kernel records reach
184+
`caplog.records` — i.e. level control on `databricks.sql.kernel`
185+
behaves like any other Python logger.
186+
187+
Scope note: `caplog.at_level` sets the logger's level and attaches a
188+
handler, so this asserts the *effective* outcome a customer sees
189+
(sub-threshold records don't surface), not specifically that the Rust
190+
side suppressed them at source. A DEBUG record that crossed the FFI
191+
would still be dropped by Python's level check before reaching
192+
`caplog`. Source-side suppression (and its per-record FFI cost
193+
avoidance) is covered by the kernel-side filtering, not asserted
194+
here."""
177195
with caplog.at_level(logging.WARNING, logger="databricks.sql.kernel"):
178196
c = sql.connect(**kernel_conn_params)
179197
try:

0 commit comments

Comments
 (0)