refactor(csharp): make ConnectionTelemetry.Create protocol-agnostic (PECO-3022)

[jadewang-db]

🥞 Stacked PR

Use this link to review incremental changes.

• stack/PECO-3022-sea-telemetry-design [Files changed] [MERGED]
  • stack/pr-phase1-connection-telemetry-create-refactor [Files changed]
    • stack/pr-phase2-observer-interface [Files changed]
      • stack/pr-phase3-databricks-statement-observer [Files changed]
        • stack/pr-phase4-sea-connection-telemetry [Files changed]
          • stack/pr-phase5-sea-statement-telemetry [Files changed]

---

Summary

Refactors ConnectionTelemetry.Create so it can serve both transports cleanly — the precondition for wiring SEA telemetry in later phases of this stack:

• Replaces TSessionHandle? sessionHandle with string sessionId. TSessionHandle is a Thrift-only type; SEA only has a string _sessionId. The Thrift call site converts at the boundary (new Guid(SessionHandle.SessionId.Guid).ToString()), keeping the conversion local to Thrift code.
• Adds DriverMode mode parameter. Removes the hardcoded DriverMode.Types.Type.Thrift from BuildDriverConnectionParams/SafeBuildDriverConnectionParams (lines 458, 642 pre-refactor). Without this, SEA records would still be labeled DRIVER_MODE_THRIFT.
• Aligns the driver_name constant (ADBC Databricks Driver) used by both transports so lumberjack dashboards see a single literal regardless of protocol.

Files touched

• csharp/src/Telemetry/ConnectionTelemetry.cs — Create signature change, mode threading, hardcoded Thrift removed.
• csharp/src/DatabricksConnection.cs — Thrift caller converts TSessionHandle → string and passes DriverMode.Thrift.
• csharp/test/Unit/Telemetry/ConnectionTelemetryCreateSignatureTests.cs (new) — covers Create_AcceptsStringSessionId, Create_ThriftMode_SetsDriverModeThrift, Create_SeaMode_SetsDriverModeSea.

Test plan

• All existing telemetry unit tests pass unchanged.
• Existing Thrift integration tests continue to emit driver_connection_params.mode = DRIVER_MODE_THRIFT.
• New unit tests cover string sessionId + both DriverMode values.

Related

• Design: docs: add SEA telemetry integration design (PECO-3022) #455 — docs/designs/PECO-3022-sea-telemetry-integration-design.md §5.2 + §10.
• Stack root: stack/PECO-3022-sea-telemetry-design.

Part of PECO-3022.

[jadewang-db]

Review of PR #460 — ConnectionTelemetry.Create protocol-agnostic refactor

Summary

The refactor itself is mechanically clean — string sessionId + DriverMode mode thread through correctly, hardcoded Thrift literals are removed, and the boundary conversion lives where it should (in the Thrift caller). The XML docs added to Create explain the why of both parameters well.

One High correctness issue: moving the TSessionHandle → string Guid conversion out of ConnectionTelemetry.Create's outer try/catch and into InitializeTelemetry opens a path where a malformed SessionId.Guid byte array would propagate to connection-open instead of degrading to NoOp telemetry. Details inline.

A few Low/Medium test-quality nits — including a test whose name promises a fallback path it doesn't actually exercise, and a misleading test name (ReturnsNullConnectionTelemetry vs. the singleton). Also inline.

Positives worth keeping

• Per-test distinct hosts to side-step the TelemetryClientManager per-host singleton cache — exactly the kind of subtle isolation note future readers benefit from.
• Both DriverMode enum values (Thrift + Sea) are covered, including the new SEA path that doesn't even have a caller in this PR yet — that's the right place to pin behavior.
• Comments reference the prior line numbers (ConnectionTelemetry.cs:642) so reviewers can verify the literal really moved. Nice forensic detail.

Scope / commit hygiene

Commit d3190ae titled [gap-fill][gap-1] Align driver_name string across SEA and Thrift only adds the regression tests — the actual alignment was done in an earlier commit on main. Suggest retitling to add regression tests pinning driver_name literal so the diff matches the title; otherwise a future bisect on "when did the alignment land" will land here and find no source change.

Overall: approve with one High issue to address before merge.

[jadewang-db]

[High] Fail-open contract regression.

The new Guid(SessionHandle.SessionId.Guid) call now lives outside any try/catch. Pre-refactor, this exact conversion was on line ~120 inside ConnectionTelemetry.Create's outer try/catch (the catch at ConnectionTelemetry.cs:150), so any throw degraded to NoOpConnectionTelemetry.Instance and the connection still opened.

new Guid(byte[]) throws ArgumentException when the array is not exactly 16 bytes. The null-guard above only protects against Guid == null, not against a wrong-length array. While the Thrift server should always send a 16-byte payload, the explicit Never throws contract documented two lines above (ConnectionTelemetry.cs:62) is now reachable from InitializeTelemetry callers — a malformed session GUID would fail connection-open, not just disable telemetry.

Fix: wrap the conversion locally:

string sessionId = string.Empty;
try
{
    if (SessionHandle?.SessionId?.Guid != null)
    {
        sessionId = new Guid(SessionHandle.SessionId.Guid).ToString();
    }
}
catch
{
    // Preserve fail-open: malformed session GUID disables telemetry, not the connection.
}

