Forward telemetry parameters to the kernel binding - #925
Conversation
8816d71 to
ae59e2c
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the core gate (is_telemetry_enabled returns False for kernel connections, via a safe getattr) is correct and well-tested, and session.use_kernel is guaranteed set before it's read. One low-severity gap: the connection-failure telemetry path (connection_failure_log) is not gated on use_kernel, so a failed kernel connect still emits a Python-side failure log.
Other findings
- 🔵 Low — This PR disables Python-side telemetry for kernel connections by short-circuiting
is_telemetry_enabled(telemetry_client.py:125). However, the connection-failure telemetry path is a separate code path that bypassesis_telemetry_enabledentirely: whensession.open()raises,connection_failure_logis invoked and gated only onenable_telemetry(defaulting toTrue), never onuse_kernel. So a kernel connection that fails to open will still emit a Python-side failure log — inconsistent with the PR's stated goal of disabling Python telemetry for kernel connections (and potentially duplicating what the kernel reports). If failure-before-kernel-open telemetry is intentionally still desired on the Python side, this is fine as-is; otherwise consider passinguse_kernelthrough and skippingconnection_failure_logon that path.
ae59e2c to
449d175
Compare
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall — the change cleanly disables Python-side telemetry on kernel connections and threads phase-7 identity/telemetry kwargs into the kernel Session, with solid regression tests (including the force-enabled case). Two low-severity notes: an identity-vs-truthiness mismatch between the telemetry bypass and backend routing, and a potential wheel-version compat gap since the new kwargs are passed unconditionally against a ^0.2.0 pin.
| return verb in _STAGING_VERBS | ||
|
|
||
|
|
||
| def _kernel_telemetry_kwargs(options: Dict[str, Any]) -> Dict[str, Any]: |
There was a problem hiding this comment.
🔵 Low — _kernel_telemetry_kwargs builds the phase-7 identity/telemetry kwargs (driver_name, telemetry_enabled, process_name, etc.) and they are spread unconditionally into _kernel.Session(**telemetry_kwargs) at open_session. The kernel wheel constraint is still ^0.2.0 (>=0.2.0,<0.3.0). If these kwargs were introduced in a later 0.2.x than 0.2.0, a user with an older-but-constraint-satisfying wheel installed would hit a TypeError: Session() got an unexpected keyword argument ... at connect time. If phase-7 requires a minimum kernel version, consider bumping the lower bound of the databricks-sql-kernel pin so the wheel and connector stay in lockstep. (Flagged Low — I can't verify the kernel Session signature from this repo.)
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
I've verified the reviewer's concern against the real installed wheel and flagged it for a human. Summary:
- Concern is valid. The installed
databricks_sql_kernel0.2.0Sessionsignature accepts none of the phase-7 telemetry kwargs that_kernel_telemetry_kwargsbuilds (nor evenrequest_timeout_secs). A user with 0.2.0 — which satisfies the^0.2.0pin — would hitTypeErroratconnecttime, and the@realkernelrouting test can't catch it because it mocksopen_session. - Why blocked, not fixed: the fix is a pin lower-bound bump in
pyproject.toml, but the correct floor version can't be determined from this environment (only 0.2.0 is present; no network/PyPI access to find which 0.2.x introduced the kwargs). Guessing risks mis-constraining a widely-used connector, s
Pushed 6cba809 (bundled with 4 other thread(s)).
There was a problem hiding this comment.
it's okay, the required kernel version will be updated once kernel is released
There was a problem hiding this comment.
Thanks — that timing plan makes sense. Just to close the loop for anyone reading the thread later: engineer-bot confirmed the concern is real (the installed 0.2.0 Session signature rejects the phase-7 telemetry kwargs), and the diff at 6cba809 only added an explanatory comment, so the ^0.2.0 lower bound is still in place. Leaving this open as a tracking reminder to bump the databricks-sql-kernel floor in pyproject.toml once the kernel release lands, so the wheel and connector stay in lockstep.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
Converged / needs-human, and already resolved out-of-band: maintainer @jay-xiao446 decided the databricks-sql-kernel pin floor will be bumped once the kernel is released, so it deliberately cannot ride in this PR. The reviewer's concern is valid (installed 0.2.0 Session rejects the phase-7 telemetry kwargs built by _kernel_telemetry_kwargs), but the fix is a pyproject.toml lower-bound bump gated on an unreleased kernel version — an out-of-band change, not actionable here. Thread is being kept open only as a tracking reminder; no code change warranted.
449d175 to
8261c66
Compare
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall — the kernel-path telemetry forwarding and the is_telemetry_enabled short-circuit are correct, defaults line up between connect()/session.py/_kernel_telemetry_kwargs, and the new tests are meaningful. Two low-severity notes: (1) the connection-failure telemetry path isn't gated on use_kernel, so the Python wrapper still emits failure telemetry on the kernel path; (2) the phase-7 Session kwargs are forwarded unconditionally while the kernel dependency floor stays ^0.2.0, a potential connect-time break if an older wheel is installed.
Other findings
- 🔵 Low — The kernel-path guard in
is_telemetry_enableddisables Python-side telemetry only for the successful connection path. When ause_kernel=Trueconnection fails duringsession.open(), thisconnection_failure_log(...)call still fires withenable_telemetry=kwargs.get("enable_telemetry", True)— it is not gated onuse_kernel. So the Python wrapper still emits a connection-failure telemetry event on the kernel path, which is the kind of wrapper-owned telemetry this PR set out to suppress foruse_kernel. It may be intentional (the kernel session never opened, so it couldn't emit its own failure telemetry), but if the goal is "no Python-side connector telemetry on the kernel path," this branch is an unguarded exception worth confirming.
| **auth_kwargs, | ||
| **tls_kwargs, | ||
| **retry_kwargs, | ||
| **telemetry_kwargs, |
There was a problem hiding this comment.
🔵 Low — Unlike retry_kwargs/http_headers_kwargs (which are conditionally omitted when empty), _kernel_telemetry_kwargs always returns the phase-7 identity fields (driver_name, runtime_*, os_*, process_name, telemetry_enabled), so these kwargs are passed to _kernel.Session(...) on every use_kernel open. If the installed kernel wheel predates phase-7 support for these Session kwargs, construction raises TypeError and every use_kernel=True connection breaks. The dependency floor is still ^0.2.0 and isn't bumped in this PR. If phase-7 requires a newer kernel wheel, consider raising the minimum version so incompatible wheels fail at install time rather than at connect time. (Low because I can't verify the 0.2.0 Session signature from this repo — the kernel is a compiled extension.)
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
I've completed my investigation and reported the outcome as blocked.
Summary: The reviewer flagged that phase-7 telemetry kwargs are passed unconditionally to _kernel.Session(...) on every use_kernel=True open, and would raise TypeError if the installed kernel wheel predates phase-7 support. I verified this empirically using the real 0.2.0 wheel present on the runner:
Session.__text_signature__contains none of the phase-7 kwargs.Session(..., telemetry_enabled=True)raisesTypeError: Session.__new__() got an unexpected keyword argument 'telemetry_enabled'.0.2.0is both this PR's floor and the latest published version — so everyuse_kernel=Trueconnection would break at connect time.
I escalated rather than editing because the reviewer's suggested fix (r
Pushed 6cba809 (bundled with 4 other thread(s)).
There was a problem hiding this comment.
it's okay, the required kernel version will be updated once kernel is released
There was a problem hiding this comment.
Thanks — that matches what the engineer-bot found empirically (the shipped 0.2.0 wheel rejects telemetry_enabled with a TypeError), so this is a real break for use_kernel=True connections until that kernel release lands. Since the version floor isn't bumped in this PR and the fix is deferred to the future kernel release, I'll leave this open so it isn't lost track of — worth bumping the ^0.2.0 floor in lockstep with that release so incompatible wheels fail at install time rather than at connect time.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
Converged + deferred out-of-band. Maintainer (jay-xiao446) explicitly decided the required kernel version will be bumped once the phase-7 kernel wheel is released; the review-bot is only keeping the thread open as a tracking reminder to raise the ^0.2.0 floor in lockstep. The concern is real (verified previously: the shipped 0.2.0 wheel raises TypeError on telemetry_enabled), but the fix — bumping the dependency floor — can only be actioned when the new kernel wheel is published and cannot ride in this PR. No safe in-PR code change exists (pinning to an unpublished version breaks install; omitting the identity kwargs disables the telemetry this PR ships). Human action: bump the kernel minimum version alongside the phase-7 kernel release.
8261c66 to
088ff16
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-tested change. Python-side telemetry is correctly disabled for kernel connections (session is assigned before the is_telemetry_enabled check), and kernel telemetry_enabled follows enable_telemetry as intended. One low note: the kernel path's default telemetry_batch_size differs from the Thrift/SEA path when the caller leaves it unset.
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall — the threading of telemetry/system kwargs into the kernel Session, the Python-side telemetry disable for the kernel path, and the accompanying unit coverage are all coherent. One medium concern: the kernel enable_telemetry default (False) diverges from the connector's documented default (True), so kernel connections that don't explicitly set the flag get telemetry silently off unlike Thrift/SEA.
Addresses: - #3833615986 at src/databricks/sql/telemetry/telemetry_client.py:125 - #3833615990 at src/databricks/sql/backend/kernel/client.py:176 - #3833661667 at src/databricks/sql/backend/kernel/client.py:419 - #3858839800 at src/databricks/sql/session.py:228 - #3876277610 at src/databricks/sql/session.py:260 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the telemetry wiring is correct. Session.__init__ sets use_kernel before Connection calls is_telemetry_enabled, so the kernel-path telemetry suppression and the forwarded kernel kwargs (identity, telemetry_enabled, batch size, circuit breaker) are consistent with the described design and well covered by tests. One low-severity cleanup: two leftover probe files were committed.
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the telemetry parameter forwarding is coherent and well covered by tests. The use_kernel opt-out in is_telemetry_enabled (telemetry_client.py:125) uses getattr(..., False), which correctly handles both real sessions (attribute set in _create_backend before the check at client.py:432) and the MagicMock-based tests. One minor default-parity inconsistency noted inline (Low). Separately, note that the connection_failure_log path (client.py:404-419) is not gated on use_kernel and would still emit Python-side failure telemetry for a failed kernel connection — a small gap in the "no duplicate kernel-path telemetry" goal, though arguably acceptable since it's the failure path.
Addresses: - #3876849398 at src/databricks/sql/session.py:276 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-tested change forwarding telemetry config to the kernel Session and disabling duplicate wrapper telemetry on the kernel path. Option threading, defaults (enable_telemetry→False, batch_size/circuit_breaker parity), force_enable_telemetry semantics, and the getattr guard on connection.session all check out, and session creation precedes telemetry init so the new check is safe. One low-severity completeness note: the connect-time connection_failure_log path is not gated by the new kernel opt-out.
Addresses: - #3876884856 at src/databricks/sql/telemetry/telemetry_client.py:125 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-commented, and well-tested telemetry forwarding into the kernel Session. The use_kernel gate in is_telemetry_enabled is safe (session.use_kernel is set in _create_backend before the check at client.py:436), key names line up across session.py/client.py/kernel/client.py, and the intentional default divergences (enable_telemetry False on kernel, force_enable_telemetry as wrapper-only) are documented and covered by tests. One low-severity observability note on the failure-path suppression is filed inline; note also that force_enable_telemetry=True has no effect on the kernel path (wrapper off + kernel follows only enable_telemetry), which is intentional but may surprise users.
| # Respect user's telemetry preference even during connection failure. | ||
| # For use_kernel connections the kernel owns telemetry, so suppress | ||
| # the wrapper-side failure log to avoid wrapper-vs-kernel duplication. | ||
| enable_telemetry = kwargs.get("enable_telemetry", True) and not kwargs.get( |
There was a problem hiding this comment.
🔵 Low — This suppresses the wrapper-side connection-failure log for use_kernel=True, delegating failure telemetry to the kernel. But a connect() failure happens inside KernelDatabricksClient.open_session (where the kernel Session is first constructed), so if the kernel never finished initializing/flushing its telemetry, the failure may not be reported by either side. Combined with the kernel path defaulting enable_telemetry to False (session.py), a default use_kernel=True connection that fails to open reports no connection-failure telemetry anywhere — whereas a Thrift connection (default enable_telemetry=True) does. Worth confirming the kernel actually captures pre-session-open connect failures; otherwise this is an observability regression for the kernel path. Design tradeoff, not a correctness bug.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
The reviewer raises a genuine but design-level observability tradeoff, explicitly "not a correctness bug." The deciding fact — whether the kernel captures/flushes connection-failure telemetry when its own databricks_sql_kernel.Session constructor throws before init completes (client.py open_session:401-423) — lives in the compiled Rust kernel wheel, which is not readable from this repo, and can only be verified against a live warehouse (E2E), which this follow-up job has no credentials to run. The connector side is confirmed to be deliberately silent for use_kernel (is_telemetry_enabled short-circuits False at telemetry_client.py:124-125; _kernel_telemetry_kwargs defaults telemetry_enabled=False "by design" at client.py:194-197), so if the kernel does NOT capture pre-session-open failures, a default use_kernel=True connect that fails would report no failure telemetry anywhere. Resolving this needs a human to (1) confirm the kernel's pre-Session-open failure telemetry behavior, and (2) decide the wrapper-vs-kernel duplication vs. coverage tradeoff — no in-PR code change can settle it here.
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — well-scoped and thoroughly tested; the kwarg names, defaults, and the auth/common.py:55 citation all check out, and the session→client ordering means session.use_kernel is set before is_telemetry_enabled reads it. One low note: the kernel path forwards enable_telemetry as None when unset (deferring to a kernel default) while the wrapper path gates the same case behind a server feature flag, which may diverge in the never-opted-in case.
Addresses: - #3877217286 at src/databricks/sql/session.py:263 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-tested change forwarding telemetry config to the kernel binding and suppressing wrapper-side telemetry on the use_kernel path. Parity/default claims in the comments verify against the code, and there's no circular-import issue. One low-severity note about the explicit enable_telemetry=True case relying on an unverifiable kernel-side feature-flag gate.
| # kernel's own default/gating, which is expected to mirror the | ||
| # feature-flag-gated wrapper behaviour; only an explicit caller | ||
| # opt-in/opt-out overrides it. | ||
| "enable_telemetry": kwargs.get("enable_telemetry"), |
There was a problem hiding this comment.
🔵 Low — Forwarding an explicit enable_telemetry=True sets the kernel's telemetry_enabled=True unconditionally, whereas the Thrift/SEA path only actually emits when the server-side enableTelemetryForPythonDriver feature flag is on (see is_telemetry_enabled). The inline comment acknowledges this and states the kernel "is expected to mirror the feature-flag-gated wrapper behaviour" — but that gate lives entirely on the kernel side and can't be verified from this repo. If the kernel does not re-implement the feature-flag gate, a caller passing enable_telemetry=True on use_kernel=True will get telemetry emitted that the Thrift path would have suppressed. Worth confirming the kernel honors the flag, or documenting the divergence for users; the None-when-unset default is fine, this is specifically about the explicit-True case.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
Valid concern, but not resolvable with a code change in this repo. The reviewer confirms the None-when-unset default is fine; the open question is only the explicit enable_telemetry=True case, where the correctness of forwarding depends on whether the kernel re-implements the server-side enableTelemetryForPythonDriver gate. That gate lives entirely on the kernel side and cannot be read or verified from this repo, so no local edit or further thread exchange can settle it. The divergence is already heavily documented in the inline comment at session.py:271-283; whether to add user-facing telemetry-parity docs is a public-API/documentation call for the connector maintainers. A human who owns the kernel telemetry path must confirm the kernel honors the feature flag (or decide to document the divergence for users).
There was a problem hiding this comment.
it's expected, telemetry for kernel requires opt-in
There was a problem hiding this comment.
Thanks for confirming — that the kernel treats enable_telemetry=True as an explicit opt-in (rather than mirroring the server-side enableTelemetryForPythonDriver gate) is exactly the design point I couldn't verify from this repo, so it's good to have it from someone who owns that path. The one thing that's still worth a moment: this is an intentional behavioral divergence from the Thrift/SEA path, where an explicit True can be suppressed by the feature flag. Since the reasoning lives only in the inline comment, a short user-facing note that kernel telemetry is opt-in and not feature-flag-gated would spare callers a surprise. I'll leave this open so that documentation decision stays on a maintainer's radar rather than getting silently resolved.
There was a problem hiding this comment.
I added a user-facing telemetry note to the use_kernel parameter docstring in src/databricks/sql/client.py, addressing the reviewer's request. The note documents that on the kernel path telemetry is strictly opt-in — an explicit enable_telemetry=True enables it unconditionally and is not gated by the server-side enableTelemetryForPythonDriver feature flag (an intentional divergence from the Thrift/SEA paths, where an explicit True can still be suppressed by the flag). This moves the reasoning out of the internal inline comment and into public docs where callers will see it. The change is docstring-only with no logic impact, so no tests were affected.
Pushed 02002a1.
Addresses: - #3877271142 at src/databricks/sql/session.py:275 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Summary
databricks_sql_kernel.Session: driver/runtime identity,telemetry_enabled, andtelemetry_batch_size.telemetry_enabledfromenable_telemetry; keepforce_enable_telemetryas a Python wrapper-only feature-flag bypass concept.use_kernel=Trueconnections so wrapper telemetry does not duplicate kernel-owned telemetry.enable_telemetry.Tests
.venv/bin/python -m pytest tests/unit/test_telemetry.py -q.venv/bin/python -m pytest tests/unit/test_session.py -q.venv/bin/python -m pytest tests/unit/test_kernel_client.py -q