feat(csharp): wire telemetry into connection/statement lifecycle with E2E tests

[jadewang-db]

🥞 Stacked PR

Use this link to review incremental changes.

• stack/itr5-pr-e2e-tests [Files changed]
  • stack/fix-telemetry-gaps-design

---

Summary

• Connection lifecycle: Wire telemetry initialization into DatabricksConnection (feature flag check, per-host client management, session context creation) and graceful disposal with reference counting
• Statement instrumentation: Override ExecuteQuery, ExecuteQueryAsync, ExecuteUpdate, and ExecuteUpdateAsync in DatabricksStatement to capture telemetry context (statement type, result format, compression, statement ID, errors) and emit one telemetry event per statement execution
• Activity tags: Add telemetry-specific tags across connection, polling, composite reader, and CloudFetch components (poll count, result format, chunk count, bytes downloaded, compression)
• HttpClientFactory: Add CreateTelemetryHttpClient using full auth/proxy handler chain for telemetry export
• CapturingTelemetryExporter: Test double that captures exported telemetry logs for assertion without hitting real endpoints
• E2E tests: Comprehensive tests validating all telemetry fields match JDBC driver output, including snake_case proto serialization, statement ID capture, error handling, and performance overhead

Test Plan

• Unit tests for Activity tag definitions
• E2E tests validating all telemetry fields per design doc
• E2E tests for connection lifecycle (init, dispose, multi-connection sharing)
• E2E tests for statement telemetry (query, update, error paths)
• snake_case field name assertions matching proto wire format
• Performance overhead test
• Build passes with 0 warnings, 0 errors (net8.0)

[jadewang-db]

[High] Static mutable ExporterOverride on singleton is a test-safety concern

csharp/src/Telemetry/TelemetryClientManager.cs:55

ExporterOverride is a static mutable field on the singleton TelemetryClientManager. This means:

1. If xUnit runs test classes in parallel, tests will interfere with each other (one test's SetupCapturingExporter() will affect another's)
2. Production code now carries test-only infrastructure

Suggestion: Consider one of:

• Use a non-static instance-level property and inject a test-specific TelemetryClientManager instance (you already have the internal TelemetryClientManager(bool forTesting) constructor)
• At minimum, make the E2E test collection serialized with [Collection("Telemetry")] to prevent parallel execution conflicts
• Document thread-safety expectations in the property doc comment

---

This comment was generated with GitHub MCP.

[jadewang-db]

[High] Blocking async in Dispose risks deadlock

csharp/src/DatabricksConnection.cs:1069

DisposeTelemetryAsync().GetAwaiter().GetResult();

This pattern can deadlock when called from a synchronization context that captures the current thread (e.g., ASP.NET classic, UI threads). While the .ConfigureAwait(false) inside DisposeTelemetryAsync helps, if any awaited method inside the chain doesn't use ConfigureAwait(false), it will deadlock.

Suggestion: Consider implementing IAsyncDisposable alongside IDisposable so callers in async contexts can use await using. The sync Dispose path can remain as a fallback but should be documented with the deadlock risk.

---

This comment was generated with GitHub MCP.

[jadewang-db]

[Medium] Performance test name misleading — asserts 50%, not 1%

csharp/test/E2E/Telemetry/ClientTelemetryE2ETests.cs:925

The test is named Telemetry_PerformanceOverhead_LessThanOnePercent but the actual assertion is:

Assert.True(overheadPercent < 50.0,
    $"Telemetry overhead {overheadPercent:F2}% exceeds acceptable threshold of 50%");

Either rename the test to match the threshold (e.g., Telemetry_PerformanceOverhead_WithinAcceptableRange) or tighten the assertion to match the name.

---

This comment was generated with GitHub MCP.

[jadewang-db]

[Medium] Comment typos — missing /// and // prefixes

csharp/src/DatabricksConnection.cs:1077 and csharp/src/DatabricksConnection.cs:1089

/// Follows the graceful shutdown sequence: flush → release client → release feature flags.

is actually:

/ Follows the graceful shutdown sequence: flush → release client → release feature flags.

And line 1089:

/ Step 1: Flush pending metrics

Both are missing a slash — should be /// and // respectively.

---

This comment was generated with GitHub MCP.

[github-actions]

🚀 Integration tests triggered! View workflow run

[github-actions]

🔒 Integration test approval reset

New commits were pushed to this PR. The integration-test label has been automatically removed for security.

A maintainer must re-review the changes and re-add the label to trigger tests again.

Why is this necessary?

• New code requires fresh security review
• Prevents approved PRs from adding malicious code later
• Ensures all tested code has been explicitly approved

Latest commit: 14a80df

[github-actions]

🚀 Integration tests triggered! View workflow run