Forward kernel telemetry options

[jay-xiao446]

Summary

• forward driver/runtime identity and telemetry config into kernel openSession options
• forward the latest kernel telemetry tuning knobs: batch size, flush interval, retry count/delay, close flush timeout, and circuit-breaker threshold/timeout
• disable Node-wrapper telemetry on useKernel connections to avoid duplicate telemetry
• preserve DATABRICKS_TELEMETRY_DISABLED as a hard opt-out for kernel-owned telemetry
• rebase onto latest main, preserving token-cache/static-token kernel work already merged there

Tests

• npx tsc --project tsconfig.build.json --noEmit
• npx mocha --config tests/unit/.mocharc.js tests/unit/kernel/execution.test.ts tests/unit/kernel/connectionOptions.test.ts tests/unit/DBSQLClient.test.ts

Notes:

• npx tsc --noEmit still fails on existing token federation example imports of @databricks/sql before package build artifacts exist.
• eslint on the touched test specs is blocked by existing test-file lint debt; touched lib files produced no lint errors.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Clean, well-tested forwarding of telemetry/runtime identity into the kernel path, and the wrapper-telemetry suppression on useKernel is correct. One medium: getLocaleName uses inverted POSIX locale precedence (checks LANG before LC_ALL/LC_MESSAGES), so overrides are ignored. One low: duplicated env-disable parsing that could drift from DBSQLClient.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested forwarding change — routing telemetry into kernel openSession and disabling the Node wrapper on the kernel path both look correct and are covered. Two things worth confirming: (1) DBSQLClient always seeds these telemetry config fields, so the isFinite "omit ⇒ kernel default" guards never actually omit — the kernel always inherits Node's defaults; (2) the circuit-breaker threshold/timeout are forwarded without ever setting telemetryCircuitBreakerEnabled. Nit (no anchor needed): getLocaleName/getProcessName are now duplicated between DBSQLClient and KernelAuth with a subtle divergence (the kernel copies apply sanitizeProcessName, the DBSQLClient ones don't) — consider sharing one implementation.

[peco-review-bot]

🟡 Medium — The Number.isFinite(...) guards here are meant to omit a knob so the kernel keeps its own default — native/kernel/index.d.ts documents each as "Omitted ⇒ kernel default." But when this runs through the normal DBSQLClient flow, getDefaultConfig() (lib/DBSQLClient.ts:170-185) always populates every one of these telemetry fields with a finite value from DEFAULT_TELEMETRY_CONFIG. So every guard always passes and the kernel always receives the Node connector's telemetry defaults (batchSize 100, flushIntervalMs 5000, maxRetries 3, backoffBaseMs 100, closeTimeoutMs 2000, circuitBreakerThreshold 5, circuitBreakerTimeout 60000) — its own tuned defaults are never used.

Those defaults were chosen for the JS HTTP exporter's batching/backoff, not the kernel's Rust telemetry pipeline. If that override is intended, the "Omitted ⇒ kernel default" wording and the isFinite guards are misleading (the omit path is only reachable from a hand-built config, e.g. the unit tests). If it isn't intended, the connector is silently overriding the kernel's telemetry tuning. Worth confirming which behavior you want and aligning the guards/docs accordingly.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

Confirmed the reviewer is correct: DBSQLClient.getDefaultConfig() (lib/DBSQLClient.ts:170-185) always populates every telemetry field from DEFAULT_TELEMETRY_CONFIG with finite values, so the Number.isFinite guards in buildKernelTelemetryOptions always pass in the normal client flow and the kernel never uses its own tuned defaults (the omit path is only reachable from hand-built configs like the unit tests). Resolving this is a design decision with two opposite outcomes — either (a) the override is intended and the "Omitted ⇒ kernel default" docs + isFinite guards should be reworded/removed, or (b) it's unintended and getDefaultConfig should stop populating these so the kernel keeps its Rust-pipeline tuning (a behavioral change to a widely-consumed connector). Which telemetry defaults should win (JS connector vs Rust kernel) is a product/design judgment I can't make from the code and can't verify here; needs a human to decide intent before either the docs/guards or getDefaultConfig are changed.

[peco-review-bot]

Verdict: 1 Low

