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

Mechanically sound: field names, guards, and env-parity all check out, and the tests cover the forwarded shape well. One medium concern: buildKernelTelemetryOptions never reads config.telemetryEnabled (declared in its Pick but unused), so kernel telemetry defaults off-by-default while Thrift defaults on — a silent behavioral divergence when a connection switches to useKernel.

[peco-review-bot]

Verdict: 2 Low

Looks solid overall — the telemetry-forwarding logic, the telemetryEnabled env/option resolution, and the !useKernel gate that suppresses duplicate Node-wrapper telemetry are all correct and well-tested. Two low-severity notes: a comment in buildKernelTelemetryOptions mislabels telemetryBackoffBaseMs as a user-settable ConnectionOptions knob (it isn't exposed), and the non-positive-value fallbacks silently drop caller input without the log line the sibling env-kill path emits.

[peco-review-bot]

Verdict: 1 Medium

Mostly solid — telemetry forwarding, the !useKernel init gate, and the shared isTelemetryDisabledByEnv helper all look correct and are well-tested. One medium concern: because DBSQLClient.getDefaultConfig() always seeds the telemetry tuning knobs, the kernel's "Omitted ⇒ kernel default" path is unreachable in production and the Node defaults silently shadow the kernel's — worth confirming intent and reconciling the comments/tests.

[peco-review-bot]

Verdict: 1 Low

Looks good — clean, well-documented telemetry-forwarding change. Verified config field names against ClientConfig/DEFAULT_TELEMETRY_CONFIG, napi target names against native/kernel/index.d.ts, and the > 0 vs >= 0 guards against the documented napi constraints; the telemetryEnabled/env-disable logic and its new tests are correct. One low: the new warnRejected out-of-range branch has no test coverage.

[peco-review-bot]

Verdict: 1 Low

Looks good — well-scoped and thoroughly tested. Field names match the napi binding, the isTelemetryDisabledByEnv refactor is behavior-preserving, and the !useKernel telemetry gate plus env kill-switch are correct with solid unit coverage. One Low: the circuit-breaker threshold/timeout are forwarded but telemetryCircuitBreakerEnabled is never set, so those knobs may be inert depending on the kernel default (F1).

[peco-review-bot]

Verdict: 1 Low

Looks good — a clean, well-tested telemetry-forwarding change with one trivial low note. I verified every forwarded field against the napi ConnectionOptions contract and every ClientConfig field in the Pick, confirmed the !useKernel telemetry gate + env-disable precedence, and checked the >0/>=0 guard logic against the new tests — all consistent. The only nit is a redundant re-read of DATABRICKS_TELEMETRY_DISABLED in DBSQLClient.connect (raised inline as F1).

[peco-review-bot]

Verdict: 1 Low

Looks good — a clean, well-tested change. The telemetry knob forwarding is correct: all ClientConfig knobs are seeded from DEFAULT_TELEMETRY_CONFIG in getDefaultConfig(), copyDefinedTelemetryOptions populates config before backend.connect() reads it, forwarded napi field names match the generated native/kernel/index.d.ts contract, the > 0 / >= 0 guards + warnRejected behave as documented, and the telemetryEnabled env-kill logic is correct across all four cases. One low-severity cross-path behavior note is inline. Nit: the package-lock.json patch bumps (js-yaml, brace-expansion) are unrelated to the telemetry work and appear to be incidental rebase/install churn — worth confirming they're intended for this PR.

[peco-review-bot]

🔵 Low — This reorders the locale env-var precedence from LANG > LC_ALL > LC_MESSAGES to POSIX LC_ALL > LC_MESSAGES > LANG. This is DBSQLClient.getLocaleName(), which feeds telemetry DriverConfiguration.localeName on the Thrift path (kernel telemetry is disabled in the wrapper by this same PR). So beyond the stated "forward kernel telemetry options" scope, this silently changes the reported localeName for existing Thrift users who have LANG set to a different locale than LC_ALL/LC_MESSAGES. The change is defensible (POSIX precedence is arguably more correct, and it makes the two backends report the same value), but it is a behavior change to a shipping path that isn't called out in the PR description. Flagging so reviewers are aware the impact isn't kernel-only.

[peco-engineer-bot]

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

The comment is an informational flag ("Flagging so reviewers are aware"), not a request for a code change. The precedence reorder to POSIX (LC_ALL > LC_MESSAGES > LANG) in getLocaleName() is intentional — it aligns the Thrift path with the kernel path's getLocaleName so telemetry localeName is backend-invariant, as documented in the inline comment. The reviewer agrees this is defensible; the only open item is a human judgment call about accepting a behavior change to the shipping Thrift telemetry path (localeName for users whose LANG differs from LC_ALL/LC_MESSAGES) that is outside the PR's stated scope and not noted in the PR description. That needs a maintainer's decision on scope/PR-description, which cannot be actioned as a code edit in this file — escalating for human review.