Skip to content

Commit c254d95

Browse files
ai: apply changes for #506 (2 review threads)
Addresses: - #3877181283 at lib/kernel/KernelAuth.ts:702 - #3877181291 at lib/kernel/KernelAuth.ts:696 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent 7f512ca commit c254d95

2 files changed

Lines changed: 39 additions & 5 deletions

File tree

lib/kernel/KernelAuth.ts

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import os from 'os';
1616
import { ConnectionOptions } from '../contracts/IDBSQLClient';
1717
import { ClientConfig } from '../contracts/IClientContext';
1818
import { InternalConnectionOptions } from '../contracts/InternalConnectionOptions';
19+
import IDBSQLLogger, { LogLevel } from '../contracts/IDBSQLLogger';
1920
import AuthenticationError from '../errors/AuthenticationError';
2021
import HiveDriverError from '../errors/HiveDriverError';
2122
import { buildUserAgentString, normalizePemBytes } from '../utils';
@@ -667,7 +668,23 @@ export function buildKernelTelemetryOptions(
667668
| 'telemetryCircuitBreakerTimeout'
668669
>,
669670
options: Pick<ConnectionOptions, 'telemetryEnabled'> = {},
671+
logger?: IDBSQLLogger,
670672
) {
673+
// Surface a rejected telemetry knob for parity with the `DATABRICKS_TELEMETRY_DISABLED`
674+
// misconfiguration warn in `DBSQLClient.connect`: a caller-supplied out-of-range value
675+
// (e.g. `telemetryBatchSize: 0`) is silently dropped in favour of the kernel default,
676+
// so without this the user gets no feedback that their setting was discarded. Only
677+
// warns when the knob was actually supplied (`Number.isFinite`) but out of range —
678+
// an unset knob (`undefined`) is the normal case, not a misconfiguration.
679+
const warnRejected = (name: string, value: number | undefined, constraint: string) => {
680+
if (Number.isFinite(value)) {
681+
logger?.log(
682+
LogLevel.warn,
683+
`Ignoring telemetry option '${name}'=${value}: value must be ${constraint}. ` +
684+
`Falling back to the kernel default.`,
685+
);
686+
}
687+
};
671688
const telemetry: KernelTelemetryOptions = {
672689
driverName: DRIVER_NAME,
673690
driverVersion,
@@ -695,33 +712,50 @@ export function buildKernelTelemetryOptions(
695712
// misconfiguration and fall back to the kernel defaults, matching the breaker guard.
696713
if (Number.isFinite(config.telemetryBatchSize) && config.telemetryBatchSize! > 0) {
697714
telemetry.telemetryBatchSize = config.telemetryBatchSize;
715+
} else {
716+
warnRejected('telemetryBatchSize', config.telemetryBatchSize, 'greater than zero');
698717
}
699718
if (Number.isFinite(config.telemetryFlushIntervalMs) && config.telemetryFlushIntervalMs! > 0) {
700719
telemetry.telemetryFlushIntervalMs = config.telemetryFlushIntervalMs;
720+
} else {
721+
warnRejected('telemetryFlushIntervalMs', config.telemetryFlushIntervalMs, 'greater than zero');
701722
}
702723
// `telemetryMaxRetries` and `telemetryRetryDelayMs` (from `telemetryBackoffBaseMs`)
703-
// both document `0` as valid, so we don't require `> 0` like the fields above. But
704-
// both are user-settable `ConnectionOptions` knobs, and a negative value mapped onto
705-
// the kernel's unsigned retry count would be rejected or wrap — so guard `>= 0` to
706-
// keep `0` valid while falling back to the kernel default for negatives.
724+
// both document `0` as valid, so we don't require `> 0` like the fields above. Only
725+
// `telemetryMaxRetries` is a user-settable `ConnectionOptions` knob (copied by
726+
// `copyDefinedTelemetryOptions`); `telemetryBackoffBaseMs` is internal and only ever
727+
// arrives from `DEFAULT_TELEMETRY_CONFIG.backoffBaseMs`, so it can't be user-negative
728+
// today. We still guard both `>= 0` uniformly: a negative mapped onto the kernel's
729+
// unsigned retry count would be rejected or wrap, so `>= 0` keeps `0` valid while
730+
// falling back to the kernel default for negatives.
707731
if (Number.isFinite(config.telemetryMaxRetries) && config.telemetryMaxRetries! >= 0) {
708732
telemetry.telemetryMaxRetries = config.telemetryMaxRetries;
733+
} else {
734+
warnRejected('telemetryMaxRetries', config.telemetryMaxRetries, 'zero or greater');
709735
}
710736
if (Number.isFinite(config.telemetryBackoffBaseMs) && config.telemetryBackoffBaseMs! >= 0) {
711737
telemetry.telemetryRetryDelayMs = config.telemetryBackoffBaseMs;
738+
} else {
739+
warnRejected('telemetryBackoffBaseMs', config.telemetryBackoffBaseMs, 'zero or greater');
712740
}
713741
if (Number.isFinite(config.telemetryCloseTimeoutMs) && config.telemetryCloseTimeoutMs! > 0) {
714742
telemetry.telemetryCloseFlushTimeoutMs = config.telemetryCloseTimeoutMs;
743+
} else {
744+
warnRejected('telemetryCloseTimeoutMs', config.telemetryCloseTimeoutMs, 'greater than zero');
715745
}
716746
// The napi contract requires threshold/timeout to be strictly positive when
717747
// supplied. A caller-supplied `0` (or negative) would otherwise be forwarded
718748
// verbatim and surface as a hard kernel `openSession` rejection, so treat any
719749
// non-positive value as a misconfiguration and delegate to the kernel default.
720750
if (Number.isFinite(config.telemetryCircuitBreakerThreshold) && config.telemetryCircuitBreakerThreshold! > 0) {
721751
telemetry.telemetryCircuitBreakerThreshold = config.telemetryCircuitBreakerThreshold;
752+
} else {
753+
warnRejected('telemetryCircuitBreakerThreshold', config.telemetryCircuitBreakerThreshold, 'greater than zero');
722754
}
723755
if (Number.isFinite(config.telemetryCircuitBreakerTimeout) && config.telemetryCircuitBreakerTimeout! > 0) {
724756
telemetry.telemetryCircuitBreakerTimeoutMs = config.telemetryCircuitBreakerTimeout;
757+
} else {
758+
warnRejected('telemetryCircuitBreakerTimeout', config.telemetryCircuitBreakerTimeout, 'greater than zero');
725759
}
726760

727761
return telemetry;

lib/kernel/KernelBackend.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ export default class KernelBackend implements IBackend {
9898
this.nativeOptions = {
9999
...buildKernelConnectionOptions(options),
100100
...buildKernelRetryOptions(this.context.getConfig()),
101-
...buildKernelTelemetryOptions(this.context.getConfig(), options),
101+
...buildKernelTelemetryOptions(this.context.getConfig(), options, this.context.getLogger()),
102102
};
103103

104104
// Bridge the Rust kernel's `tracing` logs into the SAME `DBSQLLogger` the

0 commit comments

Comments
 (0)