[4/7] Telemetry Event Emission and Aggregation

[samikshya-db]

Part 4 of 7-part Telemetry Implementation Stack

This layer adds event-driven telemetry emission, per-host shared
aggregation/export, and operation-level error telemetry on top of the
hardened infrastructure shipped in [1/7]–[3/7].

What's in this PR

TelemetryEventEmitter (lib/telemetry/TelemetryEventEmitter.ts)

Per-DBSQLClient event emitter — typed emission methods, respects the
client's telemetryEnabled flag, swallows all exceptions at debug level.
Each emitter bridges into the shared aggregator on the per-host
TelemetryClient.

Event types: CONNECTION_OPEN, CONNECTION_CLOSE (new), STATEMENT_START,
STATEMENT_COMPLETE, CLOUDFETCH_CHUNK, ERROR.

TelemetryClient owns the per-host triad (lib/telemetry/TelemetryClient.ts)

TelemetryClientProvider is a process-wide singleton. Each host gets one
TelemetryClient that owns:

• DatabricksTelemetryExporter
• MetricsAggregator
• CircuitBreakerRegistry
• FeatureFlagCache

Multiple DBSQLClient instances on the same host share these — breaker
counters and HTTP batches don't fragment per-instance. TelemetryClient
implements IClientContext so the owned components have a stable context
that survives any single DBSQLClient's close. Connection/auth providers
are tracked in a FIFO of registered contexts; the exporter falls through
to the next active one when the head closes.

MetricsAggregator (per-host, on TelemetryClient)

Restored from main's hardened version, with new functionality layered on:

• CONNECTION_CLOSE handling — emits DELETE_SESSION connection metric.
• Chunk-timing aggregation — chunkInitialLatencyMs,
  chunkSlowestLatencyMs, chunkSumLatencyMs accumulated from
  CLOUDFETCH_CHUNK events with positive latency.
• Memory bounds: maxPendingMetrics (drop preferring non-error to keep
  first-failure signal), maxErrorsPerStatement, statementTtlMs eviction.
