Skip to content

Let kernel own default telemetry enabled state - #937

Closed
jay-xiao446 wants to merge 1 commit into
jay/disable-python-telemetry-kernelfrom
jay/kernel-telemetry-enabled-pass-through
Closed

Let kernel own default telemetry enabled state#937
jay-xiao446 wants to merge 1 commit into
jay/disable-python-telemetry-kernelfrom
jay/kernel-telemetry-enabled-pass-through

Conversation

@jay-xiao446

Copy link
Copy Markdown
Collaborator

Summary

  • pass through explicit enable_telemetry values to the kernel
  • omit telemetry_enabled from kernel Session kwargs when the user does not specify enable_telemetry
  • update tests to assert the unset/None case lets the kernel default apply

Validation

  • python3.12 -m py_compile src/databricks/sql/backend/kernel/client.py src/databricks/sql/session.py tests/unit/test_session.py tests/unit/test_telemetry.py tests/unit/test_kernel_client.py
  • git diff --check

Signed-off-by: Jay Xiao <jay.xiao@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Medium

Looks sound — the None-passthrough is reachable (client.py never defaults enable_telemetry into kwargs before Session), the is not None guard correctly forwards explicit True/False while omitting unset, and tests are updated to match. One medium concern: this removes a deliberately-documented opt-out default on the kernel path, and the resulting default now depends on an external databricks_sql_kernel value not visible in this repo — worth confirming it matches intent.

"enable_telemetry": kwargs.get("enable_telemetry", False),
# Preserve the caller's explicit telemetry choice. When unset,
# leave it as None so the kernel applies its own default.
"enable_telemetry": kwargs.get("enable_telemetry"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium — This reverses a previously-deliberate divergence. The old code (and its removed comment) explicitly forced kernel-path telemetry OFF by default — "telemetry_enabled": bool(options.get("enable_telemetry", False)) in client.py — and warned "Do not 'fix' this back to True to match the other backends." With this change, when a use_kernel=True caller does not pass enable_telemetry, the key is omitted and the kernel applies its own default.

The resulting default is not verifiable in this repo — it lives in databricks_sql_kernel. If the kernel defaults telemetry_enabled to True (which would match the connector-wide enable_telemetry default of True at client.py:435), this silently flips kernel telemetry from opt-out to opt-in for users who never set the flag — a privacy-relevant default change that goes beyond the "let kernel own the default" framing.

Please confirm the kernel's built-in default matches the intended behavior here, and consider noting the effective default in the comment (or a test asserting the end-to-end default) so a future kernel-side change to that default can't silently alter connector behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant