Repository navigation
fix(telemetry): preserve unspecified kernel enablement - #497
Conversation
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
95fb228 to
a6c1925
Compare
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good — the bool→*bool tri-state for TelemetryConfig.Enabled is correctly threaded through kernelTelemetryConfig, applyTelemetry, the C shim, and all tests, preserving unspecified enablement as intended. One medium concern: the KERNEL_REV bump contradicts the PR description (which says it's unchanged) and the published bindings pin (v1.1.0 in go.mod) is not bumped alongside it, so a build against published archives would miss the new _v2 symbol.
| @@ -1 +1 @@ | |||
| 80f2aee7d884994d7b0af9a9ea6078872859a9cd | |||
| 7b158bb3100563b55cb46351f31cf7e83b4675db | |||
There was a problem hiding this comment.
🟡 Medium — The PR description states that KERNEL_REV is intentionally unchanged, but this diff bumps it (80f2aee7… → 7b158bb3…). This is a direct contradiction and should be reconciled before merge.
More importantly, the new kernel_session_config_set_telemetry_config_v2 symbol (added to the vendored header and now called unconditionally by go_kernel_set_telemetry_config) must be present in whatever kernel archive the build links against. The source-built path (make kernel-lib at the new KERNEL_REV) will have it, but the published bindings in go.mod are still pinned at v1.1.0 (unchanged in this diff). A kernel-tagged build against the published archive would fail to link against the missing _v2 symbol. The PR acknowledges it is draft pending a matching bindings release — so either keep KERNEL_REV unchanged as the description claims, or bump the bindings pin together with it so the two stay consistent.
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Looks sound — the *bool tri-state for enableTelemetry is threaded correctly through kernelTelemetryConfig, TelemetryConfig, and applyTelemetry, with tests and the one non-test consumer all updated. Two version-coupling concerns: the new kernel_session_config_set_telemetry_config_v2 symbol is absent from the still-pinned bindings v1.1.0 (kernel-tagged build will not link until bumped), and the PR description claims KERNEL_REV is unchanged while the diff bumps it.
| .retry_delay_ms = retry_delay_ms, | ||
| .close_flush_timeout_ms = close_flush_timeout_ms, | ||
| }; | ||
| return kernel_session_config_set_telemetry_config_v2(config, &telemetry_config); |
There was a problem hiding this comment.
🟡 Medium — The cgo wrapper now links kernel_session_config_set_telemetry_config_v2, but the prebuilt per-platform binding modules are still pinned at v1.1.0 in go.mod (unchanged by this PR). A standard go build -tags databricks_kernel ./... links the archives from those v1.1.0 modules, which predate the v2 symbol — so the kernel-tagged build will fail to link (undefined reference to kernel_session_config_set_telemetry_config_v2) until the bindings are bumped to a version containing it. The PR description acknowledges this is draft pending kernel #389 and a matching bindings release, so this is a gating/merge-order concern rather than a logic bug: the bindings pin (and go.sum) must be bumped before this can land, and that bump should be verified to actually export the v2 symbol.
(Anchored to the nearest changed line — see the description for the exact location.)
| @@ -1 +1 @@ | |||
| 80f2aee7d884994d7b0af9a9ea6078872859a9cd | |||
| 7ac7d9f42e7e2de3942337f4e3a077a03cfd63c4 | |||
There was a problem hiding this comment.
🔵 Low — The PR description states "The existing KERNEL_REV and bindings versions are intentionally unchanged," but this diff bumps KERNEL_REV from 80f2aee7… to 7ac7d9f4…. Only the bindings versions in go.mod are actually unchanged. Please reconcile the description with the diff — if the KERNEL_REV bump is intentional (to point source builds at a rev containing the v2 ABI), say so; if it was accidental, revert it. As written, the make kernel-lib source build (new rev) and the default binding-linked build (v1.1.0) target different kernel ABIs.
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Clean, consistent tri-state (*bool) conversion for kernel enableTelemetry across production and tests; logic and docs are accurate. One merge-gating concern: KERNEL_REV is bumped but the committed databricks-sql-kernel-bindings pins stay at v1.1.0, so the new ..._v2 symbol won't link against the shipped archives until the bindings are republished and re-pinned (author flagged this; PR is draft). Also re-verify the hand-added vendored C struct matches the upstream ABI before merge.
| @@ -1 +1 @@ | |||
| 80f2aee7d884994d7b0af9a9ea6078872859a9cd | |||
| 598966e83551926adb2870104c211b32ffae2401 | |||
There was a problem hiding this comment.
🟡 Medium — KERNEL_REV is bumped here (80f2aee7 → 598966e8), but the committed per-platform bindings in go.mod are still pinned at v1.1.0. The shim now calls kernel_session_config_set_telemetry_config_v2, a symbol the v1.1.0 archives (built from the old revision) do not export. A default kernel-tagged build (go test -tags databricks_kernel ./...) links against those committed archives — not the source-built .a from KERNEL_REV — so it will fail at link time with an undefined kernel_session_config_set_telemetry_config_v2 reference until the matching bindings module version is published and pinned in go.mod/go.sum.
The PR description acknowledges this dependency (databricks/databricks-sql-kernel#389) and marks the PR draft, so this is a merge-gating note rather than a logic defect. Two things worth reconciling before merge: (1) the description states "the existing KERNEL_REV ... [is] intentionally unchanged," which contradicts this diff that does bump it; (2) bump the databricks-sql-kernel-bindings v1.1.0 pins in go.mod to the version carrying the v2 symbol in the same change, so the committed-bindings build path stays linkable.
| * enablement setting from an explicit false. Passing `telemetry_config == NULL` | ||
| * leaves the kernel telemetry configuration unchanged. Otherwise the tuning | ||
| * fields are applied and `has_enabled` controls whether `enabled` overrides the | ||
| * workspace feature flag. The original positional setter above remains ABI |
There was a problem hiding this comment.
🔵 Low — The KernelTelemetryConfig struct and kernel_session_config_set_telemetry_config_v2 prototype are hand-added to the vendored header ahead of the upstream kernel landing them. The cgo shim initializes this struct by field with designated initializers, so its field order/types/size must match the kernel's canonical layout byte-for-byte; any divergence (field reorder, padding, extra fields the real ABI expects) would be a silent ABI mismatch at the C boundary rather than a compile error. Please re-verify this declaration against the published kernel header verbatim when #389 lands and the bindings are pinned, and keep the hand-added block minimal as noted.
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Clean, well-tested tri-state refactor of kernel telemetry enablement (bool → *bool): the Go logic in kernelTelemetryConfig/applyTelemetry, the cgo v2 shim, and the updated unit tests all line up correctly. One release-gating concern — the shipped go.mod bindings (v1.1.0) don't yet contain kernel_session_config_set_telemetry_config_v2, so this must stay draft until they're bumped — plus a minor KERNEL_REV/description mismatch.
2 participants
Summary
enableTelemetryas a tri-state when constructing kernel configurationfalsetrueandfalseas client overridesBehavior
enableTelemetryDependency
Depends on databricks/databricks-sql-kernel#389. This PR is draft until that lands and a matching
databricks-sql-kernel-bindingsversion containingkernel_session_config_set_telemetry_config_v2is published and pinned here. The existingKERNEL_REVand bindings versions are intentionally unchanged to avoid unrelated kernel API churn.Tests
go test ./...go test -tags databricks_kernel ./...against the source archive built from kernel PR Stop using html/template in U2M OAuth callback to restore linker DCE (fixes #343) #389