fix: address Jade's review comments

samikshya-db · samikshya-db · commit 658c4f69493a · 2026-04-17T19:26:22.000Z
- ExceptionClassifier: remove ENOTFOUND from retryable (DNS "not found"
  is deterministic — retrying just pushes load at the resolver without
  any expectation of success). Add ETIMEDOUT and EAI_AGAIN per Jade's
  follow-up list.
- Extract shared `normalizeHeaders` + `hasAuthorization` helpers into
  telemetryUtils; DatabricksTelemetryExporter and FeatureFlagCache both
  use them — eliminates the ~40-line duplication Jade flagged.
- normalizeHeaders now guards `typeof raw === 'object'` before
  Object.entries, preventing Object.entries('some-string') index entries
  from leaking in as headers (Jade: "should we do type check here?").
- FeatureFlagCache.fetchFeatureFlag: add single-retry on transient errors
  (classified via ExceptionClassifier). Without a retry, one DNS hiccup
  would disable telemetry for the full 15-minute cache TTL; one retry
  gives an ephemeral failure a second chance without pushing sustained
  load at a broken endpoint.
- Drop the private hasAuthorization/normalizeHeaders on the exporter;
  drop the inlined branching in FFC getAuthHeaders.
- Update ExceptionClassifier tests: invert ENOTFOUND expectation with
  a comment explaining why; add cases for ETIMEDOUT and EAI_AGAIN.

702 unit tests pass, ESLint clean.

Co-authored-by: Isaac
Signed-off-by: samikshya-chand_data &lt;samikshya.chand@databricks.com&gt;
diff --git a/lib/telemetry/DatabricksTelemetryExporter.ts b/lib/telemetry/DatabricksTelemetryExporter.ts
@@ -15,7 +15,7 @@
  */
 
 import { v4 as uuidv4 } from 'uuid';
-import fetch, { Headers, RequestInit, Response } from 'node-fetch';
+import fetch, { RequestInit, Response } from 'node-fetch';
 import IClientContext from '../contracts/IClientContext';
 import { LogLevel } from '../contracts/IDBSQLLogger';
 import IAuthentication from '../connection/contracts/IAuthentication';
@@ -24,7 +24,13 @@ import HiveDriverError from '../errors/HiveDriverError';
 import { TelemetryMetric, DEFAULT_TELEMETRY_CONFIG } from './types';
 import { CircuitBreaker, CircuitBreakerOpenError, CircuitBreakerRegistry } from './CircuitBreaker';
 import ExceptionClassifier from './ExceptionClassifier';
