feat(csharp): implement telemetry client pipeline with circuit breaker protection

[jadewang-db]

🥞 Stacked PR

Use this link to review incremental changes.

• stack/itr5-pr-telemetry-client-impl [Files changed]
  • stack/itr5-pr-e2e-tests [Files changed]

---

Summary

Implements the telemetry client pipeline for the C# ADBC Databricks driver. This includes the full lifecycle from event ingestion through circuit-breaker-protected export to the Databricks telemetry endpoint.

Key components

• TelemetryClient — Main client coordinating enqueue → flush → export. Uses SemaphoreSlim to gate concurrent flushes, Interlocked counter for O(1) batch-size checks, and a periodic Timer driven by FlushIntervalMs. Graceful shutdown flushes all pending events before disposing.
• TelemetryClientManager — Singleton factory managing one client per host with atomic reference counting (AddRef/Release via Interlocked). Prevents rate-limiting by sharing clients across concurrent connections to the same host.
• TelemetryClientHolder — Holds client + thread-safe ref count using Interlocked.Increment/Decrement.
• CircuitBreakerTelemetryExporter — Wraps the inner exporter with per-host circuit breaker protection (via Polly). Silently drops events when the circuit is open. Never throws — all exceptions including OperationCanceledException are swallowed per the telemetry contract.
• CircuitBreakerManager — Singleton managing per-host circuit breakers with cleanup on last client release.
• ITelemetryClient — Interface defining Enqueue, FlushAsync, CloseAsync, and IAsyncDisposable.
• TelemetrySessionContext / StatementTelemetryContext — Immutable context objects carrying session-level and statement-level telemetry data.

Design decisions

• Thread safety: CloseAsync uses Interlocked.Exchange for atomic dispose check; TelemetryClientHolder uses Interlocked for ref counting; flush is gated by SemaphoreSlim(1,1).
• No data loss on close: CloseAsync performs a final FlushAsync before cancelling the token and disposing resources.
• Telemetry never impacts driver: All exceptions are swallowed; disabled telemetry skips the entire path via _enabled check in Enqueue.
• Memory leak prevention: CircuitBreakerManager.RemoveCircuitBreaker is called when the last client for a host is released.

Test plan

• Unit tests for TelemetryClient (enqueue, flush, close, dispose, error handling)
• Unit tests for TelemetryClientManager (singleton, ref counting, thread safety, multi-host)
• Unit tests for CircuitBreakerTelemetryExporter (circuit states, cancellation swallowed, error tracking)
• Unit tests for TelemetrySessionContext and StatementTelemetryContext
• All 245 telemetry tests pass

🤖 Generated with Claude Code

[jadewang-db]

Code Review Summary — PR #305

Reviewed the incremental changes (10 files, +2435/-16 lines) implementing the telemetry client pipeline: TelemetryClient, TelemetryClientManager, CircuitBreakerManager, CircuitBreakerTelemetryExporter, and associated tests.

Critical Issues (2)

#	File	Issue
1	TelemetryClient.cs:263	Race condition in CloseAsync — _disposed check-then-set is not atomic. Two threads can both proceed past the guard. Use Interlocked.Exchange.
2	TelemetryClient.cs:256	CloseAsync doesn't flush pending logs — docstring and interface contract promise graceful flush, but FlushAsync() is never called. Pending logs are silently lost.

High Issues (1)

#	File	Issue
3	TelemetryClient.cs:167	Fire-and-forget Task.Run for flush — concurrent flushes race on TryDequeue, causing duplicate partial exports. Unobserved exceptions. No backpressure.

Medium Issues (4)

#	File	Issue
4	TelemetryClient.cs:59	_cts unused — created and disposed but never passed to any operation. Dead code.
5	TelemetryClientHolder.cs:33	Misleading doc — says "Interlocked" but actual access uses lock.
6	CircuitBreakerTelemetryExporter.cs:120	TOCTOU race — pre-check of circuit state is redundant with Polly's BrokenCircuitException.
7	CircuitBreakerManager.cs:46	Memory leak — circuit breakers accumulate forever, never removed.

Low Issues (2)

#	File	Issue
8	TelemetryClientManager.cs:62	Unused forTesting parameter
9	TelemetryClientManagerTests.cs:146	HttpClient leaks in tests

Positive Observations

• Clean separation of concerns: exporter → circuit breaker → client → manager hierarchy
• Good per-host isolation design for circuit breakers and client sharing
• Circuit breaker tests are thorough with good coverage of state transitions
• Proper use of ActivitySource for structured tracing
• Reference counting in TelemetryClientManager is a sound design for shared resources

Recommendations (prioritized)

1. Fix CloseAsync: Make it async, call FlushAsync() before cancellation, use Interlocked for _disposed
2. Fix flush concurrency: Add a SemaphoreSlim(1,1) or use a dedicated background flush task
3. Either use or remove _cts: If keeping it, create linked tokens in FlushAsync
4. Add RemoveCircuitBreaker to CircuitBreakerManager to prevent unbounded growth
5. Remove redundant TOCTOU check in CircuitBreakerTelemetryExporter.ExportAsync

---

This comment was generated with GitHub MCP.

[github-actions]

🚀 Integration tests triggered! View workflow run