Skip to content

Commit d3585fc

Browse files
samikshya-dbclaude
andcommitted
Fix telemetry and feature flag implementation
- Fix event listener names: use 'connection.open' not 'telemetry.connection.open' - Fix feature flag endpoint: use NODEJS client type instead of OSS_NODEJS - Fix telemetry endpoints: use /telemetry-ext and /telemetry-unauth (not /api/2.0/sql/...) - Update telemetry payload to match proto: use system_configuration with snake_case fields - Add URL utility to handle hosts with or without protocol - Add telemetryBatchSize and telemetryAuthenticatedExport config options - Remove debug statements and temporary feature flag override Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 7496433 commit d3585fc

5 files changed

Lines changed: 83 additions & 20 deletions

File tree

lib/DBSQLClient.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
236236

237237
// Check if telemetry enabled via feature flag
238238
const enabled = await this.featureFlagCache.isTelemetryEnabled(this.host);
239+
239240
if (!enabled) {
240241
this.logger.log(LogLevel.debug, 'Telemetry disabled via feature flag');
241242
return;
@@ -254,39 +255,39 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
254255
this.telemetryAggregator = new MetricsAggregator(this, exporter);
255256

256257
// Wire up event listeners
257-
this.telemetryEmitter.on('telemetry.connection.open', (event) => {
258+
this.telemetryEmitter.on('connection.open', (event) => {
258259
try {
259260
this.telemetryAggregator?.processEvent(event);
260261
} catch (error: any) {
261262
this.logger.log(LogLevel.debug, `Error processing connection.open event: ${error.message}`);
262263
}
263264
});
264265

265-
this.telemetryEmitter.on('telemetry.statement.start', (event) => {
266+
this.telemetryEmitter.on('statement.start', (event) => {
266267
try {
267268
this.telemetryAggregator?.processEvent(event);
268269
} catch (error: any) {
269270
this.logger.log(LogLevel.debug, `Error processing statement.start event: ${error.message}`);
270271
}
271272
});
272273

273-
this.telemetryEmitter.on('telemetry.statement.complete', (event) => {
274+
this.telemetryEmitter.on('statement.complete', (event) => {
274275
try {
275276
this.telemetryAggregator?.processEvent(event);
276277
} catch (error: any) {
277278
this.logger.log(LogLevel.debug, `Error processing statement.complete event: ${error.message}`);
278279
}
279280
});
280281

281-
this.telemetryEmitter.on('telemetry.cloudfetch.chunk', (event) => {
282+
this.telemetryEmitter.on('cloudfetch.chunk', (event) => {
282283
try {
283284
this.telemetryAggregator?.processEvent(event);
284285
} catch (error: any) {
285286
this.logger.log(LogLevel.debug, `Error processing cloudfetch.chunk event: ${error.message}`);
286287
}
287288
});
288289

289-
this.telemetryEmitter.on('telemetry.error', (event) => {
290+
this.telemetryEmitter.on('error', (event) => {
290291
try {
291292
this.telemetryAggregator?.processEvent(event);
292293
} catch (error: any) {
@@ -334,6 +335,12 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
334335
if (options.telemetryEnabled !== undefined) {
335336
this.config.telemetryEnabled = options.telemetryEnabled;
336337
}
338+
if (options.telemetryBatchSize !== undefined) {
339+
this.config.telemetryBatchSize = options.telemetryBatchSize;
340+
}
341+
if (options.telemetryAuthenticatedExport !== undefined) {
342+
this.config.telemetryAuthenticatedExport = options.telemetryAuthenticatedExport;
343+
}
337344

338345
this.authProvider = this.createAuthProvider(options, authProvider);
339346

lib/contracts/IDBSQLClient.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,10 @@ export type ConnectionOptions = {
3434
socketTimeout?: number;
3535
proxy?: ProxyOptions;
3636
enableMetricViewMetadata?: boolean;
37-
// Optional telemetry override
37+
// Optional telemetry overrides
3838
telemetryEnabled?: boolean;
39+
telemetryBatchSize?: number;
40+
telemetryAuthenticatedExport?: boolean;
3941
} & AuthOptions;
4042

4143
export interface OpenSessionRequest {

lib/telemetry/DatabricksTelemetryExporter.ts

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import { LogLevel } from '../contracts/IDBSQLLogger';
2020
import { TelemetryMetric, DEFAULT_TELEMETRY_CONFIG } from './types';
2121
import { CircuitBreakerRegistry } from './CircuitBreaker';
2222
import ExceptionClassifier from './ExceptionClassifier';
23+
import { buildUrl } from './urlUtils';
2324

2425
/**
2526
* Databricks telemetry log format for export.
@@ -37,19 +38,33 @@ interface DatabricksTelemetryLog {
3738
sql_driver_log: {
3839
session_id?: string;
3940
sql_statement_id?: string;
41+
system_configuration?: {
42+
driver_version?: string;
43+
runtime_name?: string;
44+
runtime_version?: string;
45+
runtime_vendor?: string;
46+
os_name?: string;
47+
os_version?: string;
48+
os_arch?: string;
49+
driver_name?: string;
50+
client_app_name?: string;
51+
};
52+
driver_connection_params?: any;
4053
operation_latency_ms?: number;
4154
sql_operation?: {
42-
execution_result_format?: string;
55+
execution_result?: string;
4356
chunk_details?: {
44-
chunk_count: number;
45-
total_bytes?: number;
57+
total_chunks_present?: number;
58+
total_chunks_iterated?: number;
59+
initial_chunk_latency_millis?: number;
60+
slowest_chunk_latency_millis?: number;
61+
sum_chunks_download_time_millis?: number;
4662
};
4763
};
4864
error_info?: {
4965
error_name: string;
5066
stack_trace: string;
5167
};
52-
driver_config?: any;
5368
};
5469
};
5570
}
@@ -190,8 +205,8 @@ export default class DatabricksTelemetryExporter {
190205
const authenticatedExport =
191206
config.telemetryAuthenticatedExport ?? DEFAULT_TELEMETRY_CONFIG.authenticatedExport;
192207
const endpoint = authenticatedExport
193-
? `https://${this.host}/api/2.0/sql/telemetry-ext`
194-
: `https://${this.host}/api/2.0/sql/telemetry-unauth`;
208+
? buildUrl(this.host, '/telemetry-ext')
209+
: buildUrl(this.host, '/telemetry-unauth');
195210

196211
// Format payload
197212
const payload: DatabricksTelemetryPayload = {
@@ -231,7 +246,7 @@ export default class DatabricksTelemetryExporter {
231246
*/
232247
private toTelemetryLog(metric: TelemetryMetric): DatabricksTelemetryLog {
233248
const log: DatabricksTelemetryLog = {
234-
workspace_id: metric.workspaceId,
249+
// workspace_id: metric.workspaceId, // TODO: Determine if this should be numeric or omitted
235250
frontend_log_event_id: this.generateUUID(),
236251
context: {
237252
client_context: {
@@ -247,21 +262,29 @@ export default class DatabricksTelemetryExporter {
247262
},
248263
};
249264

250-
// Add metric-specific fields
265+
// Add metric-specific fields based on proto definition
251266
if (metric.metricType === 'connection' && metric.driverConfig) {
252-
log.entry.sql_driver_log.driver_config = metric.driverConfig;
267+
// Map driverConfig to system_configuration (snake_case as per proto)
268+
log.entry.sql_driver_log.system_configuration = {
269+
driver_version: metric.driverConfig.driverVersion,
270+
driver_name: metric.driverConfig.driverName,
271+
runtime_name: 'Node.js',
272+
runtime_version: metric.driverConfig.nodeVersion,
273+
os_name: metric.driverConfig.platform,
274+
os_version: metric.driverConfig.osVersion,
275+
};
253276
} else if (metric.metricType === 'statement') {
254277
log.entry.sql_driver_log.operation_latency_ms = metric.latencyMs;
255278

256279
if (metric.resultFormat || metric.chunkCount) {
257280
log.entry.sql_driver_log.sql_operation = {
258-
execution_result_format: metric.resultFormat,
281+
execution_result: metric.resultFormat,
259282
};
260283

261284
if (metric.chunkCount && metric.chunkCount > 0) {
262285
log.entry.sql_driver_log.sql_operation.chunk_details = {
263-
chunk_count: metric.chunkCount,
264-
total_bytes: metric.bytesDownloaded,
286+
total_chunks_present: metric.chunkCount,
287+
total_chunks_iterated: metric.chunkCount,
265288
};
266289
}
267290
}

lib/telemetry/FeatureFlagCache.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import IClientContext from '../contracts/IClientContext';
1818
import { LogLevel } from '../contracts/IDBSQLLogger';
1919
import fetch from 'node-fetch';
20+
import { buildUrl } from './urlUtils';
2021

2122
/**
2223
* Context holding feature flag state for a specific host.
@@ -118,8 +119,8 @@ export default class FeatureFlagCache {
118119
// Get driver version for endpoint
119120
const driverVersion = this.getDriverVersion();
120121

121-
// Build feature flags endpoint
122-
const endpoint = `https://${host}/api/2.0/connector-service/feature-flags/OSS_NODEJS/${driverVersion}`;
122+
// Build feature flags endpoint for Node.js driver
123+
const endpoint = buildUrl(host, `/api/2.0/connector-service/feature-flags/NODEJS/${driverVersion}`);
123124

124125
// Get authentication headers
125126
const authHeaders = await this.context.getAuthHeaders();

lib/telemetry/urlUtils.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/**
2+
* Copyright (c) 2025 Databricks Contributors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
/**
18+
* Build full URL from host and path, handling protocol correctly.
19+
* @param host The hostname (with or without protocol)
20+
* @param path The path to append (should start with /)
21+
* @returns Full URL with protocol
22+
*/
23+
export function buildUrl(host: string, path: string): string {
24+
// Check if host already has protocol
25+
if (host.startsWith('http://') || host.startsWith('https://')) {
26+
return `${host}${path}`;
27+
}
28+
// Add https:// if no protocol present
29+
return `https://${host}${path}`;
30+
}

0 commit comments

Comments
 (0)