Leave kernel telemetry default unset

[jay-xiao446]

Summary

• leave kernel telemetry config nil whenever enableTelemetry is unset, so the kernel default decides whether telemetry is enabled
• forward telemetry_batch_size and telemetry_flush_interval to the kernel only when enableTelemetry is explicitly supplied
• update docs/tests to capture the kernel C ABI constraint that the enabled flag cannot be unset while tuning fields are supplied

Tests

• make lint
• go test -run 'TestBuildKernelConfig|TestKernelConfigFieldsClassified|TestKernelSkipsDriverTelemetry' .
• CGO_ENABLED=1 go test -tags databricks_kernel ./internal/backend/kernel -run TestSetTelemetry
• go test ./...
• make test-kernel

[peco-review-bot]

Verdict: 1 Medium

One medium concern: when only a telemetry tuning knob is set without enableTelemetry, kernelTelemetryConfig forwards Enabled=false, which the kernel setter treats as an explicit disable rather than "use the kernel default" — contradicting the PR's documented intent and regressing prior behavior. The fully-unset nil path and the explicit-enableTelemetry path both look correct. Docs updates are consistent with the code otherwise.

[peco-review-bot]

🟡 Medium — When a tuning knob is supplied but enableTelemetry is not (e.g. DSN sets only telemetry_batch_size=200 or telemetry_flush_interval), enableSet is false, so enabled takes the bool zero value false. The nil short-circuit above does not fire (a tuning field is non-zero), so this returns &kernel.TelemetryConfig{Enabled: false, BatchSize: ...}. applyTelemetry then forwards C.bool(t.Enabled) = false to kernel_session_config_set_telemetry_config, which is an explicit disable, not "leave the kernel default."

So tuning telemetry without also passing enableTelemetry=true now turns kernel telemetry OFF — the opposite of the documented contract for this PR (README/doc.go/CONNECTION_PARAMETERS: unset enableTelemetry should let "the kernel default decide"). It's also a regression from the prior behavior, where enabled defaulted to true.

The new test kernel telemetry tuning without enableTelemetry uses kernel enabled default illustrates the mismatch: its name says the kernel's enabled default should apply, but its assertion locks in Enabled=false — which the C setter interprets as a hard disable, not a default.

If the intent is "tune the knobs but leave enablement to the kernel," the Enabled field alone can't express that (there's no tri-state at the C ABI as modeled here). Consider either defaulting enabled to true when only tuning is set (restoring prior behavior), or documenting explicitly that supplying a tuning knob without enableTelemetry disables telemetry.

(Anchored to the nearest changed line — see the description for the exact location.)

[peco-review-bot]

Verdict: 1 Low

Looks good — a clean, well-tested change that leaves kernel Telemetry nil when enableTelemetry is unset so the kernel default governs. Downstream nil-handling (applyTelemetry) and the doc/test updates are all consistent; one Low note about the now-silent drop of tuning knobs when enableTelemetry is unset.

[peco-review-bot]

🔵 Low — When enableTelemetry is unset but a user supplies telemetry_batch_size / telemetry_flush_interval, this now returns nil and those tuning knobs are silently dropped (the new test kernel telemetry config is nil when enableTelemetry is unset sets TelemetryBatchSize=17/FlushInterval=9s and asserts Telemetry == nil, confirming the drop). This is a deliberate, documented consequence of the kernel C ABI constraint, but it sits in tension with this repo's "nothing silently ignored" contract — and unlike the sibling retry policy, which is deliberately logged at Debug in OpenSession precisely because forwarding is otherwise invisible, this drop leaves no trace. Consider a one-line Debug log (or a one-time warning) when tuning fields are set while enableTelemetry is unset, so an on-call reviewer can see the knobs were intentionally not forwarded. Not a correctness bug — the behavior matches the docs; this is an observability gap.