Let kernel own default telemetry enabled state - #936
Conversation
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the omit-when-None telemetry threading is internally consistent (session.py forwards raw enable_telemetry, client.py omits it when None) and the tests are updated to match. One low-severity note: this reverses a formerly-deliberate kernel default-OFF divergence, and the resulting default is now whatever the opaque kernel picks — worth confirming it matches the intended posture.
| "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"), |
There was a problem hiding this comment.
🔵 Low — This reverses a previously-deliberate divergence: the removed comments here and in client.py stated the kernel path intentionally defaulted telemetry to OFF ("Do not 'fix' this back to True to match the other backends"). By omitting telemetry_enabled when unset, the effective default is now whatever the Rust kernel picks. If the kernel's internal default is ON (to match the connector-wide True default at client.py:435), this silently flips unset-telemetry from off→on for use_kernel=True — a behavior change no unit test can catch, since the tests only assert the key is omitted, not the resulting kernel behavior. Worth confirming the kernel's default matches the intended privacy posture before merge, and ideally noting the expected default in the comment so a future reader isn't left guessing. Not blocking — the PR title indicates this hand-off is intentional — but the loss of the explicit assertion means the actual default is now unverified in this repo.
Summary
Validation