Skip to content

Commit 0f4ec12

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 a3c049f commit 0f4ec12

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
@@ -91,7 +91,7 @@ interface DatabricksTelemetryLog {
9191
interface DatabricksTelemetryPayload {
9292
uploadTime: number;
9393
items: string[]; // Always empty - required field
94-
protoLogs: string[]; // JSON-stringified TelemetryFrontendLog objects
94+
protoLogs: string[]; // JSON-stringified DatabricksTelemetryLog objects
9595
}
9696

9797
/**
@@ -237,7 +237,11 @@ export default class DatabricksTelemetryExporter {
237237
// Get authentication headers if using authenticated endpoint
238238
const authHeaders = authenticatedExport ? await this.context.getAuthHeaders() : {};
239239

240-
// Make HTTP POST request with authentication
240+
// Get agent with proxy settings (same pattern as CloudFetchResultHandler and DBSQLSession)
241+
const connectionProvider = await this.context.getConnectionProvider();
242+
const agent = await connectionProvider.getAgent();
243+
244+
// Make HTTP POST request with authentication and proxy support
241245
const response: Response = await this.fetchFn(endpoint, {
242246
method: 'POST',
243247
headers: {
@@ -246,6 +250,7 @@ export default class DatabricksTelemetryExporter {
246250
'User-Agent': this.userAgent,
247251
},
248252
body: JSON.stringify(payload),
253+
agent, // Include agent for proxy support
249254
});
250255

251256
if (!response.ok) {

lib/telemetry/MetricsAggregator.ts

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

338338
/**
339339
* Flush all pending metrics to exporter. Never throws.
340+
*
341+
* @param resetTimer If true, resets the flush timer after flushing (default: true)
340342
*/
341-
flush(): void {
343+
flush(resetTimer: boolean = true): void {
342344
const logger = this.context.getLogger();
343345

344346
try {
@@ -351,6 +353,12 @@ export default class MetricsAggregator {
351353

352354
// Export metrics (exporter.export never throws)
353355
this.exporter.export(metricsToExport);
356+
357+
// Reset timer to avoid rapid successive flushes (e.g., batch flush at 25s then timer flush at 30s)
358+
// This ensures consistent spacing between exports and helps avoid rate limiting
359+
if (resetTimer) {
360+
this.startFlushTimer();
361+
}
354362
} catch (error: any) {
355363
// CRITICAL: All exceptions swallowed and logged at debug level ONLY
356364
logger.log(LogLevel.debug, `MetricsAggregator.flush error: ${error.message}`);
@@ -369,7 +377,8 @@ export default class MetricsAggregator {
369377
}
370378

371379
this.flushTimer = setInterval(() => {
372-
this.flush();
380+
// Don't reset timer when flush is triggered by the timer itself
381+
this.flush(false);
373382
}, this.flushIntervalMs);
374383

375384
// Prevent timer from keeping Node.js process alive
@@ -398,8 +407,8 @@ export default class MetricsAggregator {
398407
this.completeStatement(statementId);
399408
}
400409

401-
// Final flush
402-
this.flush();
410+
// Final flush - don't reset timer since we're closing
411+
this.flush(false);
403412
} catch (error: any) {
404413
// CRITICAL: All exceptions swallowed and logged at debug level ONLY
405414
logger.log(LogLevel.debug, `MetricsAggregator.close error: ${error.message}`);

0 commit comments

Comments
 (0)