• Flush triggers: batch size, periodic timer (unref()'d), terminal-error
  immediate flush, manual flush().
• close() is async and awaits the final HTTP POST so
  await client.close(); process.exit(0) doesn't truncate the last batch.

Error telemetry wired into operation entry points

DBSQLOperation now emits ERROR events (with ExceptionClassifier
terminal/retryable classification) from fetchChunk, cancel, close,
and getMetadata. Failed queries produce a STATEMENT_COMPLETE plus an
ERROR proto with error_info: { error_name, stack_trace } (stack run
through redactSensitive).

emitStatementComplete no longer issues a getMetadata Thrift RPC on
close (perf regression + spurious-error-telemetry trap).

Type-safe wiring (IClientContext)

Added optional getTelemetryEmitter() / getTelemetryAggregator() to
IClientContext. Removed all (this.context as any) casts at the seven
emit call sites (DBSQLOperation, DBSQLSession, RowSetProvider,
CloudFetchResultHandler).

The six copy-pasted listeners in DBSQLClient.initializeTelemetry are now
one bridge loop over Object.values(TelemetryEventType) — closes the
listener-name mismatch that originally caused error events to be silently
dropped.

mapAuthType covers all six authType values (access-token, databricks-oauth
{U2M/M2M}, custom, token-provider, external-token, static-token).

Verified end-to-end against an Azure Databricks workspace

Healthy SELECT 1 produces 3 wire metrics: CREATE_SESSION (with
system_configuration), STATEMENT_COMPLETE (with
sql_operation.execution_result), DELETE_SESSION.

Failed query produces 4: CREATE_SESSION, STATEMENT_COMPLETE (latency
only), ERROR (with redacted stack), DELETE_SESSION.

Server-side feature flag is still the kill switch — telemetryEnabled: false
on the client also skips the entire pipeline (no acquire/release noise).

Testing

484 unit tests pass (telemetry + DBSQLClient/Operation/Session/result).
Test files for the rebased modules are restored from main. Provider tests
updated for the singleton API.

Coverage gap acknowledged: no unit tests yet for the re-applied
chunk-timing aggregation or CONNECTION_CLOSE handling. Adding these as a
follow-up to the same epic.

Dependencies

Depends on:

• [1/7] Telemetry Foundation ([1/7] Telemetry Foundation: Types, Config, and Exception Classifier #324)
• [2/7] Infrastructure ([2/7] Telemetry Infrastructure: CircuitBreaker and FeatureFlagCache #325)
• [3/7] Client Management ([3/7] Telemetry Client Management: TelemetryClient and Provider #326)

Out of scope (open follow-ups)

• Add operation_type proto field so CREATE_SESSION and DELETE_SESSION
  are explicitly distinguished on the wire (today they're distinguishable
  only by the incidental presence of system_configuration).
• Add getStats() for telemetry-pipeline self-observability (drop counts,
  queue depth, last-success timestamp).
• Expose remaining telemetry config knobs in ConnectionOptions (currently
  3 of 13).
• Reunify connection.open/connection.close emission (open from DBSQLClient,
  close from DBSQLSession).
• Unit tests for chunk-timing aggregation and CONNECTION_CLOSE.

[samikshya-db]

The emission format confirms to the telemetry proto, marked this ready for review.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[vikrantpuppala]

[F1 — Critical] connection.close.latencyMs reports session lifetime, not RPC duration

this.openTime is set in the session constructor (DBSQLSession.ts:175, this.openTime = Date.now()), so this latencyMs is the wall-clock session lifetime, not the closeSession RPC duration. The matching CREATE_SESSION event emits actual openSession RPC duration. Two different definitions land under the same operation_latency_ms field server-side, leading to incorrect close-latency conclusions.

Fix: capture closeStart = Date.now() immediately before await driver.closeSession(...) and emit Date.now() - closeStart. Or rename the field on close (e.g. sessionLifetimeMs) and document both meanings.

Posted by Code Review Squad.

[vikrantpuppala]

[F2 — Critical] extractWorkspaceId is wrong on AWS and Azure hostnames

host.split('.')[0] returns:

• AWS dbc-XXXXX-YYYY.cloud.databricks.com → dbc-XXXXX-YYYY (not the workspace ID; that's the deployment shard prefix)
• Azure adb-NNNNNNNNNNNNN.NN.azuredatabricks.net → adb-NNNNNNNNNNNNN (workspace ID is the numeric portion of adb-N, dropping the adb- prefix and trailing form-factor digit)

Every CREATE_SESSION/DELETE_SESSION metric ships a confidently-wrong workspaceId for AWS and Azure customers, silently mis-attributing usage server-side. The JSDoc example workspace-id.cloud.databricks.com is not a real hostname format.

Fix: pull the real workspace ID from the auth response or the connection URL ?o=N parameter. If neither is available, omit the field rather than ship a wrong value. Also: split('.') never "fails" — the or host if extraction fails comment is misleading.

Posted by Code Review Squad.

[vikrantpuppala]

[F3 — High] getTelemetryEmitter() exposes un-redacted error stacks to in-process listeners

@internal is a JSDoc tag, not a runtime guard — this method is reachable from any consumer that imports @databricks/sql. The TelemetryEventEmitter extends Node's EventEmitter, so:

client.getTelemetryEmitter()?.on('telemetry.error', e => exfil(e.errorStack));

works at runtime today. redactSensitive runs only at export time in DatabricksTelemetryExporter.toTelemetryLog, not at emit time, so an in-process listener sees raw errorMessage and errorStack (see DBSQLOperation.ts:592-602). With telemetry now default-on, this is a credential/PII channel that didn't materially exist before.

Fix: redact at the emitter (emitError) before calling this.emit(...). The aggregator/exporter then receive already-redacted strings — single redaction site. Optionally narrow getTelemetryEmitter() to a curated read-only surface.

Posted by Code Review Squad.

[vikrantpuppala]

[F4 — High] Multi-tenant FIFO credential bleed across DBSQLClients on same host

A SaaS layer fronting tenants A and B against the same workspace host shares one TelemetryClient. Telemetry from tenant B's queries can be POSTed under tenant A's auth headers (whoever connected first wins the FIFO head). Same shape applies to userAgentEntry and telemetryAuthenticatedExport — tenant A's value silently wins. If tenant B explicitly set telemetryAuthenticatedExport: false, B's events ride A's authenticated pipeline.

Fix options (pick one):

1. Shard the registry by (host, userAgentEntry, authenticatedExport, authProvider-identity) instead of host alone.
2. Auto-disable telemetry when a second context registers with diverging auth/authenticatedExport (lose telemetry > leak credential).
3. Per-context exporter with shared circuit-breaker counters.

At minimum, document this prominently in the README opt-out section so SaaS deployments know to set telemetryEnabled: false.

Posted by Code Review Squad.

[vikrantpuppala]

[F5 — High] Auth-provider deregistration uses identity at unregister-time → stale credential leak on rotation

unregisterContext calls context.getAuthProvider?.() at unregister time and filters by referential equality. If the context's auth provider was rotated between register and unregister (token refresh that reconstructs the provider, lazy wrappers, anything that doesn't return the same object reference), the original provider is never removed from this.authProviders. The exporter's FIFO walk in getAuthProvider() (line 203-215) can keep authenticating with revoked credentials for the lifetime of the per-host singleton.

Fix: cache the auth-provider snapshot on the contexts entry at register time (registerContext); use that cached snapshot at unregister time, not a fresh call.

Posted by Code Review Squad.

[vikrantpuppala]

[F9 — High] Default-config drift: 7 keys here vs 15 in DEFAULT_TELEMETRY_CONFIG

getDefaultConfig (this method, lines 128-134) defines 7 telemetry keys. DEFAULT_TELEMETRY_CONFIG in lib/telemetry/types.ts:91-103 defines 15. Components like MetricsAggregator fall back through ?? DEFAULT_TELEMETRY_CONFIG.X for the missing keys. Today the values agree by hand; they will silently desync the next time someone changes one source but not the other.

Fix: spread DEFAULT_TELEMETRY_CONFIG here (with a typed mapper for the telemetry-prefixed ClientConfig keys), or stop maintaining the inline copy and route every component through the single frozen const.

Posted by Code Review Squad.

[vikrantpuppala]

[F11 — Medium] as any cast removes type safety on every telemetry option override

The telemetryOverrides array above is as const so the keys are a literal union. Both ConnectionOptions and ClientConfig have identical key names and types for the telemetry knobs. The cast escapes the structural type system for no reason. A user passing telemetryBatchSize: "100" (string) silently writes a string into a number field; MetricsAggregator then reads it as number and aggregation thresholds break at runtime.

Fix: per-key narrowed assignments, or a small typed pickDefined<T, K>(dst: T, src: Partial<T>, keys: readonly K[]) helper. Same readability, no cast.

Posted by Code Review Squad.

[vikrantpuppala]

[F18 — Medium] getTelemetryAggregator() lacks explicit return-type annotation

Three reviewers flagged this. The return type is inferred as MetricsAggregator | undefined (from this.telemetryClient?.getAggregator()), but MetricsAggregator is intentionally not re-exported from lib/index.ts. Consumers calling client.getTelemetryAggregator() see a method whose return type references a non-exported symbol — autocomplete degrades, agent-completion gets confused, and the contract diverges from IClientContext.getTelemetryAggregator?(): MetricsAggregator | undefined even though the line two above (getTelemetryEmitter()) does annotate explicitly.

Fix: add : MetricsAggregator | undefined to match IClientContext. Since the method is @internal, an even cleaner option is to remove it from the public class entirely (move to a friend interface used only within the package) — TS-public methods marked only with a JSDoc @internal are the worst of both worlds for autocomplete.

Posted by Code Review Squad.

[vikrantpuppala]

[F22 — Medium] DATABRICKS_TELEMETRY_DISABLED=false keeps telemetry on — silently ignored

The regex /^(1|true|yes|on)$/i is the deliberate "footgun-avoidance" choice (so false/0/no don't accidentally disable opt-out), and the comment above explains it. The remaining UX problem: an ops engineer who sees the env var and tries to "set it to false to keep telemetry on" gets the opposite of what they expect — false is silently ignored, leaving runtime config (default true) in charge.

Fix: log a LogLevel.warn line when the env var is set to a non-recognized non-empty value (false, 0, no, off, etc.) so ops can see their disable didn't take effect. That preserves the safe-default behavior while surfacing the misconfiguration.

Posted by Code Review Squad.

[vikrantpuppala]

[F10 — Medium] RowSetProvider.emitChunkEvent undercounts bytes for non-arrow paths

bytes is computed only from response.results?.arrowBatches. A TFetchResultsResp carrying inline JSON / column-based data (COLUMN_BASED_SET) reports bytes: 0. URL-based result sets report bytes via CloudFetchResultHandler (a separate emit site), so URL-based is fine. But column/json paths flow through here and the metric is then aggregated into bytesDownloaded in MetricsAggregator.processStatementEvent (details.bytesDownloaded += event.bytes ?? 0).

Net effect: dashboards that aggregate bytesDownloaded will be silently undercounted whenever the result is inline JSON / column-based.

Fix: compute and account inline-result bytes here for non-arrow paths, or document explicitly that bytesDownloaded from this emit site counts only arrow-batch payloads (so dashboards know to add the CloudFetch-emitted byte counts separately).

Posted by Code Review Squad.

[vikrantpuppala]

[F12 — Medium] sessionId default mismatch — '' here, undefined in emitErrorEvent

emitStatementStart (line 549) and emitStatementComplete (line 575) emit sessionId: this.sessionId || ''. emitErrorEvent (line 596) emits sessionId: this.sessionId (no || ''). The aggregator's per-statement state groups by sessionId, so an operation with no sessionId emits start/complete keyed under empty string and errors keyed under undefined — split into two synthetic sessions in the aggregator and exporter.

Fix: pick one default and apply consistently. Prefer undefined and let the aggregator filter at the processEvent boundary (cleaner than synthesizing fake empty-string sessions).

Posted by Code Review Squad.

[vikrantpuppala]

Code Review Squad Report

Scope: 29 files, +2970/-424 lines
Iteration: 1
Coverage: 9/9 reviewers delivered (security, devils-advocate, language, test, architecture, maintainability, agent-compat, ops, performance)
Merge Safety Score: 0/100 — CRITICAL RISK (deduction-floored by volume of findings; substance below)

Executive Summary

This is a sizable, mostly well-engineered telemetry layer that ships two verified data-integrity bugs that mis-stamp every connection-close metric and every workspace-attribution field on AWS+Azure customers, plus multi-tenant safety issues (FIFO auth-provider sharing, silent override of opt-out config) that materially undermine the privacy posture of a default-on feature. Tests for the +314-line DBSQLClient changes are essentially absent.

None of these are unrecoverable — most have small, focused fixes — but they should land before this merges and ships to customers as part of the 1.13 default-on telemetry rollout.

---

Critical & High findings (inline comments above)

ID	Severity	Title
F1	Critical	connection.close.latencyMs reports session lifetime, not RPC duration (DBSQLSession.ts:596)
F2	Critical	extractWorkspaceId is wrong on AWS+Azure hostnames (DBSQLClient.ts:255)
F3	High	getTelemetryEmitter() exposes un-redacted error stacks to in-process listeners (DBSQLClient.ts:678)
F4	High	Multi-tenant FIFO credential bleed across DBSQLClients on same host (TelemetryClient.ts:215)
F5	High	Auth-provider deregistration uses identity at unregister-time → stale credential leak on rotation (TelemetryClient.ts:145)
F9	High	Default-config drift: 7 keys in getDefaultConfig vs 15 in DEFAULT_TELEMETRY_CONFIG (DBSQLClient.ts:134)

High findings without a specific anchor

F6 — Default-on telemetry ships customer-controlled userAgentEntry un-redacted to the telemetry endpoint.
DatabricksTelemetryExporter.ts:119 concatenates userAgentEntry into the User-Agent header on every telemetry POST. The existing UA redactor only catches Databricks token prefixes — emails, JWTs, OAuth client_secret values, internal system codes, anything else customers stuff into userAgentEntry ships in the User-Agent header. With telemetry default-on, this shifts from opt-in risk to default risk. Fix: run redactSensitive over userAgentEntry before it lands in the UA, or document explicitly that anything passed to userAgentEntry is shipped to the telemetry endpoint.

F7 — DBSQLClient.ts +314 lines has zero new unit tests.
tests/unit/DBSQLClient.test.ts is unchanged in this PR. Untested code paths: initializeTelemetry (the refcount-release fix that motivated commit 7d10d1e), getTelemetryEmitter, getTelemetryAggregator, extractWorkspaceId, buildDriverConfiguration, mapAuthType, getLocaleName, getProcessName, the DATABRICKS_TELEMETRY_DISABLED env-var parser, the reconnect refcount-release path. Same gap for DBSQLSession.close CONNECTION_CLOSE emission and CloudFetchResultHandler.emitCloudFetchChunk. Fix: add unit tests with stubbed TelemetryClient/TelemetryClientProvider for at least the env-kill parser, reconnect refcount path, CONNECTION_OPEN/CLOSE emission, and the CloudFetch chunk emit site.

F8 — Telemetry HTTP retry double-stacks with user's 15-min retriesTimeout.
The exporter's sendRequest goes through connectionProvider.getRetryPolicy() → HttpRetryPolicy.invokeWithRetry, which is bounded by the customer's ClientConfig.retriesTimeout (15 min default) and retryMaxAttempts (5). On top of that the exporter has its own retry loop (telemetryMaxRetries=3, telemetryBackoffMaxMs=1s). Worst-case single-batch retry budget reaches ~15 min wall-clock per outer attempt × 3 outer ≈ ~15 min before the circuit breaker even sees the failure. During that window flushInFlight blocks subsequent flushes; addPendingMetric silently drops past the 500 cap. Fix: in DatabricksTelemetryExporter.sendRequest, override the retry policy to telemetry-specific bounds (or pass a NullRetryPolicy).

Medium / Low findings (no inline comment, summarized here)

• F13 — Drop counter / queue depth / breaker state are invisible to operators. addPendingMetric drops are at LogLevel.debug only; default driver log level is info. Add droppedMetrics counter, warn-level summary on flush when > 0, and a getTelemetryStats() API.
• F14 — Untyped EventEmitter; listener payloads inferred as any in TelemetryEventEmitter and the listener wired up in DBSQLClient.initializeTelemetry. Either typed on<K> overloads, or drop the EventEmitter inheritance entirely (the bridge has one listener per type).
• F15 — CONNECTION_OPEN emitted per openSession, not per connect() — A long-running DBSQLClient opening 1000 sessions emits 1000 CREATE_SESSION events each with a ~1KB driverConfig blob. Strip static driverConfig from events 2..N or emit once per connect().
• F16 — safeEmit typed as any despite no real cycle. Comment cites "dependency cycle through IClientContext / TelemetryEventEmitter," but IClientContext already does import type TelemetryEventEmitter. A typo like emitter.emiitConnectionOpen(...) slips through and fails at runtime as a swallowed exception. Type-only imports both ways and safeEmit becomes properly typed.
• F17 — TelemetryClient fall-through helpers (getDriver/getClient/getConnectionProvider) don't prune throwing contexts. A throwing context (revoked auth, closed client) stays in this.contexts and is retried on every export indefinitely.
• F19 — Stack-trace redaction misses common non-home paths: /var/lib/<service>/..., /opt/..., container /app/... (Lambda/k8s/Cloud Run common), \\server\share\..., macOS /private/var/folders/.... The Bearer/JWT patterns are correctly implemented; the path patterns are too narrow.
• F20 — e2e telemetry-integration tests are spy-only. Asserts only that internal sinon spies were called; no ingestion verification. "Reference Counting" uses at.least(2). "Cleanup on close" uses disjunction over 3 spies. One spy created and never asserted (telemetryProviderSpy). No before() skip-guard on missing creds — runner hard-exits in unconfigured CI via validateConfig's process.exit(1).
• F21 — No redaction of operation errorMessage body content. DBSQLOperation.ts:597-600: errorMessage: error.message || 'Unknown error' flows through verbatim. README claims "non-PII error context" but errorMessage strings can contain query fragments, table names, hostnames, parameter values that the SECRET_PATTERNS regex doesn't catch. errorStack runs through redactSensitive at export time, but not the message body.
• F23 — Workspace-id casing inconsistency. Registry lowercases host (TelemetryClientProvider.normalizeHostKey); metric reports original case from extractWorkspaceId(this.host). Workspace-A.cloud.databricks.com registers under workspace-a but ships workspaceId: 'Workspace-A'.
• F24 — Documented vs code defaults reconciliation. Have one canonical source for documented defaults (the README either stays abstract or quotes from DEFAULT_TELEMETRY_CONFIG).
• F26 — chunkSumLatencyMs reported only when > 0 while chunkCount always reported (MetricsAggregator). 5 chunks / 0ms in dashboards is confusing.
• F27 — compressionEnabled only carries last chunk's value — model mismatch with mixed-compression batches.
• F28 — Redundant Promise.resolve(this.flush(false)).catch(...) wrapping in MetricsAggregator (3 sites). flush() already returns Promise<void>; drop the Promise.resolve wrapper.
• F29 — No process.on('beforeExit', flush) hook to mitigate process.exit(0) data loss. Optional defensive add (gated by a flag).
• F30 — TelemetryClientProvider.resetInstance() is reachable in production code. Rename to __resetInstanceForTests to discourage use.
• F31 — IClientContext optional-method approach is a smell across many call sites; an ITelemetrySink interface would be cleaner long-term.
• F32 — telemetryMaxPendingMetrics is read by aggregator from ClientConfig but not exposed in ConnectionOptions — users have no public knob to raise it.
• F36 — Six near-identical emit* methods in TelemetryEventEmitter could share a private emitWrapped(type, build) helper. ~170 of 220 lines are scaffold.

---

Verified non-issues (confirmed against PR head; flagged so reviewers don't re-raise)

• No "6 placeholder event handlers" in DBSQLClient.ts. They don't exist; the listener registration is an enum-driven bridge into the shared aggregator. (Confirmed by 4 reviewers: devils-advocate, maintainability, agent-compat, ops.)
• No getTelemetryAggregator(): any. TS infers MetricsAggregator | undefined; the issue is missing explicit annotation (F18), not an any return.
• No createHash('sha256') / connectionId hashing anywhere in this PR — the brief was wrong on this point.
• HTTPS-only telemetry URL with comprehensive SSRF deny-list for IMDS/loopback/RFC1918/link-local is solid.
• func-names: off in tests is legitimate for the Mocha function () { this.timeout(...) } idiom.
• Failure isolation: telemetry never blocks the data path; safeEmit swallows; withErrorTelemetry re-throws after emit. Strong (multiple reviewers verified).
• Bearer <token> redaction works on space-separated form (devils-advocate's H8 was wrong on this — the regex is correct).
• TelemetryEventEmitter and TelemetryClientProvider test rewrites are coverage supersets, not abandonment.
• Per-host shared singleton aggregator + 1 timer/host is well-engineered for scalability; performance review found no regressions on the data path.
• Memory bounds (maxPendingMetrics, maxErrorsPerStatement, maxStatementMetrics, statementTtlMs) are correctly enforced at insert/eviction time.
• Circuit breaker is per-host, with synchronous tryAdmit for HALF_OPEN probes (no probe race), counter resets on success.

---

Feedback? Drop it in #code-review-squad-feedback.