feat(csharp): implement DatabricksTelemetryExporter (WI-3.4)

[jadewang-db]

🥞 Stacked PR

Use this link to review incremental changes.

• stack/wi-3.4-databricks-telemetry-exporter [Files changed]
  • stack/wi-3.5-metrics-aggregator [Files changed]
    • stack/wi-3.5-circuit-breaker-manager [Files changed]
      • stack/e2e-feature-flag-cache-tests [Files changed]
        • stack/databricks-activity-listener [Files changed]
          • stack/circuit-breaker-telemetry-exporter [Files changed]
            • stack/telemetry-client-manager-wi-3.2 [Files changed]
              • stack/telemetry-client-wi-5.5 [Files changed]
                • stack/telemetry-client-manager-e2e-wi-7 [Files changed]
                  • stack/telemetry-client-e2e-tests-wi-7-standalone [Files changed]
                  • stack/wi-6.1-databricks-connection-telemetry-integration [Files changed]
                  • stack/wi-6.2-telemetry-tags-driver-activities [Files changed]
                  • stack/wi-9-full-integration-e2e-tests [Files changed]

---

[samikshya-db]

I have left some comments, feel free to merge post addressing them. LGTM otherwise.

[jadewang-db]

Code Review Summary - DatabricksTelemetryExporter (WI-3.4)

Executive Summary

Well-structured implementation of the telemetry exporter following the JDBC reference design. The code demonstrates good separation of concerns, proper error handling, and comprehensive test coverage.

Critical Issues

None identified

High Priority Items

None identified

Medium Priority Suggestions

1. Address existing review comments (various files)

   • samikshya-db's comments about changing endpoint from OSS_JDBC to ADBC (the ADBC endpoint is already configured server-side per the linked proto)
   • Documentation noting first call is blocking
   • Making FeatureFlagsResponse.Flags immutable (IReadOnlyList)

2. FeatureFlagCache singleton disposal (FeatureFlagCache.cs:48)

   • The static singleton s_instance is created eagerly and implements IDisposable but may never be disposed in long-running applications
   • The eviction callback in OnCacheEntryEvicted properly disposes individual contexts, but the cache itself and its SemaphoreSlim may not be cleaned up
   • This is a known pattern limitation and acceptable for singleton caches, but worth documenting

3. FeatureFlagContext Activity.Current in background task (FeatureFlagContext.cs:223-226)

   • Background refresh exceptions log to Activity.Current?.AddEvent(), but Activity.Current may be null in a background task context (no parent activity)
   • Consider using s_activitySource.StartActivity() within the catch block to ensure events are captured

Low Priority / Nits

1. User-Agent verification (HttpClientFactory.cs)

   • As samikshya-db mentioned, verify User-Agent strings pass through DatabricksLogRedactor.scala to avoid log ingestion issues

2. Configuration graceful degradation (TelemetryConfiguration.cs:226-228)

   • Invalid configuration values silently use defaults - this is by design for graceful degradation, but consider adding activity tracing when invalid values are encountered for debugging

Positive Observations

1. Excellent error handling design - ExportAsync correctly never throws exceptions (except OperationCanceledException), following telemetry design principles

2. Good use of ActivitySource - Proper structured tracing using ActivitySource instead of Debug.WriteLine

3. Solid retry logic - Terminal error detection via ExceptionClassifier prevents unnecessary retries on auth failures

4. Thread-safe implementation - Proper use of ConcurrentDictionary and SemaphoreSlim for async locking

5. Comprehensive test coverage - Unit tests cover constructor validation, endpoint selection, request creation, retry behavior, and cancellation

6. Clear documentation - Well-documented interfaces and methods with JDBC references

Code Quality Checklist

• No critical security issues
• Proper error handling
• Thread-safe implementation
• Good test coverage
• Documentation complete
• Follows existing code patterns
• Uses ActivitySource for tracing (not Debug.WriteLine)

Recommendation

Approve with minor changes - The implementation is solid. Please address samikshya-db's existing comments about using the ADBC endpoint and the other nits mentioned in this review.

---

Review generated with assistance from Claude Code

---

This comment was generated with GitHub MCP.

[github-actions]

🚀 Integration tests triggered! View workflow run