Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions lib/DBSQLClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import TelemetryClientProvider from './telemetry/TelemetryClientProvider';
import TelemetryEventEmitter from './telemetry/TelemetryEventEmitter';
import MetricsAggregator from './telemetry/MetricsAggregator';
import { DriverConfiguration, DRIVER_NAME, TelemetryEventType, DEFAULT_TELEMETRY_CONFIG } from './telemetry/types';
import { safeEmit } from './telemetry/telemetryUtils';
import { safeEmit, isTelemetryDisabledByEnv } from './telemetry/telemetryUtils';
import driverVersion from './version';

function prependSlash(str: string): string {
Expand Down Expand Up @@ -521,8 +521,10 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
*/
private getLocaleName(): string {
try {
// Try to get from environment variables
const lang = process.env.LANG || process.env.LC_ALL || process.env.LC_MESSAGES || '';
// Try to get from environment variables. Use POSIX precedence
// (LC_ALL > LC_MESSAGES > LANG) so this matches the kernel path's
// getLocaleName and telemetry localeName stays backend-invariant.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const lang = process.env.LC_ALL || process.env.LC_MESSAGES || process.env.LANG || '';
if (lang) {
// LANG format is typically "en_US.UTF-8", extract "en_US"
const match = lang.match(/^([a-z]{2}_[A-Z]{2})/);
Expand Down Expand Up @@ -729,7 +731,8 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
// doesn't ship in the public `.d.ts`. Mirrors Python's `kwargs.get("use_kernel")`
// pattern (see databricks-sql-python/src/databricks/sql/session.py).
const internalOptions = options as ConnectionOptions & InternalConnectionOptions;
const backend = internalOptions.useKernel
const useKernel = internalOptions.useKernel === true;
const backend = useKernel
? new KernelBackend({ context: this })
: new ThriftBackend({
context: this,
Expand Down Expand Up @@ -763,7 +766,7 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
// expecting to enable telemetry.
const envKill = process.env.DATABRICKS_TELEMETRY_DISABLED;
const trimmedEnvKill = typeof envKill === 'string' ? envKill.trim() : '';
const envDisabled = trimmedEnvKill.length > 0 && /^(1|true|yes|on)$/i.test(trimmedEnvKill);
const envDisabled = isTelemetryDisabledByEnv();
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Outdated
// Surface the misconfiguration: an ops engineer who sees the var name and
// tries to "set it to false to keep telemetry on" otherwise gets the
// opposite of what they expect (the var is then silently ignored, runtime
Expand All @@ -777,7 +780,7 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
`Telemetry remains controlled by the runtime config and feature flag.`,
);
}
if (this.config.telemetryEnabled && !envDisabled) {
if (!useKernel && this.config.telemetryEnabled && !envDisabled) {
await this.initializeTelemetry();
}

Expand Down
191 changes: 191 additions & 0 deletions lib/kernel/KernelAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,17 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import os from 'os';
import { ConnectionOptions } from '../contracts/IDBSQLClient';
import { ClientConfig } from '../contracts/IClientContext';
import { InternalConnectionOptions } from '../contracts/InternalConnectionOptions';
import IDBSQLLogger, { LogLevel } from '../contracts/IDBSQLLogger';
import AuthenticationError from '../errors/AuthenticationError';
import HiveDriverError from '../errors/HiveDriverError';
import { buildUserAgentString, normalizePemBytes } from '../utils';
import driverVersion from '../version';
import { DRIVER_NAME } from '../telemetry/types';
import { sanitizeProcessName, isTelemetryDisabledByEnv } from '../telemetry/telemetryUtils';

/**
* Default local listener port for the U2M authorization-code callback.
Expand Down Expand Up @@ -131,6 +137,32 @@ export interface KernelSessionDefaults {
retryOverallTimeoutSecs?: number;
}

export interface KernelTelemetryOptions {
/** Driver/runtime identity forwarded to kernel-owned telemetry. */
driverName?: string;
driverVersion?: string;
runtimeName?: string;
runtimeVersion?: string;
runtimeVendor?: string;
osName?: string;
osVersion?: string;
osArch?: string;
clientAppName?: string;
localeName?: string;
charSetEncoding?: string;
processName?: string;
/** Kernel-owned telemetry switch and batching. */
telemetryEnabled?: boolean;
telemetryBatchSize?: number;
telemetryFlushIntervalMs?: number;
telemetryMaxRetries?: number;
telemetryRetryDelayMs?: number;
telemetryCloseFlushTimeoutMs?: number;
telemetryCircuitBreakerEnabled?: boolean;
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
telemetryCircuitBreakerThreshold?: number;
telemetryCircuitBreakerTimeoutMs?: number;
}

/**
* TLS options shared across all auth-mode variants. Mirror the napi
* binding's `ConnectionOptions.checkServerCertificate` / `.customCaCert`
Expand Down Expand Up @@ -227,6 +259,7 @@ export interface KernelFederationOptions {
export type KernelNativeConnectionOptions = KernelSessionDefaults &
KernelTlsOptions &
KernelHttpOptions &
KernelTelemetryOptions &
KernelProxyOptions &
KernelFederationOptions &
(
Expand Down Expand Up @@ -588,6 +621,164 @@ export function buildKernelRetryOptions(config: {
return out;
}

function getLocaleName(env: NodeJS.ProcessEnv = process.env): string {
try {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
const lang = env.LC_ALL || env.LC_MESSAGES || env.LANG || '';
const match = lang.match(/^([a-z]{2}_[A-Z]{2})/);
return match?.[1] ?? 'en_US';
} catch {
return 'en_US';
}
}

function getProcessName(): string {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
try {
if (process.title && process.title !== 'node') {
return sanitizeProcessName(process.title) || 'node';
}
const scriptPath = process.argv?.[1];
if (scriptPath) {
return sanitizeProcessName(scriptPath).replace(/\.[^.]*$/, '') || 'node';
}
return 'node';
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
} catch {
return 'node';
}
}

// Re-exported from the shared telemetry helper so the kernel opt-out and the
// Thrift-path opt-out (DBSQLClient) parse `DATABRICKS_TELEMETRY_DISABLED`
// through one implementation and can never drift.
export { isTelemetryDisabledByEnv };

/**
* Build the kernel telemetry options block from the driver's `ClientConfig`.
*
* **Always-forward is intentional, mirroring `buildKernelRetryOptions`.** On the
* real `DBSQLClient` path `getDefaultConfig()` seeds every one of these knobs from
* `DEFAULT_TELEMETRY_CONFIG`, so they are never `undefined` and always propagate to
* the kernel's `openSession` — the driver's telemetry-tuning defaults deliberately
* govern both backends from one `ClientConfig` (same rationale as the retry knobs),
* rather than letting the kernel's independent defaults apply. The kernel's own
* `"Omitted ⇒ kernel default"` branch is therefore only reachable via a bare
* `configOverrides`-style config (e.g. unit tests), not the default-populated one a
* live client uses. The `Number.isFinite(...) && > 0` guards below are NOT an
* opt-in gate: they exist to reject a caller-supplied out-of-range value (warning
* via `warnRejected`) and to tolerate sparse test configs, not to compare against
* the default.
*/
export function buildKernelTelemetryOptions(
config: Pick<
ClientConfig,
// NOTE: `telemetryEnabled` is intentionally NOT in this Pick. On the kernel
// path the enable decision is opt-in via `ConnectionOptions.telemetryEnabled`
// (plus the `DATABRICKS_TELEMETRY_DISABLED` env kill-switch) — the driver's
// default-true `config.telemetryEnabled` does not propagate here, so leaving
// it out keeps the signature honest rather than advertising a knob we ignore.
| 'telemetryBatchSize'
| 'telemetryFlushIntervalMs'
| 'telemetryMaxRetries'
| 'telemetryBackoffBaseMs'
| 'telemetryCloseTimeoutMs'
| 'telemetryCircuitBreakerThreshold'
| 'telemetryCircuitBreakerTimeout'
>,
options: Pick<ConnectionOptions, 'telemetryEnabled'> = {},
logger?: IDBSQLLogger,
) {
// Surface a rejected telemetry knob for parity with the `DATABRICKS_TELEMETRY_DISABLED`
// misconfiguration warn in `DBSQLClient.connect`: a caller-supplied out-of-range value
// (e.g. `telemetryBatchSize: 0`) is silently dropped in favour of the kernel default,
// so without this the user gets no feedback that their setting was discarded. Only
// warns when the knob was actually supplied (`Number.isFinite`) but out of range —
// an unset knob (`undefined`) is never a misconfiguration. On the live `DBSQLClient`
// path these knobs are always populated from `DEFAULT_TELEMETRY_CONFIG`, so the
// `undefined` branch is the sparse-config (e.g. unit-test) case, not the norm.
const warnRejected = (name: string, value: number | undefined, constraint: string) => {
if (Number.isFinite(value)) {
logger?.log(
LogLevel.warn,
`Ignoring telemetry option '${name}'=${value}: value must be ${constraint}. ` +
`Falling back to the kernel default.`,
);
}
};
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
const telemetry: KernelTelemetryOptions = {
driverName: DRIVER_NAME,
driverVersion,
runtimeName: 'Node.js',
runtimeVersion: process.version,
runtimeVendor: 'Node.js Foundation',
osName: process.platform,
osVersion: os.release(),
osArch: os.arch(),
clientAppName: undefined,
localeName: getLocaleName(),
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
charSetEncoding: 'UTF-8',
processName: getProcessName(),
};

const envDisabled = isTelemetryDisabledByEnv();
if (options.telemetryEnabled !== undefined || envDisabled) {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
telemetry.telemetryEnabled = (options.telemetryEnabled ?? true) && !envDisabled;
}

// `batchSize`, `flushIntervalMs`, and `closeFlushTimeoutMs` share the same napi
// contract constraint as the breaker fields below (`Must be greater than zero when
// supplied`), so a caller-supplied `0`/negative would forward verbatim and surface
// as a hard kernel `openSession` rejection. Treat any non-positive value as a
// misconfiguration and fall back to the kernel defaults, matching the breaker guard.
if (Number.isFinite(config.telemetryBatchSize) && config.telemetryBatchSize! > 0) {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
telemetry.telemetryBatchSize = config.telemetryBatchSize;
} else {
warnRejected('telemetryBatchSize', config.telemetryBatchSize, 'greater than zero');
}
if (Number.isFinite(config.telemetryFlushIntervalMs) && config.telemetryFlushIntervalMs! > 0) {
telemetry.telemetryFlushIntervalMs = config.telemetryFlushIntervalMs;
} else {
warnRejected('telemetryFlushIntervalMs', config.telemetryFlushIntervalMs, 'greater than zero');
}
// `telemetryMaxRetries` and `telemetryRetryDelayMs` (from `telemetryBackoffBaseMs`)
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
// both document `0` as valid, so we don't require `> 0` like the fields above. Only
// `telemetryMaxRetries` is a user-settable `ConnectionOptions` knob (copied by
// `copyDefinedTelemetryOptions`); `telemetryBackoffBaseMs` is internal and only ever
// arrives from `DEFAULT_TELEMETRY_CONFIG.backoffBaseMs`, so it can't be user-negative
// today. We still guard both `>= 0` uniformly: a negative mapped onto the kernel's
// unsigned retry count would be rejected or wrap, so `>= 0` keeps `0` valid while
// falling back to the kernel default for negatives.
if (Number.isFinite(config.telemetryMaxRetries) && config.telemetryMaxRetries! >= 0) {
telemetry.telemetryMaxRetries = config.telemetryMaxRetries;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
warnRejected('telemetryMaxRetries', config.telemetryMaxRetries, 'zero or greater');
}
if (Number.isFinite(config.telemetryBackoffBaseMs) && config.telemetryBackoffBaseMs! >= 0) {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
telemetry.telemetryRetryDelayMs = config.telemetryBackoffBaseMs;
} else {
warnRejected('telemetryBackoffBaseMs', config.telemetryBackoffBaseMs, 'zero or greater');
}
if (Number.isFinite(config.telemetryCloseTimeoutMs) && config.telemetryCloseTimeoutMs! > 0) {
telemetry.telemetryCloseFlushTimeoutMs = config.telemetryCloseTimeoutMs;
} else {
warnRejected('telemetryCloseTimeoutMs', config.telemetryCloseTimeoutMs, 'greater than zero');
}
// The napi contract requires threshold/timeout to be strictly positive when
// supplied. A caller-supplied `0` (or negative) would otherwise be forwarded
// verbatim and surface as a hard kernel `openSession` rejection, so treat any
// non-positive value as a misconfiguration and delegate to the kernel default.
if (Number.isFinite(config.telemetryCircuitBreakerThreshold) && config.telemetryCircuitBreakerThreshold! > 0) {
telemetry.telemetryCircuitBreakerThreshold = config.telemetryCircuitBreakerThreshold;
} else {
warnRejected('telemetryCircuitBreakerThreshold', config.telemetryCircuitBreakerThreshold, 'greater than zero');
}
if (Number.isFinite(config.telemetryCircuitBreakerTimeout) && config.telemetryCircuitBreakerTimeout! > 0) {
telemetry.telemetryCircuitBreakerTimeoutMs = config.telemetryCircuitBreakerTimeout;
} else {
warnRejected('telemetryCircuitBreakerTimeout', config.telemetryCircuitBreakerTimeout, 'greater than zero');
}

return telemetry;
}

/**
* Map the public `ConnectionOptions.proxy` (`{protocol, host, port, auth}` —
* the same shape the Thrift backend accepts) onto the kernel's structured napi
Expand Down
8 changes: 7 additions & 1 deletion lib/kernel/KernelBackend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ import HiveDriverError from '../errors/HiveDriverError';
import { serializeQueryTags } from '../utils';
import { getKernelNative, KernelNativeBinding, KernelConnection } from './KernelNativeLoader';
import { decodeNapiKernelError } from './KernelErrorMapping';
import { buildKernelConnectionOptions, buildKernelRetryOptions, KernelNativeConnectionOptions } from './KernelAuth';
import {
buildKernelConnectionOptions,
buildKernelRetryOptions,
buildKernelTelemetryOptions,
KernelNativeConnectionOptions,
} from './KernelAuth';
import { installKernelLogBridge } from './KernelLogging';
import KernelSessionBackend from './KernelSessionBackend';

Expand Down Expand Up @@ -93,6 +98,7 @@ export default class KernelBackend implements IBackend {
this.nativeOptions = {
...buildKernelConnectionOptions(options),
...buildKernelRetryOptions(this.context.getConfig()),
...buildKernelTelemetryOptions(this.context.getConfig(), options, this.context.getLogger()),
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
};

// Bridge the Rust kernel's `tracing` logs into the SAME `DBSQLLogger` the
Expand Down
15 changes: 15 additions & 0 deletions lib/telemetry/telemetryUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,21 @@ export function sanitizeProcessName(name: string | undefined): string {
return lastSep < 0 ? firstToken : firstToken.slice(lastSep + 1);
}

/**
* Parse the `DATABRICKS_TELEMETRY_DISABLED` hard kill switch. Recognized truthy
* values are `1`, `true`, `yes`, `on` (case-insensitive, surrounding whitespace
* trimmed); anything else (empty, `0`, `false`, `no`, `off`, unrecognized)
* returns `false` and leaves telemetry under the runtime config's control.
*
* Single source of truth for both the Thrift path (DBSQLClient) and the kernel
* path (KernelAuth) so the two opt-outs can never drift.
*/
export function isTelemetryDisabledByEnv(env: NodeJS.ProcessEnv = process.env): boolean {
const raw = env.DATABRICKS_TELEMETRY_DISABLED;
const trimmed = typeof raw === 'string' ? raw.trim() : '';
return trimmed.length > 0 && /^(1|true|yes|on)$/i.test(trimmed);
}

/**
* Run a telemetry emit at a call site, swallowing all exceptions and logging
* at debug level. Replaces the copy-pasted try/catch + getTelemetryEmitter?.()
Expand Down
14 changes: 14 additions & 0 deletions native/kernel/index.d.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions tests/unit/DBSQLClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import fs from 'fs';
import DBSQLClient, { ThriftLibrary } from '../../lib/DBSQLClient';
import DBSQLSession from '../../lib/DBSQLSession';
import ThriftBackend from '../../lib/thrift-backend/ThriftBackend';
import KernelBackend from '../../lib/kernel/KernelBackend';
import * as KernelNativeLoader from '../../lib/kernel/KernelNativeLoader';

import PlainHttpAuthentication from '../../lib/connection/auth/PlainHttpAuthentication';
import DatabricksOAuth from '../../lib/connection/auth/DatabricksOAuth';
Expand Down Expand Up @@ -957,6 +959,29 @@ describe('DBSQLClient telemetry paths', () => {
.filter((c) => c.args[0] === LogLevel.warn && /DATABRICKS_TELEMETRY_DISABLED/.test(c.args[1] as string));
expect(warnCalls.length).to.equal(0);
});

it('does not initialize Node telemetry on the kernel path', async () => {
delete process.env.DATABRICKS_TELEMETRY_DISABLED;
const client = new DBSQLClient();
const initStub = sinon.stub(client as any, 'initializeTelemetry').resolves();
// DBSQLClient constructs `new KernelBackend({ context: this })` without
// injecting a `nativeBinding`, so the KernelBackend constructor calls
// `getKernelNative()`, which throws where the native `.node` artifact
// isn't built (e.g. CI). Stub the loader so construction succeeds and the
// assertion below tests the `!useKernel` telemetry gate rather than the
// presence of a built kernel artifact.
sinon.stub(KernelNativeLoader, 'getKernelNative').returns({} as any);
sinon.stub(KernelBackend.prototype, 'connect').resolves();
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
sinon.stub(KernelBackend.prototype, 'close').resolves();

try {
await client.connect({ ...connectOptions, telemetryEnabled: true, useKernel: true } as any);

expect(initStub.callCount).to.equal(0);
} finally {
await client.close();
}
});
});

describe('extractWorkspaceId', () => {
Expand Down
Loading
Loading