Skip to content

Commit 42540bc

Browse files
samikshya-dbclaude
andcommitted
Fix telemetry PR review comments from #325
Three fixes addressing review feedback: 1. Fix documentation typo (sreekanth-db comment) - DatabricksTelemetryExporter.ts:94 - Changed "TelemetryFrontendLog" to "DatabricksTelemetryLog" 2. Add proxy support (jadewang-db comment) - DatabricksTelemetryExporter.ts:exportInternal() - Get HTTP agent from connection provider - Pass agent to fetch for proxy support - Follows same pattern as CloudFetchResultHandler and DBSQLSession - Supports http/https/socks proxies with authentication 3. Fix flush timer to prevent rate limiting (sreekanth-db comment) - MetricsAggregator.ts:flush() - Reset timer after manual flushes (batch size, terminal errors) - Ensures consistent 30s spacing between exports - Prevents rapid successive flushes (e.g., batch at 25s, timer at 30s) All changes follow existing driver patterns and maintain backward compatibility. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 6110797 commit 42540bc

File tree

2 files changed

+20
-6
lines changed

2 files changed

+20
-6
lines changed

lib/telemetry/DatabricksTelemetryExporter.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ interface DatabricksTelemetryLog {
7979
interface DatabricksTelemetryPayload {
8080
uploadTime: number;
8181
items: string[]; // Always empty - required field
82-
protoLogs: string[]; // JSON-stringified TelemetryFrontendLog objects
82+
protoLogs: string[]; // JSON-stringified DatabricksTelemetryLog objects
8383
}
8484

8585
/**
@@ -233,7 +233,11 @@ export default class DatabricksTelemetryExporter {
233233
// Get authentication headers if using authenticated endpoint
234234
const authHeaders = authenticatedExport ? await this.context.getAuthHeaders() : {};
235235

236-
// Make HTTP POST request with authentication
236+
// Get agent with proxy settings (same pattern as CloudFetchResultHandler and DBSQLSession)
237+
const connectionProvider = await this.context.getConnectionProvider();
238+
const agent = await connectionProvider.getAgent();
239+
240+
// Make HTTP POST request with authentication and proxy support
237241
const response: Response = await this.fetchFn(endpoint, {
238242
method: 'POST',
239243
headers: {
@@ -242,6 +246,7 @@ export default class DatabricksTelemetryExporter {
242246
'User-Agent': this.userAgent,
243247
},
244248
body: JSON.stringify(payload),
249+
agent, // Include agent for proxy support
245250
});
246251

247252
if (!response.ok) {

lib/telemetry/MetricsAggregator.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,10 @@ export default class MetricsAggregator {
296296

297297
/**
298298
* Flush all pending metrics to exporter. Never throws.
299+
*
300+
* @param resetTimer If true, resets the flush timer after flushing (default: true)
299301
*/
300-
flush(): void {
302+
flush(resetTimer: boolean = true): void {
301303
const logger = this.context.getLogger();
302304

303305
try {
@@ -312,6 +314,12 @@ export default class MetricsAggregator {
312314

313315
// Export metrics (exporter.export never throws)
314316
this.exporter.export(metricsToExport);
317+
318+
// Reset timer to avoid rapid successive flushes (e.g., batch flush at 25s then timer flush at 30s)
319+
// This ensures consistent spacing between exports and helps avoid rate limiting
320+
if (resetTimer) {
321+
this.startFlushTimer();
322+
}
315323
} catch (error: any) {
316324
// CRITICAL: All exceptions swallowed and logged at debug level ONLY
317325
logger.log(LogLevel.debug, `MetricsAggregator.flush error: ${error.message}`);
@@ -330,7 +338,8 @@ export default class MetricsAggregator {
330338
}
331339

332340
this.flushTimer = setInterval(() => {
333-
this.flush();
341+
// Don't reset timer when flush is triggered by the timer itself
342+
this.flush(false);
334343
}, this.flushIntervalMs);
335344

336345
// Prevent timer from keeping Node.js process alive
@@ -359,8 +368,8 @@ export default class MetricsAggregator {
359368
this.completeStatement(statementId);
360369
}
361370

362-
// Final flush
363-
this.flush();
371+
// Final flush - don't reset timer since we're closing
372+
this.flush(false);
364373
} catch (error: any) {
365374
// CRITICAL: All exceptions swallowed and logged at debug level ONLY
366375
logger.log(LogLevel.debug, `MetricsAggregator.close error: ${error.message}`);

0 commit comments

Comments
 (0)