feat(csharp): implement CircuitBreakerManager (WI-3.5)

[jadewang-db]

🥞 Stacked PR

Use this link to review incremental changes.

• 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]

---

[jadewang-db]

Code Review Summary

🔴 Critical Issues (Must Fix)

1. Missing CircuitBreakerConfig class - Code will not compile

The CircuitBreakerManager.cs references CircuitBreakerConfig (lines 39, 50, 58, 104) but this class doesn't exist anywhere in the codebase. Additionally:

• CircuitBreaker class doesn't have a constructor that accepts CircuitBreakerConfig (line 85, 119)
• CircuitBreaker class doesn't have a Config property (referenced in tests at lines 83-85, 248-250, 274, 635-637)

The existing CircuitBreaker constructor signature is:

public CircuitBreaker(int failureThreshold = 5, TimeSpan? timeout = null)

Required fix: Create the CircuitBreakerConfig class and update CircuitBreaker to:

1. Accept CircuitBreakerConfig in its constructor
2. Expose a Config property

---

🟡 Medium Priority Issues

2. Use structured tracing instead of Debug.WriteLine (lines 84, 118, 186, 199)

Per C#/.NET best practices, use ActivitySource and Activity.Current?.AddEvent() for structured tracing instead of Debug.WriteLine. This enables proper distributed tracing and telemetry integration.

Example fix:

// Instead of:
Debug.WriteLine($"[DEBUG] CircuitBreakerManager: Creating circuit breaker for host '{host}'");

// Use:
Activity.Current?.AddEvent(new ActivityEvent("CircuitBreakerCreated", tags: new ActivityTagsCollection
{
    { "host", host }
}));

---

✅ Positive Observations

1. Thread-safe implementation - Correct use of ConcurrentDictionary with GetOrAdd pattern
2. Case-insensitive host comparison - Good use of StringComparer.OrdinalIgnoreCase for hostname handling
3. Comprehensive input validation - Proper null/whitespace checks with descriptive error messages
4. Excellent test coverage - Tests cover singleton behavior, thread safety, per-host isolation, edge cases
5. Well-organized test structure - Good use of regions and AAA pattern
6. Good documentation - XML comments are clear and include JDBC reference

---

📋 Recommendations (Priority Order)

1. [Critical] Add CircuitBreakerConfig class and update CircuitBreaker to support it
2. [Medium] Replace Debug.WriteLine with ActivitySource tracing
3. [Low] Consider adding a SuccessThreshold property validation since it's being tested but not explicitly documented

[jadewang-db]

✅ All issues addressed:

🔴 Critical Issues - FIXED

1. Missing CircuitBreakerConfig class

• Created CircuitBreakerConfig.cs with:
  • FailureThreshold (default: 5)
  • Timeout (default: 1 minute)
  • SuccessThreshold (default: 2)
• Updated CircuitBreaker class to accept CircuitBreakerConfig in constructor
• Added Config property to expose the configuration

🟡 Medium Priority Issues - FIXED

2. Replaced Debug.WriteLine with structured tracing

• Added ActivitySource TraceSource to CircuitBreakerManager
• Replaced all Debug.WriteLine calls with Activity.Current?.AddEvent() using ActivityEvent with proper tags
• Events: CircuitBreakerCreated, CircuitBreakerCreatedWithConfig, CircuitBreakerRemoved, CircuitBreakersCleared

✅ Verification

All 24 CircuitBreaker tests pass.

[samikshya-db]

where is this used

[samikshya-db]

Minor comments, LGTM otherwise