Looks good — the telemetry-forwarding logic, env kill-switch unification, and Node-telemetry disabling on the kernel path are correct, well-tested, and all forwarded fields are declared on the napi .d.ts. One low-severity inconsistency: the kernel and Thrift getLocaleName implementations use different env-var precedence, so the same process can report different localeName across backends.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks good overall — the lib/ changes (kernel telemetry forwarding, shared isTelemetryDisabledByEnv, disabling Node-wrapper telemetry on kernel connections) are clean and well-tested. One medium concern: native/kernel/index.d.ts is an auto-generated NAPI-RS file being hand-edited (verify the binding supports the new fields and regenerate). Plus a low note that the shared test helper now strips all telemetry keys, weakening coverage on the non-telemetry paths.

[peco-review-bot]

🟡 Medium — This file is auto-generated by NAPI-RS (/* auto-generated by NAPI-RS */ on line 1), but the three new telemetryCircuitBreaker* fields are being added by hand. Two risks:

1. Regeneration drift — the next napi build will regenerate this .d.ts from the Rust #[napi] structs. If the Rust source doesn't actually declare these fields, the hand-added declarations are lost and the TypeScript will stop compiling; if it does declare them, the hand-edit was unnecessary and should have come from a regen.
2. Silent no-op — buildKernelTelemetryOptions sets telemetryCircuitBreakerEnabled: true (and forwards telemetryCircuitBreakerThreshold/...TimeoutMs) specifically so the breaker knobs take effect. But the .d.ts only describes the JS→native contract; it does not make the compiled .node binding honor the fields. If the shipped binding predates these fields, napi silently drops the unknown properties and the breaker is never enabled — defeating the stated purpose of the change while the tests (which assert against a JS stub, not the real binding) still pass.

Please confirm the compiled kernel binding actually accepts these three fields and, if so, regenerate this file from the Rust source rather than editing it directly.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

The comment targets native/kernel/index.d.ts, a NAPI-RS auto-generated file under native/ — a denied/generated boundary I cannot read or edit here. Both concerns (regen drift vs. the Rust #[napi] structs, and whether the compiled .node binding actually honors telemetryCircuitBreaker*) can only be resolved by inspecting the Rust source and rebuilding/regenerating in the native build pipeline — out-of-band from this TypeScript PR, and unverifiable by the mocked unit suite (which asserts against a JS stub, not the real binding) with no live warehouse creds on this job. The JS-side wiring in lib/kernel/KernelAuth.ts (buildKernelTelemetryOptions enabling the breaker and forwarding the knobs) is correct and needs no change. A human owning the native/kernel Rust source must confirm the fields exist on the #[napi] struct and regenerate the .d.ts from source rather than hand-editing it.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested wiring of runtime identity + telemetry knobs into the kernel openSession path; the shared isTelemetryDisabledByEnv helper and the !useKernel gate on Node-wrapper telemetry are clean and correct. One Medium: the new kernel-path telemetry unit test depends on the native binding being present and will fail in CI where it isn't built (unlike its sibling test, which tolerates that). One Low around forcing the circuit breaker on while forwarding a possibly-zero threshold.

[peco-review-bot]

Verdict: 1 Medium

Looks good overall — clean, well-tested telemetry forwarding with a single-source env opt-out and correct !useKernel gating to avoid duplicate telemetry. One medium consistency gap: the strict-positive guard applied to the circuit-breaker knobs is not applied to telemetryBatchSize/telemetryFlushIntervalMs/telemetryCloseTimeoutMs, which the napi contract documents with the same must-be-positive constraint, so a caller-supplied 0 there could still hard-reject openSession.

[peco-review-bot]

Verdict: 1 Low

Looks good — clean, well-tested forwarding of kernel telemetry options with correct field-name mapping and env/feature gating. One low-severity guard-consistency gap: the telemetryMaxRetries (user-settable) guard admits negative values where the sibling positive-only fields are defended.

[peco-review-bot]

Verdict: 2 Low

Looks good overall — clean forwarding of runtime identity + telemetry knobs, with correct guards, verified config/napi field-name alignment, and proper gating of Node-path telemetry off kernel connections. Two low-severity notes: the new kernel getProcessName sanitizes process.title while the Thrift-path one doesn't (backend-inconsistent telemetry processName), and the forwarded retry delay is not actually user-configurable since telemetryBackoffBaseMs isn't a public option.