Skip to content

Commit 5dbc5a0

Browse files
committed
fix: address telemetry code review issues
- ExceptionClassifier: classify ECONNREFUSED, ENOTFOUND, EHOSTUNREACH, ECONNRESET as retryable — these are transient network failures that were previously falling through as non-retryable and silently dropped - DatabricksTelemetryExporter: read driver version from lib/version.ts instead of hardcoded '1.0.0'; use uuid.v4() instead of hand-rolled UUID generation which had incorrect version/variant bits Co-authored-by: Isaac
1 parent 9a55d3b commit 5dbc5a0

File tree

3 files changed

+42
-12
lines changed

3 files changed

+42
-12
lines changed

lib/telemetry/DatabricksTelemetryExporter.ts

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@
1414
* limitations under the License.
1515
*/
1616

17+
import { v4 as uuidv4 } from 'uuid';
1718
import fetch, { Response, RequestInit } from 'node-fetch';
1819
import IClientContext from '../contracts/IClientContext';
1920
import { LogLevel } from '../contracts/IDBSQLLogger';
2021
import { TelemetryMetric, DEFAULT_TELEMETRY_CONFIG } from './types';
2122
import { CircuitBreakerRegistry } from './CircuitBreaker';
2223
import ExceptionClassifier from './ExceptionClassifier';
24+
import driverVersion from '../version';
2325

2426
/**
2527
* Databricks telemetry log format for export.
@@ -333,23 +335,14 @@ export default class DatabricksTelemetryExporter {
333335
* Generate a UUID v4.
334336
*/
335337
private generateUUID(): string {
336-
return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => {
337-
const r = (Math.random() * 16) | 0;
338-
const v = c === 'x' ? r : (r & 0x3) | 0x8;
339-
return v.toString(16);
340-
});
338+
return uuidv4();
341339
}
342340

343341
/**
344-
* Get driver version from package.json.
342+
* Get driver version from the version module.
345343
*/
346344
private getDriverVersion(): string {
347-
try {
348-
// In production, this would read from package.json
349-
return '1.0.0';
350-
} catch {
351-
return 'unknown';
352-
}
345+
return driverVersion;
353346
}
354347

355348
/**

lib/telemetry/ExceptionClassifier.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,17 @@ export default class ExceptionClassifier {
8181
return true;
8282
}
8383

84+
// Check for transient network errors (connection refused, DNS failure, host unreachable)
85+
const errorCode = (error as any).code;
86+
if (
87+
errorCode === 'ECONNREFUSED' ||
88+
errorCode === 'ENOTFOUND' ||
89+
errorCode === 'EHOSTUNREACH' ||
90+
errorCode === 'ECONNRESET'
91+
) {
92+
return true;
93+
}
94+
8495
// Check for HTTP status codes in error properties
8596
// Supporting both 'statusCode' and 'status' property names for flexibility
8697
const statusCode = (error as any).statusCode ?? (error as any).status;

tests/unit/telemetry/ExceptionClassifier.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,32 @@ describe('ExceptionClassifier', () => {
169169
});
170170
});
171171

172+
describe('Network connection errors', () => {
173+
it('should identify ECONNREFUSED as retryable', () => {
174+
const error = new Error('connect ECONNREFUSED 127.0.0.1:443');
175+
(error as any).code = 'ECONNREFUSED';
176+
expect(ExceptionClassifier.isRetryable(error)).to.be.true;
177+
});
178+
179+
it('should identify ENOTFOUND as retryable', () => {
180+
const error = new Error('getaddrinfo ENOTFOUND host.example.com');
181+
(error as any).code = 'ENOTFOUND';
182+
expect(ExceptionClassifier.isRetryable(error)).to.be.true;
183+
});
184+
185+
it('should identify EHOSTUNREACH as retryable', () => {
186+
const error = new Error('connect EHOSTUNREACH');
187+
(error as any).code = 'EHOSTUNREACH';
188+
expect(ExceptionClassifier.isRetryable(error)).to.be.true;
189+
});
190+
191+
it('should identify ECONNRESET as retryable', () => {
192+
const error = new Error('read ECONNRESET');
193+
(error as any).code = 'ECONNRESET';
194+
expect(ExceptionClassifier.isRetryable(error)).to.be.true;
195+
});
196+
});
197+
172198
describe('HTTP 429 Too Many Requests', () => {
173199
it('should identify 429 status code as retryable', () => {
174200
const error = new Error('Too Many Requests');

0 commit comments

Comments
 (0)