Skip to content

Commit df4fba5

Browse files
rahuls-dbIsaac
andcommitted
fix: attribute recovered-kernel connection failures to the kernel for telemetry
The connection-failure telemetry suppression read the original connect() kwargs to decide whether the failed connection was a kernel connection. On the Reyden auto-recovery path the kernel retry uses a kwargs copy, so the original still said Thrift — a kernel open-failure was logged by the wrapper despite the kernel owning telemetry for kernel connections. Decide from the session that actually failed (self.session.use_kernel) instead. If the kernel was never constructed (e.g. its wheel is missing), self.session stays Thrift and the wrapper still logs, so that otherwise-invisible failure is still recorded. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
1 parent f5e8adb commit df4fba5

2 files changed

Lines changed: 32 additions & 4 deletions

File tree

src/databricks/sql/client.py

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -417,10 +417,18 @@ def read(self) -> Optional[OAuthToken]:
417417
)
418418
except Exception as e:
419419
# Respect user's telemetry preference even during connection failure.
420-
# For use_kernel connections the kernel owns telemetry, so suppress
421-
# the wrapper-side failure log to avoid wrapper-vs-kernel duplication.
422-
enable_telemetry = kwargs.get("enable_telemetry", True) and not kwargs.get(
423-
"use_kernel", False
420+
# For a kernel connection the kernel owns telemetry, so suppress the
421+
# wrapper-side failure log to avoid wrapper-vs-kernel duplication.
422+
# Read the backend from the session that actually failed rather than
423+
# the caller's kwargs: on the Reyden auto-recovery path we retry on
424+
# the kernel via a kwargs copy, so the original kwargs still says
425+
# Thrift. If the kernel never got constructed (e.g. its wheel is
426+
# missing), self.session is the Thrift session and we still log.
427+
attempted_kernel = getattr(
428+
getattr(self, "session", None), "use_kernel", False
429+
)
430+
enable_telemetry = (
431+
kwargs.get("enable_telemetry", True) and not attempted_kernel
424432
)
425433
TelemetryClientFactory.connection_failure_log(
426434
error_name="Exception",

tests/unit/test_session.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -910,3 +910,23 @@ def test_kernel_retry_failure_chains_both_errors(self, mock_thrift):
910910
# Kernel failure is surfaced as primary; the Thrift rejection is
911911
# preserved in the chain for diagnosis.
912912
assert isinstance(excinfo.value.__cause__, ReydenThriftUnsupportedError)
913+
914+
@patch("%s.session.ThriftDatabricksClient" % PACKAGE)
915+
def test_recovered_kernel_failure_suppresses_wrapper_telemetry(self, mock_thrift):
916+
# A connection that recovered onto the kernel and then failed there must
917+
# NOT emit the wrapper's connection-failure log — the kernel owns
918+
# telemetry for kernel connections. Guards against reading the original
919+
# (Thrift) kwargs instead of the session that actually failed.
920+
mock_thrift.return_value.open_session.side_effect = self._reject()
921+
with self._fake_kernel() as mock_kernel, patch(
922+
"databricks.sql.client.TelemetryClientFactory.connection_failure_log"
923+
) as mock_fail_log:
924+
mock_kernel.return_value.open_session.side_effect = OperationalError(
925+
"kernel boom"
926+
)
927+
# enable_telemetry=True so only the kernel-suppression logic can flip
928+
# it off — proving the fix rather than the user's opt-out.
929+
with pytest.raises(OperationalError):
930+
self._connect(enable_telemetry=True)
931+
mock_fail_log.assert_called_once()
932+
assert mock_fail_log.call_args.kwargs["enable_telemetry"] is False

0 commit comments

Comments
 (0)