Alternatively, move the conversion back inside Create by accepting byte[]? rawSessionGuid — but then SEA's caller has to construct a fake byte array, which defeats the whole refactor. The local try/catch is the cheaper fix.

[jadewang-db]

Done — wrapped the byte[] -> Guid conversion in a local try/catch (commit 9b31881). A malformed session GUID now degrades to sessionId = string.Empty (which Create maps to SessionId = null), preserving the pre-refactor fail-open guarantee.

[jadewang-db]

[Medium] Test name promises a fallback that isn't actually exercised.

The test passes assemblyVersion: string.Empty expecting that to trip the catch block in SafeBuildSystemConfiguration. But CreateSystemConfiguration at ConnectionTelemetry.cs:512 does:

DriverVersion = assemblyVersion ?? string.Empty,

Empty string is coalesced, not validated — it flows through happily. So _FallbackPath is actually exercising the same code path as _HappyPath above it, just with a different DriverVersion value. The catch block in SafeBuildSystemConfiguration is never reached.

Two ways to fix:

1. Rename to reflect reality: drop _FallbackPath — there's nothing in the catch worth covering separately from the happy path.
2. Actually trigger the fallback — harder with static methods, but BuildSystemConfiguration can throw via the TrySet(() => config.OsArch = RuntimeInformation.OSArchitecture.ToString()); chain on some constrained runtimes. A cleaner approach would be to add an internal overload that takes a fault-injection delegate, or to assert the literal directly on the catch fallback proto using reflection-based invocation of the catch path.

Option 1 is fine — the intent (pin the literal) is already covered by CanonicalConstant_HasExpectedLiteralValue. The fallback path can be tested separately if/when a real fault-injection seam is added.

[jadewang-db]

Done — went with option 1: removed the misleading _FallbackPath test and consolidated the happy path into a single SafeBuildSystemConfiguration_ReturnsCanonicalDriverName (commit 9b31881). Added a comment explaining why we don't separately cover the catch block: there's no fault-injection seam for the static helper, and CanonicalConstant_HasExpectedLiteralValue already pins the literal that both the happy-path and fallback branches reference.

[jadewang-db]

[Low] Test name says ReturnsNull... but the method returns the NoOp singleton, not null.

The assertion two lines down — Assert.Same(NoOpConnectionTelemetry.Instance, telemetry); — confirms the actual contract: a NoOp instance, never null. Suggest renaming to Create_ThrowingHttpClient_ReturnsNoOpConnectionTelemetry. The current name will confuse the next reader who greps for Null thinking we have a null-return path.

[jadewang-db]

Done — renamed to Create_ThrowingHttpClient_ReturnsNoOpConnectionTelemetry (commit 9b31881).

[jadewang-db]

[Low] Brittle test — depends on the current implementation of HttpClientFactory / TelemetryClientManager throwing on empty host.

The comment is honest about the mechanism (new Uri("https://") throws), but that's two layers down from the unit under test. If HttpClientFactory.CreateTelemetryHttpClient or TelemetryClientManager.GetOrCreateClient later gains an early defensive check that returns gracefully on empty host, this test would still pass — but it would no longer be testing what its name suggests. It'd be a silent non-test.

Suggestion (non-blocking): add a one-line Assert.True(string.IsNullOrEmpty(host), "This test relies on empty host tripping the inner URI/client construction") near the top, or pull the fault-injection seam up (an internal overload of Create that accepts the HttpClient) so the throw point is explicit in the test rather than implicit in transitive dependencies.

[jadewang-db]

Done — added an ASSUMPTION: paragraph to the test's comment block that documents the implicit dependency on HttpClientFactory / TelemetryClientManager throwing on empty host, with explicit guidance for the future case where they get defensive handling and this test silently becomes a non-test (commit 9b31881). Left the underlying mechanism as-is for now — a real fault-injection seam can wait until a second consumer needs it.

[jadewang-db]

[Low/Suggestion] Coverage gap: no test covering the string.Empty → SessionId = null mapping.

ConnectionTelemetry.cs:133 now does:

SessionId = !string.IsNullOrEmpty(sessionId) ? sessionId : null,

This intentionally maps empty string to null SessionContext.SessionId so callers that don't yet have a session (e.g., InitializeTelemetry called before OpenSession returns a handle) get null in the telemetry payload, not an empty literal. That's good behavior — but no test pins it. Suggest adding:

[Fact]
public async Task Create_EmptySessionId_MapsToNullInContext()
{
    var telemetry = ConnectionTelemetry.Create(
        ..., sessionId: string.Empty, mode: DriverModeType.Thrift, ...);
    try { Assert.Null(telemetry.Session?.SessionId); }
    finally { await telemetry.DisposeAsync(); }
}

Without this, a refactor that drops the !string.IsNullOrEmpty guard (and starts emitting empty-string SessionId to lumberjack) would not be caught by any test.

[jadewang-db]

Done — added Create_EmptySessionId_MapsToNullInContext (commit 9b31881). It calls Create with sessionId: string.Empty and asserts telemetry.Session.SessionId is null, pinning the mapping at ConnectionTelemetry.cs:133. Test passes locally (5/5 in ConnectionTelemetryCreateSignatureTests).