-import { buildTelemetryUrl, redactSensitive, sanitizeProcessName } from './telemetryUtils';
+import {
+  buildTelemetryUrl,
+  hasAuthorization,
+  normalizeHeaders,
+  redactSensitive,
+  sanitizeProcessName,
+} from './telemetryUtils';
 import buildUserAgentString from '../utils/buildUserAgentString';
 
 interface DatabricksTelemetryLog {
@@ -238,7 +244,7 @@ export default class DatabricksTelemetryExporter {
 
     if (authenticatedExport) {
       headers = { ...headers, ...(await this.getAuthHeaders()) };
-      if (!this.hasAuthorization(headers)) {
+      if (!hasAuthorization(headers)) {
         if (!this.authMissingWarned) {
           this.authMissingWarned = true;
           logger.log(LogLevel.warn, 'Telemetry: Authorization header missing — metrics will be dropped');
@@ -288,54 +294,13 @@ export default class DatabricksTelemetryExporter {
     }
     const logger = this.context.getLogger();
     try {
-      const raw = await this.authProvider.authenticate();
-      return this.normalizeHeaders(raw);
+      return normalizeHeaders(await this.authProvider.authenticate());
     } catch (error: any) {
       logger.log(LogLevel.debug, `Telemetry: auth provider threw: ${error?.message ?? error}`);
       return {};
     }
   }
 
-  private normalizeHeaders(raw: unknown): Record<string, string> {
-    if (!raw) {
-      return {};
-    }
-    // node-fetch HeadersInit = Headers | [string,string][] | Record<string,string>.
-    if (raw instanceof Headers) {
-      const out: Record<string, string> = {};
-      raw.forEach((value: string, key: string) => {
-        out[key] = value;
-      });
-      return out;
-    }
-    if (Array.isArray(raw)) {
-      const out: Record<string, string> = {};
-      for (const entry of raw as Array<[string, string]>) {
-        if (entry && entry.length === 2 && typeof entry[1] === 'string') {
-          const [key, value] = entry;
-          out[key] = value;
-        }
-      }
-      return out;
-    }
-    const out: Record<string, string> = {};
-    for (const [k, v] of Object.entries(raw as Record<string, unknown>)) {
-      if (typeof v === 'string') {
-        out[k] = v;
-      }
-    }
-    return out;
-  }
-
-  private hasAuthorization(headers: Record<string, string>): boolean {
-    for (const key of Object.keys(headers)) {
-      if (key.toLowerCase() === 'authorization' && headers[key]) {
-        return true;
-      }
-    }
-    return false;
-  }
-
   private async sendRequest(url: string, init: RequestInit): Promise<Response> {
     const connectionProvider = await this.context.getConnectionProvider();
     const agent = await connectionProvider.getAgent();
diff --git a/lib/telemetry/ExceptionClassifier.ts b/lib/telemetry/ExceptionClassifier.ts
@@ -81,13 +81,17 @@ export default class ExceptionClassifier {
       return true;
     }
 
-    // Check for transient network errors (connection refused, DNS failure, host unreachable)
+    // Transient network errors. Notably does NOT include `ENOTFOUND`:
+    // DNS "not found" usually indicates a misconfigured host, not a transient
+    // fault. Retrying pushes load at the resolver without any expectation of
+    // success; circuit-breaker failure accounting is the better response.
     const errorCode = (error as any).code;
     if (
       errorCode === 'ECONNREFUSED' ||
-      errorCode === 'ENOTFOUND' ||
+      errorCode === 'ECONNRESET' ||
       errorCode === 'EHOSTUNREACH' ||
-      errorCode === 'ECONNRESET'
+      errorCode === 'ETIMEDOUT' ||
+      errorCode === 'EAI_AGAIN'
     ) {
       return true;
     }
diff --git a/lib/telemetry/FeatureFlagCache.ts b/lib/telemetry/FeatureFlagCache.ts
@@ -14,11 +14,12 @@
  * limitations under the License.
  */
 
-import fetch, { Headers, RequestInit, Response } from 'node-fetch';
+import fetch, { RequestInit, Response } from 'node-fetch';
 import IClientContext from '../contracts/IClientContext';
 import { LogLevel } from '../contracts/IDBSQLLogger';
 import IAuthentication from '../connection/contracts/IAuthentication';
-import { buildTelemetryUrl } from './telemetryUtils';
+import { buildTelemetryUrl, normalizeHeaders } from './telemetryUtils';
+import ExceptionClassifier from './ExceptionClassifier';
 import buildUserAgentString from '../utils/buildUserAgentString';
 import driverVersion from '../version';
 
@@ -145,7 +146,7 @@ export default class FeatureFlagCache {
 
       logger.log(LogLevel.debug, `Fetching feature flags from ${endpoint}`);
 
-      const response = await this.sendRequest(endpoint, {
+      const response = await this.fetchWithRetry(endpoint, {
         method: 'GET',
         headers,
         timeout: 10000,
@@ -183,39 +184,37 @@ export default class FeatureFlagCache {
     }
   }
 
-  private async sendRequest(url: string, init: RequestInit): Promise<Response> {
+  /**
+   * Retries transient network errors once before giving up. Without a retry
+   * a single hiccup would leave telemetry disabled for the full cache TTL
+   * (15 min). One retry gives an ephemeral DNS / connection-reset failure
+   * a second chance without pushing sustained load at a broken endpoint.
+   */
+  private async fetchWithRetry(url: string, init: RequestInit): Promise<Response> {
     const connectionProvider = await this.context.getConnectionProvider();
     const agent = await connectionProvider.getAgent();
-    return fetch(url, { ...init, agent });
+    const logger = this.context.getLogger();
+
+    try {
+      return await fetch(url, { ...init, agent });
+    } catch (err: any) {
+      if (!ExceptionClassifier.isRetryable(err)) {
+        throw err;
+      }
+      logger.log(LogLevel.debug, `Feature flag fetch retry after transient: ${err?.code ?? err?.message ?? err}`);
+      await new Promise((resolve) => {
+        setTimeout(resolve, 100 + Math.random() * 100);
+      });
+      return fetch(url, { ...init, agent });
+    }
   }
 
   private async getAuthHeaders(): Promise<Record<string, string>> {
     if (!this.authProvider) {
       return {};
     }
     try {
-      const raw = await this.authProvider.authenticate();
-      if (!raw) {
-        return {};
-      }
-      if (raw instanceof Headers) {
-        const out: Record<string, string> = {};
-        raw.forEach((value: string, key: string) => {
-          out[key] = value;
-        });
-        return out;
-      }
-      if (Array.isArray(raw)) {
-        const out: Record<string, string> = {};
-        for (const entry of raw as Array<[string, string]>) {
-          if (entry && entry.length === 2) {
-            const [key, value] = entry;
-            out[key] = value;
-          }
-        }
-        return out;
-      }
-      return { ...(raw as Record<string, string>) };
+      return normalizeHeaders(await this.authProvider.authenticate());
     } catch (error: any) {
       this.context.getLogger().log(LogLevel.debug, `Feature flag auth failed: ${error?.message ?? error}`);
       return {};
diff --git a/lib/telemetry/telemetryUtils.ts b/lib/telemetry/telemetryUtils.ts
@@ -154,6 +154,57 @@ export function redactSensitive(value: string | undefined, maxLen = 2048): strin
   return redacted;
 }
 
+/**
+ * Normalises any `HeadersInit` shape (`Headers`, `[string,string][]`, or
+ * `Record<string, string>`) into a plain string dictionary. Non-string
+ * values are dropped. Shared by the exporter and FeatureFlagCache so there
+ * is one source of truth for auth-header handling.
+ */
+export function normalizeHeaders(raw: unknown): Record<string, string> {
+  if (!raw || typeof raw !== 'object') {
+    return {};
+  }
+  // Avoid importing node-fetch here; use structural check.
+  if (typeof (raw as any).forEach === 'function' && !Array.isArray(raw)) {
+    const out: Record<string, string> = {};
+    (raw as any).forEach((value: unknown, key: unknown) => {
+      if (typeof key === 'string' && typeof value === 'string') {
+        out[key] = value;
+      }
+    });
+    return out;
+  }
+  if (Array.isArray(raw)) {
+    const out: Record<string, string> = {};
+    for (const entry of raw as Array<[unknown, unknown]>) {
+      if (Array.isArray(entry) && entry.length === 2 && typeof entry[0] === 'string' && typeof entry[1] === 'string') {
+        const [key, value] = entry;
+        out[key] = value;
+      }
+    }
+    return out;
+  }
+  const out: Record<string, string> = {};
+  for (const [k, v] of Object.entries(raw as Record<string, unknown>)) {
+    if (typeof v === 'string') {
+      out[k] = v;
+    }
+  }
+  return out;
+}
+
+/**
+ * Case-insensitive check for a non-empty `Authorization` header.
+ */
+export function hasAuthorization(headers: Record<string, string>): boolean {
+  for (const key of Object.keys(headers)) {
+    if (key.toLowerCase() === 'authorization' && headers[key]) {
+      return true;
+    }
+  }
+  return false;
+}
+
 /**
  * Returns a safe `process_name` value: the basename of the first whitespace-
  * delimited token, with trailing whitespace trimmed. This defeats both the
diff --git a/tests/unit/telemetry/ExceptionClassifier.test.ts b/tests/unit/telemetry/ExceptionClassifier.test.ts
@@ -176,10 +176,13 @@ describe('ExceptionClassifier', () => {
         expect(ExceptionClassifier.isRetryable(error)).to.be.true;
       });
 
-      it('should identify ENOTFOUND as retryable', () => {
+      it('should NOT identify ENOTFOUND as retryable (likely misconfigured host)', () => {
+        // DNS "not found" is deterministic; retrying just pushes load at
+        // the resolver without any expectation of success. Breaker records
+        // it as a failure and moves on.
         const error = new Error('getaddrinfo ENOTFOUND host.example.com');
         (error as any).code = 'ENOTFOUND';
-        expect(ExceptionClassifier.isRetryable(error)).to.be.true;
+        expect(ExceptionClassifier.isRetryable(error)).to.be.false;
       });
 
       it('should identify EHOSTUNREACH as retryable', () => {
@@ -193,6 +196,19 @@ describe('ExceptionClassifier', () => {
         (error as any).code = 'ECONNRESET';
         expect(ExceptionClassifier.isRetryable(error)).to.be.true;
       });
+
+      it('should identify ETIMEDOUT as retryable', () => {
+        const error = new Error('connect ETIMEDOUT');
+        (error as any).code = 'ETIMEDOUT';
+        expect(ExceptionClassifier.isRetryable(error)).to.be.true;
+      });
+
+      it('should identify EAI_AGAIN as retryable', () => {
+        // getaddrinfo EAI_AGAIN is a temporary DNS lookup failure (unlike ENOTFOUND).
+        const error = new Error('getaddrinfo EAI_AGAIN host.example.com');
+        (error as any).code = 'EAI_AGAIN';
+        expect(ExceptionClassifier.isRetryable(error)).to.be.true;
+      });
     });
 
     describe('HTTP 429 Too Many Requests', () => {
