-
Notifications
You must be signed in to change notification settings - Fork 145
Forward telemetry parameters to the kernel binding #925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| NotSupportedError, | ||
| ProgrammingError, | ||
| ) | ||
| from databricks.sql.telemetry.telemetry_client import TelemetryHelper | ||
| from databricks.sql.thrift_api.TCLIService import ttypes | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -165,6 +166,31 @@ def _is_staging_statement(operation: str) -> bool: | |
| return verb in _STAGING_VERBS | ||
|
|
||
|
|
||
| def _kernel_telemetry_kwargs(options: Dict[str, Any]) -> Dict[str, Any]: | ||
| """Build phase-7 telemetry/system kwargs for ``databricks_sql_kernel.Session``.""" | ||
| system = TelemetryHelper.get_driver_system_configuration() | ||
| out: Dict[str, Any] = { | ||
| "driver_name": system.driver_name, | ||
| "driver_version": system.driver_version, | ||
| "runtime_name": system.runtime_name, | ||
| "runtime_version": system.runtime_version, | ||
| "runtime_vendor": system.runtime_vendor, | ||
| "os_name": system.os_name, | ||
| "os_version": system.os_version, | ||
| "os_arch": system.os_arch, | ||
| "client_app_name": system.client_app_name, | ||
| "locale_name": system.locale_name, | ||
| "char_set_encoding": system.char_set_encoding, | ||
| # The Python telemetry model does not currently track process | ||
| # name; omit it and let the kernel fill what it can derive. | ||
| "process_name": None, | ||
| "telemetry_enabled": bool(options.get("enable_telemetry", True)), | ||
| } | ||
| if options.get("telemetry_batch_size") is not None: | ||
| out["telemetry_batch_size"] = options["telemetry_batch_size"] | ||
| return out | ||
|
|
||
|
|
||
| # ─── Client ───────────────────────────────────────────────────────────────── | ||
|
|
||
|
|
||
|
|
@@ -217,6 +243,9 @@ def __init__( | |
| # to the kernel ``Session``'s ``retry_*`` kwargs in | ||
| # ``open_session`` via ``_kernel_retry_kwargs``. | ||
| self._retry_options = kwargs.get("retry_options") or {} | ||
| # Kernel telemetry phase 7 adds binding/runtime identity and | ||
| # telemetry config kwargs directly to ``databricks_sql_kernel.Session``. | ||
| self._telemetry_options = kwargs.get("telemetry_options") or {} | ||
| self._catalog = catalog | ||
| self._schema = schema | ||
| # ``_use_arrow_native_complex_types`` is the connector-side | ||
|
|
@@ -316,6 +345,7 @@ def open_session( | |
| # Translate the connector's ``_retry_*`` kwargs into the | ||
| # kernel's ``retry_*`` kwargs. Empty when at defaults. | ||
| retry_kwargs = _kernel_retry_kwargs(self._retry_options) | ||
| telemetry_kwargs = _kernel_telemetry_kwargs(self._telemetry_options) | ||
| # Forward caller / connector HTTP headers. The kernel applies | ||
| # them on every request; a caller ``User-Agent`` is appended | ||
| # to the kernel's base UA. Only pass the kwarg when there's | ||
|
|
@@ -358,6 +388,7 @@ def open_session( | |
| **auth_kwargs, | ||
| **tls_kwargs, | ||
| **retry_kwargs, | ||
| **telemetry_kwargs, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — Unlike |
||
| **http_headers_kwargs, | ||
| ) | ||
| except Exception as exc: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,6 +122,9 @@ def get_auth_flow(auth_provider): | |
|
|
||
| @staticmethod | ||
| def is_telemetry_enabled(connection: "Connection") -> bool: | ||
| if getattr(connection.session, "use_kernel", False) is True: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — The kernel bypass uses an identity check ( Recommend matching the routing semantics with a plain truthiness check so the two code paths can't diverge: if getattr(connection.session, "use_kernel", False):
return FalseMinor, since |
||
| return False | ||
|
|
||
| # Fast path: force enabled - skip feature flag fetch entirely | ||
| if connection.force_enable_telemetry: | ||
| return True | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Low —
_kernel_telemetry_kwargsbuilds 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 aTypeError: Session() got an unexpected keyword argument ...at connect time. If phase-7 requires a minimum kernel version, consider bumping the lower bound of thedatabricks-sql-kernelpin so the wheel and connector stay in lockstep. (Flagged Low — I can't verify the kernelSessionsignature from this repo.)