diff --git a/csharp/doc/sprint-plan-PECO-3022-sea-telemetry-2026-05-14.md b/csharp/doc/sprint-plan-PECO-3022-sea-telemetry-2026-05-14.md new file mode 100644 index 00000000..99bde1d5 --- /dev/null +++ b/csharp/doc/sprint-plan-PECO-3022-sea-telemetry-2026-05-14.md @@ -0,0 +1,236 @@ + + +# Sprint Plan — PECO-3022 SEA Telemetry Integration + +**Sprint window:** 2026-05-14 → 2026-05-28 (2 weeks) +**Implementer:** Jade Wang (sole) +**Design doc:** [`docs/designs/PECO-3022-sea-telemetry-integration-design.md`](../../docs/designs/PECO-3022-sea-telemetry-integration-design.md) +**Design PR:** https://github.com/adbc-drivers/databricks/pull/455 +**Jira:** [PECO-3022](https://databricks.atlassian.net/browse/PECO-3022) + +--- + +## Sprint Goal + +Ship end-to-end SEA client telemetry to production parity with Thrift — connection session events, per-statement operation events, error events, chunk metrics — verified against a real SQL warehouse. Includes the mechanical refactor of `DatabricksStatement` to consume the new observer interface so both transports use the same telemetry seam. + +### Success criteria + +- A statement executed via `adbc.databricks.protocol=rest` emits a `OssSqlDriverTelemetryLog` carrying `driver_connection_params.mode = DRIVER_MODE_SEA`, populated session id, statement id, operation latency, result format, poll count, first-batch and consumed latencies. +- Error in a SEA statement produces an `error_info` record with `error_name` populated. +- Thrift telemetry output is byte-identical to current main (regression-tested). +- SEA telemetry visible in `eng_lumberjack.prod_frontend_log_sql_driver_log` after sprint deploys. + +### Dependency + +- The gap-fix workstream's `CloudFetchDownloader → ChunkMetrics → CloudFetchReader.GetChunkMetrics()` plumbing. If it lands in-sprint, wire it in. If not, ship with `ChunkMetrics.Empty` and backfill in a follow-up. + +--- + +## Task Breakdown (7 tasks, ~11.5 person-days) + +### T1 — Refactor `ConnectionTelemetry.Create` signature (1 day) + +Replace `TSessionHandle? sessionHandle` with `string sessionId`. Add `DriverMode.Types.Type mode` parameter. Remove the hardcoded `DriverMode.Types.Type.Thrift` at `ConnectionTelemetry.cs:458` and `:642`. + +**Files touched:** +- `csharp/src/Telemetry/ConnectionTelemetry.cs` +- `csharp/src/DatabricksConnection.cs` (single Thrift call site, convert `sessionHandle.SessionId.Guid.ToString()` at boundary) + +**Acceptance criteria:** +- All existing telemetry unit tests pass unchanged. +- Thrift integration test emits `driver_connection_params.mode = DRIVER_MODE_THRIFT` (regression check). +- New unit test: `Create_AcceptsStringSessionId`, `Create_ThriftMode_SetsDriverModeThrift`, `Create_SeaMode_SetsDriverModeSea`. + +**Risks:** Low. Mechanical refactor with one Thrift caller to update. + +--- + +### T2 — Introduce `IStatementOperationObserver` + impls (2 days) + +Create the interface and three implementations per design §5.1 and §12. + +**New files:** +- `csharp/src/Telemetry/IStatementOperationObserver.cs` +- `csharp/src/Telemetry/TelemetryObserver.cs` (uses `Safe(Action)` helper pattern from design §12) +- `csharp/src/Telemetry/NullObserver.cs` (singleton) +- `csharp/src/Telemetry/SafeObserver.cs` (decorator) + +**Acceptance criteria:** +- Interface contract documented: methods MUST NOT throw, thread-safe, `OnFinalized` is terminal and idempotent. +- `TelemetryObserver` writes into a `StatementTelemetryContext`; on `OnFinalized` builds `OssSqlDriverTelemetryLog` and enqueues via `IConnectionTelemetry`. +- Unit tests per design §15: + - `NullObserver_AllMethods_AreNoOps` / `NullObserver_IsSingleton` + - `TelemetryObserver_OnExecuteStarted_PopulatesContext` + - `TelemetryObserver_OnExecuteSucceeded_RecordsStatementId` + - `TelemetryObserver_OnFinalized_EnqueuesExactlyOnce` + - `TelemetryObserver_OnFinalized_CalledTwice_EnqueuesOnce` + - `TelemetryObserver_OnError_RecordsErrorAndFinalizes` + - `TelemetryObserver_AllMethods_NeverThrow_WhenContextCorrupted` + - `TelemetryObserver_OnChunksDownloaded_MergesIntoChunkDetails` + - `SafeObserver_PropagatesNormalCallsToInner` + - `SafeObserver_SwallowsExceptionsFromInner_LogsAtTrace` + +**Risks:** Low. New code, no existing callers yet. + +--- + +### T3 — Add `SeaResultFormatMapper` (1 day) + +Static helper that maps wire `disposition` × manifest state → proto `ExecutionResult.Format`. Per design §8. + +**New files:** +- `csharp/src/StatementExecution/SeaResultFormatMapper.cs` + +**Acceptance criteria:** +- Unit tests covering all four cells in §8 table: + - `Map_InlineDisposition_ReturnsInlineArrow` + - `Map_ExternalLinksDisposition_ReturnsExternalLinks` + - `Map_AutoDisposition_WithInlineResult_ReturnsInlineArrow` + - `Map_AutoDisposition_WithExternalLinks_ReturnsExternalLinks` + +**Risks:** Low. Isolated pure-function helper. + +--- + +### T4 — Refactor `DatabricksStatement` to use observer (1 day) + +Mechanical refactor: replace the private telemetry methods (`CreateTelemetryContext`, `CreateMetadataTelemetryContext`, `RecordSuccess`, `RecordError`, `EmitTelemetry`) with `_observer: IStatementOperationObserver` field calls. Behavior unchanged. + +**Files touched:** +- `csharp/src/DatabricksStatement.cs` + +**Acceptance criteria:** +- All existing Thrift telemetry unit tests pass unchanged. +- Manual diff check: byte-equivalent `OssSqlDriverTelemetryLog` for a known statement before/after the refactor. +- `((DatabricksConnection)Connection).TelemetrySession` cast eliminated; observer is injected at statement construction from `DatabricksConnection.CreateStatement()`. + +**Risks:** Medium. The refactor is mechanical but the existing Thrift test suite is the safety net. Allocate buffer time for any subtle behavior differences (e.g. `PendingTelemetryContext` exposure used by external callers). + +**Depends on:** T1 (Create signature), T2 (observer types). + +--- + +### T5 — Wire telemetry into `StatementExecutionConnection` (1.5 days) + +Mirror the Thrift pattern at `DatabricksConnection.cs:594-724`. Add `_telemetry: IConnectionTelemetry` field. Call `ConnectionTelemetry.Create(...)` in `OpenAsync` after `CreateSessionAsync` succeeds, emit `CREATE_SESSION` event, then on `Dispose` emit `DELETE_SESSION` and run `DisposeAsync` with 5-second timeout. + +**Files touched:** +- `csharp/src/StatementExecution/StatementExecutionConnection.cs` + +**Acceptance criteria:** +- `OpenAsync` succeeds even if telemetry initialization throws (telemetry is fail-open; falls back to `NullConnectionTelemetry`). +- `Dispose` completes within 5 seconds even if exporter is wedged. +- Observer is created in `CreateStatement()` using `_telemetry.Session`; falls back to `NullObserver.Instance` if telemetry is disabled or `Session` is null. +- Manual test: open + close a REST connection, verify `CREATE_SESSION` and `DELETE_SESSION` records arrive in lumberjack. + +**Risks:** Medium. New telemetry surface on a class that has never had it. Watch for null-handling around `_telemetry` and `Session`. + +**Depends on:** T1 (Create signature). + +--- + +### T6 — Wire telemetry into `StatementExecutionStatement` (3 days) + +The meatiest task. Add `_observer: IStatementOperationObserver` field (defaults to `NullObserver.Instance`, set by `StatementExecutionConnection.CreateStatement`). Call observer methods at all 7 hookpoints per design §6: + +1. `OnExecuteStarted` — `ExecuteQueryInternalAsync` before `_client.ExecuteStatementAsync` (line 345) +2. `OnExecuteSucceeded` — after response received, using `SeaResultFormatMapper` +3. `OnPollCompleted` — in `PollUntilCompleteAsync` (line 453), accumulate count/ms across the loop, emit once on terminal state +4. `OnFirstBatchReady` — at `CreateCloudFetchReader` (line 542) and `InlineArrowStreamReader` construction (nested at line 900) +5. `OnConsumed` + `OnChunksDownloaded` — at reader Dispose +6. `OnError` — `ExecuteQueryInternalAsync` catch block +7. `OnFinalized` — `Dispose` (line 817) + +**Files touched:** +- `csharp/src/StatementExecution/StatementExecutionStatement.cs` + +**Acceptance criteria:** +- Manual test: execute a SELECT via REST, verify a telemetry record arrives with `statement_id`, `result_format`, `operation_latency_ms`, `poll_count`, `result_set_ready_latency_millis`, `result_set_consumption_latency_millis` populated. +- Manual test: execute a bad SQL via REST, verify `error_info.error_name` populated. +- `OnFinalized` exactly-once even when both error path and dispose path fire. +- `ChunkMetrics`: wire to `OnChunksDownloaded` if gap-fix plumbing is available, else pass `ChunkMetrics.Empty`. + +**Risks:** Medium-high. Largest scope; touches Execute, Poll loop, both reader construction paths, Dispose, and error catch. Highest chance of edge-case regressions. + +**Depends on:** T1, T2, T3. + +--- + +### T7 — SEA integration tests against real SQL warehouse (2 days) + +Mirror the Thrift integration test set per design §15. + +**New files:** +- `csharp/test/E2E/Telemetry/SeaTelemetryIntegrationTests.cs` (or similar) + +**Test cases:** +- `Sea_ExecuteQuery_EmitsTelemetryWithStatementId` +- `Sea_ExecuteQuery_WithSyntaxError_EmitsErrorTelemetry` +- `Sea_ExecuteQuery_CloudFetch_RecordsChunkMetrics` (skipped if gap-fix plumbing not present) +- `Sea_ExecuteQuery_InlineResults_RecordsInlineFormat` +- `Sea_OpenConnection_EmitsCreateSession` +- `Sea_CloseConnection_EmitsDeleteSessionAndFlushes` +- `Sea_TelemetryDisabledByFeatureFlag_EmitsZeroEvents` +- `Sea_TelemetryDisabledByProperty_EmitsZeroEvents` +- `Sea_TelemetryExporterFails_DoesNotAffectQueryExecution` +- `Sea_TelemetryRecord_HasDriverModeSea` +- `Sea_ConcurrentStatements_EachEmitsExactlyOneRecord` + +**Acceptance criteria:** +- All tests pass against a dev/staging Databricks SQL warehouse. +- Test infrastructure verifies records via either a local capture exporter or by querying `eng_lumberjack.prod_frontend_log_sql_driver_log` after a settling delay. + +**Risks:** Medium. Real-warehouse tests are slow and flaky; allocate time for retry/stabilization. + +**Depends on:** T5, T6. + +--- + +## Sequencing + +``` +Week 1 (Mon-Fri): T1 → T2 → T3 → T4 + (T2 and T3 parallelizable if context allows) + +Week 2 (Mon-Fri): T5 → T6 → T7 + (T5 in parallel with start of T6 if discipline holds) +``` + +**Critical path:** T1 → T6 → T7 (≈6 days). +**Slack:** ~1.5 days for review iteration, unexpected edge cases, gap-fix integration if it lands. + +--- + +## Definition of Done + +- All 7 tasks merged to `main`. +- Design PR (#455) approved and merged. +- SEA telemetry records visible in `eng_lumberjack.prod_frontend_log_sql_driver_log` via the [client-telemetry-query](https://databricks.atlassian.net/) skill. +- Thrift telemetry regression test green. +- Sprint demo: show side-by-side Thrift vs SEA telemetry records for the same query. + +--- + +## Risks and Mitigations + +| Risk | Likelihood | Mitigation | +|---|---|---| +| Gap-fix `ChunkMetrics` plumbing slips | Medium | Ship with `ChunkMetrics.Empty`; backfill in follow-up sprint | +| `DatabricksStatement` refactor (T4) hits subtle regression | Medium | Cross-transport byte-identical regression test in T1, dry-run in T4 | +| SEA integration tests flaky in CI | Medium | Tag as `[Trait("Category", "Integration")]`; run on-demand initially | +| Sprint overflow (11.5d est in 10d capacity) | High | T7 can slip to follow-up sprint if T5/T6 take longer than estimated; foundation is the priority | diff --git a/csharp/src/DatabricksConnection.cs b/csharp/src/DatabricksConnection.cs index 5ec2ecbf..d2a1cbeb 100644 --- a/csharp/src/DatabricksConnection.cs +++ b/csharp/src/DatabricksConnection.cs @@ -727,12 +727,45 @@ internal IConnectionTelemetry TelemetryForTesting /// private void InitializeTelemetry(Activity? activity = null) { + // Convert TSessionHandle -> string at the transport boundary so + // ConnectionTelemetry.Create stays transport-agnostic. SEA will pass its + // server-assigned session id string directly. + // + // Wrap the byte[] -> Guid conversion locally: `new Guid(byte[])` throws + // ArgumentException on a wrong-length array, and that must not propagate + // to connection-open. + // + // Behavior on conversion failure: sessionId stays empty, and + // ConnectionTelemetry.Create maps that to SessionId = null on a live + // TelemetrySessionContext (see ConnectionTelemetry.cs ~L133). Telemetry + // remains enabled — only the session-id field is unset. This is a small, + // deliberate behavior change from pre-refactor, where the same conversion + // sat inside Create's outer try/catch and a bad GUID returned the NoOp + // telemetry instance. Both paths keep the *connection* fail-open; the new + // path additionally keeps telemetry on so we still emit driver_connection_params, + // driver_system_configuration, and error events for the affected session. + string sessionId = string.Empty; + try + { + if (SessionHandle?.SessionId?.Guid != null) + { + sessionId = new Guid(SessionHandle.SessionId.Guid).ToString(); + } + } + catch + { + // Intentionally swallowed. Leaves sessionId = string.Empty, which + // Create maps to SessionId = null on a live ConnectionTelemetry. + // See block comment above for the deliberate behavior choice. + } + _telemetry = Telemetry.ConnectionTelemetry.Create( properties: Properties, host: GetHost(), assemblyVersion: s_assemblyVersion, oauthTokenProvider: _oauthTokenProvider, - sessionHandle: SessionHandle, + sessionId: sessionId, + mode: Telemetry.Proto.DriverMode.Types.Type.Thrift, enableDirectResults: _enableDirectResults, useDescTableExtended: _useDescTableExtended, connectTimeoutMilliseconds: ConnectTimeoutMilliseconds, diff --git a/csharp/src/Telemetry/ConnectionTelemetry.cs b/csharp/src/Telemetry/ConnectionTelemetry.cs index 00f87576..a32f950b 100644 --- a/csharp/src/Telemetry/ConnectionTelemetry.cs +++ b/csharp/src/Telemetry/ConnectionTelemetry.cs @@ -29,7 +29,6 @@ using AdbcDrivers.HiveServer2; using AdbcDrivers.HiveServer2.Spark; using Apache.Arrow.Adbc; -using Apache.Hive.Service.Rpc.Thrift; namespace AdbcDrivers.Databricks.Telemetry { @@ -62,12 +61,22 @@ internal ConnectionTelemetry( /// Returns if telemetry is disabled, misconfigured, or fails to initialize. /// Never throws. /// + /// + /// The transport-agnostic session id (a GUID string for Thrift, server-assigned id for SEA). + /// Callers convert at the boundary so this method has no transport-specific dependency. + /// + /// + /// Driver transport mode (THRIFT or SEA) stamped onto + /// driver_connection_params.mode. Threaded through from the caller so the + /// telemetry payload reflects the real transport. + /// public static IConnectionTelemetry Create( IReadOnlyDictionary properties, string host, string assemblyVersion, OAuthClientCredentialsProvider? oauthTokenProvider, - TSessionHandle? sessionHandle, + string sessionId, + Proto.DriverMode.Types.Type mode, bool enableDirectResults, bool useDescTableExtended, int connectTimeoutMilliseconds, @@ -115,15 +124,13 @@ public static IConnectionTelemetry Create( SafeBuildSystemConfiguration(assemblyVersion, activity); Proto.DriverConnectionParameters driverConnectionParams = SafeBuildDriverConnectionParams( - properties, host, enableDirectResults, useDescTableExtended, + properties, host, mode, enableDirectResults, useDescTableExtended, connectTimeoutMilliseconds, activity); string authType = SafeDetermineAuthType(properties, activity); var session = new TelemetrySessionContext { - SessionId = sessionHandle?.SessionId?.Guid != null - ? new System.Guid(sessionHandle.SessionId.Guid).ToString() - : null, + SessionId = !string.IsNullOrEmpty(sessionId) ? sessionId : null, TelemetryClient = telemetryClient, SystemConfiguration = systemConfiguration, DriverConnectionParams = driverConnectionParams, @@ -430,6 +437,7 @@ internal static Proto.DriverSystemConfiguration SafeBuildSystemConfiguration( internal static Proto.DriverConnectionParameters SafeBuildDriverConnectionParams( IReadOnlyDictionary properties, string host, + Proto.DriverMode.Types.Type mode, bool enableDirectResults, bool useDescTableExtended, int connectTimeoutMilliseconds, @@ -438,7 +446,7 @@ internal static Proto.DriverConnectionParameters SafeBuildDriverConnectionParams try { return BuildDriverConnectionParams( - properties, host, enableDirectResults, useDescTableExtended, + properties, host, mode, enableDirectResults, useDescTableExtended, connectTimeoutMilliseconds); } catch (Exception ex) @@ -455,7 +463,7 @@ internal static Proto.DriverConnectionParameters SafeBuildDriverConnectionParams return new Proto.DriverConnectionParameters { HttpPath = string.Empty, - Mode = Proto.DriverMode.Types.Type.Thrift, + Mode = mode, HostInfo = new Proto.HostDetails { HostUrl = host ?? string.Empty, @@ -624,6 +632,7 @@ internal static string UnquoteOsReleaseValue(string raw) internal static Proto.DriverConnectionParameters BuildDriverConnectionParams( IReadOnlyDictionary properties, string host, + Proto.DriverMode.Types.Type mode, bool enableDirectResults, bool useDescTableExtended, int connectTimeoutMilliseconds) @@ -639,7 +648,7 @@ internal static Proto.DriverConnectionParameters BuildDriverConnectionParams( var connectionParams = new Proto.DriverConnectionParameters { HttpPath = httpPath ?? "", - Mode = Proto.DriverMode.Types.Type.Thrift, + Mode = mode, HostInfo = new Proto.HostDetails { // Bare hostname, matching JDBC. Scheme is implicit (always https) and diff --git a/csharp/test/Unit/Telemetry/ConnectionTelemetryAuthMechTests.cs b/csharp/test/Unit/Telemetry/ConnectionTelemetryAuthMechTests.cs index 10e7af6a..dbb506f4 100644 --- a/csharp/test/Unit/Telemetry/ConnectionTelemetryAuthMechTests.cs +++ b/csharp/test/Unit/Telemetry/ConnectionTelemetryAuthMechTests.cs @@ -19,6 +19,7 @@ using AdbcDrivers.HiveServer2.Spark; using DriverAuthFlowType = AdbcDrivers.Databricks.Telemetry.Proto.DriverAuthFlow.Types.Type; using DriverAuthMechType = AdbcDrivers.Databricks.Telemetry.Proto.DriverAuthMech.Types.Type; +using DriverModeType = AdbcDrivers.Databricks.Telemetry.Proto.DriverMode.Types.Type; using Xunit; namespace AdbcDrivers.Databricks.Tests.Unit.Telemetry @@ -47,7 +48,8 @@ public void AuthMech_Pat_WhenAuthTypeIsTokenAndNoOAuthGrantType() properties[SparkParameters.Token] = "dapi-redacted"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.Equal(DriverAuthMechType.Pat, connParams.AuthMech); Assert.Equal(DriverAuthFlowType.TokenPassthrough, connParams.AuthFlow); @@ -64,7 +66,8 @@ public void AuthMech_Oauth_ClientCredentials_WhenGrantTypeIsClientCredentials() properties[DatabricksParameters.OAuthClientSecret] = "client-secret"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.Equal(DriverAuthMechType.Oauth, connParams.AuthMech); Assert.Equal(DriverAuthFlowType.ClientCredentials, connParams.AuthFlow); @@ -83,7 +86,8 @@ public void AuthMech_Oauth_TokenPassthrough_WhenAuthTypeIsOauthWithNoGrantType() properties[SparkParameters.AccessToken] = "oauth-access-token-redacted"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.Equal(DriverAuthMechType.Oauth, connParams.AuthMech); Assert.Equal(DriverAuthFlowType.TokenPassthrough, connParams.AuthFlow); @@ -103,7 +107,8 @@ public void AuthMech_Oauth_TokenPassthrough_WhenGrantTypeIsAccessToken() properties[SparkParameters.AccessToken] = "oauth-access-token-redacted"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.Equal(DriverAuthMechType.Oauth, connParams.AuthMech); Assert.Equal(DriverAuthFlowType.TokenPassthrough, connParams.AuthFlow); @@ -115,7 +120,8 @@ public void AuthMech_Pat_WhenNoAuthConfigured() var properties = BaseProperties(); var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.Equal(DriverAuthMechType.Pat, connParams.AuthMech); Assert.Equal(DriverAuthFlowType.TokenPassthrough, connParams.AuthFlow); diff --git a/csharp/test/Unit/Telemetry/ConnectionTelemetryCreateSignatureTests.cs b/csharp/test/Unit/Telemetry/ConnectionTelemetryCreateSignatureTests.cs new file mode 100644 index 00000000..ccf5d1a5 --- /dev/null +++ b/csharp/test/Unit/Telemetry/ConnectionTelemetryCreateSignatureTests.cs @@ -0,0 +1,216 @@ +/* +* Copyright (c) 2025 ADBC Drivers Contributors +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using System.Collections.Generic; +using System.Threading.Tasks; +using AdbcDrivers.Databricks.Telemetry; +using AdbcDrivers.HiveServer2.Spark; +using DriverModeType = AdbcDrivers.Databricks.Telemetry.Proto.DriverMode.Types.Type; +using Xunit; + +namespace AdbcDrivers.Databricks.Tests.Unit.Telemetry +{ + /// + /// Tests for PECO-3022 (TELEM/SEA T1): + /// is now transport-agnostic — it takes a string sessionId (converted at the + /// caller's boundary) and a DriverMode.Types.Type mode threaded through to + /// driver_connection_params.mode. The two formerly hardcoded + /// DriverMode.Types.Type.Thrift literals in BuildDriverConnectionParams + /// and the fallback in SafeBuildDriverConnectionParams are gone; the mode is + /// always the value supplied by the caller (THRIFT for Thrift, SEA for the upcoming + /// SEA transport). + /// + public class ConnectionTelemetryCreateSignatureTests + { + private const string AssemblyVersion = "1.2.3-test"; + private const int DefaultTimeoutMs = 30_000; + + private static IReadOnlyDictionary TelemetryEnabledProperties() => + new Dictionary + { + { TelemetryConfiguration.PropertyKeyEnabled, "true" }, + { SparkParameters.AuthType, SparkAuthTypeConstants.Token }, + { SparkParameters.Token, "dapi-redacted" }, + { SparkParameters.Path, "/sql/1.0/warehouses/abc123" }, + }; + + // Tests share a TelemetryClient cache keyed by host (TelemetryClientManager + // singleton). Use distinct hosts per test to keep them isolated. + [Fact] + public async Task Create_AcceptsStringSessionId() + { + // Regression: the original signature took TSessionHandle?, forcing the + // (Thrift) caller to leak its transport handle through telemetry. The new + // signature accepts the already-stringified id so the SEA caller can pass + // its server-assigned id without inventing a fake TSessionHandle. + const string Host = "create-string-sid.databricks.com"; + const string SessionId = "9e6a3f88-1234-4321-abcd-deadbeefcafe"; + + IConnectionTelemetry telemetry = ConnectionTelemetry.Create( + properties: TelemetryEnabledProperties(), + host: Host, + assemblyVersion: AssemblyVersion, + oauthTokenProvider: null, + sessionId: SessionId, + mode: DriverModeType.Thrift, + enableDirectResults: true, + useDescTableExtended: false, + connectTimeoutMilliseconds: DefaultTimeoutMs, + activity: null); + + try + { + Assert.NotNull(telemetry.Session); + Assert.Equal(SessionId, telemetry.Session!.SessionId); + } + finally + { + await telemetry.DisposeAsync(); + } + } + + [Fact] + public async Task Create_EmptySessionId_MapsToNullInContext() + { + // ConnectionTelemetry.Create maps `string.Empty` -> `SessionId = null` in the + // resulting TelemetrySessionContext. This matters because Create is called + // from InitializeTelemetry before OpenSession returns a real handle on some + // code paths, and the DatabricksConnection caller passes string.Empty rather + // than null in that window. Pin the mapping so a future refactor that drops + // the `!string.IsNullOrEmpty` guard at ConnectionTelemetry.cs would surface + // here, rather than silently emitting empty-string SessionId to lumberjack. + const string Host = "create-empty-sid.databricks.com"; + + IConnectionTelemetry telemetry = ConnectionTelemetry.Create( + properties: TelemetryEnabledProperties(), + host: Host, + assemblyVersion: AssemblyVersion, + oauthTokenProvider: null, + sessionId: string.Empty, + mode: DriverModeType.Thrift, + enableDirectResults: true, + useDescTableExtended: false, + connectTimeoutMilliseconds: DefaultTimeoutMs, + activity: null); + + try + { + Assert.NotNull(telemetry.Session); + Assert.Null(telemetry.Session!.SessionId); + } + finally + { + await telemetry.DisposeAsync(); + } + } + + [Fact] + public async Task Create_ThriftMode_SetsDriverModeThrift() + { + // Regression for the literal that used to live at ConnectionTelemetry.cs:642 + // — `Mode = DriverMode.Types.Type.Thrift` is now threaded from the caller. + const string Host = "create-thrift-mode.databricks.com"; + + IConnectionTelemetry telemetry = ConnectionTelemetry.Create( + properties: TelemetryEnabledProperties(), + host: Host, + assemblyVersion: AssemblyVersion, + oauthTokenProvider: null, + sessionId: "session-thrift", + mode: DriverModeType.Thrift, + enableDirectResults: true, + useDescTableExtended: false, + connectTimeoutMilliseconds: DefaultTimeoutMs, + activity: null); + + try + { + Assert.NotNull(telemetry.Session); + Assert.NotNull(telemetry.Session!.DriverConnectionParams); + Assert.Equal(DriverModeType.Thrift, telemetry.Session.DriverConnectionParams!.Mode); + } + finally + { + await telemetry.DisposeAsync(); + } + } + + [Fact] + public async Task Create_SeaMode_SetsDriverModeSea() + { + // The reason this refactor exists: the SEA telemetry caller (added in a later + // phase) must produce telemetry rows with `driver_connection_params.mode = SEA`. + const string Host = "create-sea-mode.databricks.com"; + + IConnectionTelemetry telemetry = ConnectionTelemetry.Create( + properties: TelemetryEnabledProperties(), + host: Host, + assemblyVersion: AssemblyVersion, + oauthTokenProvider: null, + sessionId: "session-sea", + mode: DriverModeType.Sea, + enableDirectResults: true, + useDescTableExtended: false, + connectTimeoutMilliseconds: DefaultTimeoutMs, + activity: null); + + try + { + Assert.NotNull(telemetry.Session); + Assert.NotNull(telemetry.Session!.DriverConnectionParams); + Assert.Equal(DriverModeType.Sea, telemetry.Session.DriverConnectionParams!.Mode); + } + finally + { + await telemetry.DisposeAsync(); + } + } + + [Fact] + public void Create_ThrowingHttpClient_ReturnsNoOpConnectionTelemetry() + { + // Create() is declared `Never throws`: any initialization failure — HttpClient + // construction, exporter wire-up, etc. — must surface as NoOpConnectionTelemetry + // rather than propagate into the connection-open path. We exercise this by + // enabling telemetry while passing a blank host so `new Uri("https://")` (inside + // HttpClientFactory.CreateTelemetryHttpClient) and/or + // TelemetryClientManager.GetOrCreateClient's argument-check throw, both of + // which land in Create's outer catch. + // + // ASSUMPTION: this test depends on either HttpClientFactory.CreateTelemetryHttpClient + // or TelemetryClientManager.GetOrCreateClient throwing when host is empty. If a + // future change adds defensive handling of empty host upstream of Create's catch, + // this test would silently pass for the wrong reason (Create would return + // NoOpConnectionTelemetry via the disabled/feature-flag path instead of the + // outer catch). When that happens, swap to a real fault-injection seam (e.g., + // an internal overload that accepts a pre-built HttpClient). + IConnectionTelemetry telemetry = ConnectionTelemetry.Create( + properties: TelemetryEnabledProperties(), + host: string.Empty, + assemblyVersion: AssemblyVersion, + oauthTokenProvider: null, + sessionId: "session-throwing-http", + mode: DriverModeType.Thrift, + enableDirectResults: true, + useDescTableExtended: false, + connectTimeoutMilliseconds: DefaultTimeoutMs, + activity: null); + + Assert.Same(NoOpConnectionTelemetry.Instance, telemetry); + Assert.Null(telemetry.Session); + } + } +} diff --git a/csharp/test/Unit/Telemetry/ConnectionTelemetryDiscoveryFieldsTests.cs b/csharp/test/Unit/Telemetry/ConnectionTelemetryDiscoveryFieldsTests.cs index 27889427..9f3e65b0 100644 --- a/csharp/test/Unit/Telemetry/ConnectionTelemetryDiscoveryFieldsTests.cs +++ b/csharp/test/Unit/Telemetry/ConnectionTelemetryDiscoveryFieldsTests.cs @@ -17,6 +17,7 @@ using System.Collections.Generic; using AdbcDrivers.Databricks.Telemetry; using AdbcDrivers.HiveServer2.Spark; +using DriverModeType = AdbcDrivers.Databricks.Telemetry.Proto.DriverMode.Types.Type; using Xunit; namespace AdbcDrivers.Databricks.Tests.Unit.Telemetry @@ -57,7 +58,8 @@ public void DiscoveryModeEnabled_AlwaysFalse_PatAuth() properties[SparkParameters.Token] = "dapi-redacted"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.True(connParams.HasDiscoveryModeEnabled); Assert.False(connParams.DiscoveryModeEnabled); @@ -74,7 +76,8 @@ public void DiscoveryModeEnabled_AlwaysFalse_OAuthClientCredentials() properties[DatabricksParameters.OAuthClientSecret] = "client-secret"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.True(connParams.HasDiscoveryModeEnabled); Assert.False(connParams.DiscoveryModeEnabled); @@ -89,7 +92,8 @@ public void DiscoveryUrl_LeftUnset_NoDiscoverySupported() DatabricksConstants.OAuthGrantTypes.ClientCredentials; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); // The C# driver hardcodes the OIDC token endpoint and never performs // .well-known discovery, so there is no URL to report. Leaving the @@ -105,7 +109,8 @@ public void EnableTokenCache_AlwaysFalse_PatAuth() properties[SparkParameters.Token] = "dapi-redacted"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.True(connParams.HasEnableTokenCache); Assert.False(connParams.EnableTokenCache); @@ -122,7 +127,8 @@ public void EnableTokenCache_AlwaysFalse_OAuthClientCredentials() properties[DatabricksParameters.OAuthClientSecret] = "client-secret"; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.True(connParams.HasEnableTokenCache); Assert.False(connParams.EnableTokenCache); diff --git a/csharp/test/Unit/Telemetry/ConnectionTelemetryDriverNameTests.cs b/csharp/test/Unit/Telemetry/ConnectionTelemetryDriverNameTests.cs new file mode 100644 index 00000000..1e90ac7a --- /dev/null +++ b/csharp/test/Unit/Telemetry/ConnectionTelemetryDriverNameTests.cs @@ -0,0 +1,102 @@ +/* +* Copyright (c) 2025 ADBC Drivers Contributors +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +using AdbcDrivers.Databricks.Telemetry; +using Xunit; + +namespace AdbcDrivers.Databricks.Tests.Unit.Telemetry +{ + /// + /// Regression tests for PECO-3022 B1: driver_name string drift between + /// SEA and Thrift transports. + /// + /// Production lumberjack data from v1.1.4 showed two distinct strings coexisting: + /// + /// "Databricks ADBC Driver" — 685 records, all THRIFT mode + /// "ADBC Databricks Driver" — 4,401 records, mixed THRIFT + 69 SEA + /// + /// Dashboards filtering on the older string silently missed all SEA records and a + /// significant fraction of recent Thrift records. + /// + /// The fix is to make the + /// single source of truth, referenced by both + /// and its fallback so + /// that every caller of — Thrift today via + /// DatabricksConnection, SEA via StatementExecutionConnection — emits + /// the same literal. + /// + /// These tests pin the literal value so that a typo or rename in the constant gets + /// caught at unit-test time before it ships to production. + /// + public class ConnectionTelemetryDriverNameTests + { + /// + /// The canonical driver_name literal that must appear in every telemetry record + /// regardless of transport. Picked because it matches the value already returned + /// via AdbcInfoCode.DriverName (see ) + /// and represented the majority of v1.1.4 production records, so dashboards + /// already keyed on this string see the most history. + /// + private const string CanonicalDriverName = "ADBC Databricks Driver"; + + [Fact] + public void CanonicalConstant_HasExpectedLiteralValue() + { + // Pin the literal. If anyone renames the constant, this test fails and the + // change is forced into review rather than silently breaking downstream + // dashboards that filter on the string. + Assert.Equal(CanonicalDriverName, DatabricksConnection.DatabricksDriverName); + } + + [Fact] + public void BuildSystemConfiguration_ReturnsCanonicalDriverName() + { + var config = ConnectionTelemetry.BuildSystemConfiguration("1.2.3"); + + Assert.Equal(CanonicalDriverName, config.DriverName); + } + + [Fact] + public void SafeBuildSystemConfiguration_ReturnsCanonicalDriverName() + { + // SafeBuildSystemConfiguration delegates to BuildSystemConfiguration on the + // happy path and must return the same canonical literal. The catch-block + // fallback in SafeBuildSystemConfiguration is not exercised here because + // there is no in-process fault-injection seam for the static helper; the + // literal-pin in CanonicalConstant_HasExpectedLiteralValue covers that path + // by construction (both branches reference the same constant). + var config = ConnectionTelemetry.SafeBuildSystemConfiguration("1.2.3", activity: null); + + Assert.NotNull(config); + Assert.Equal(CanonicalDriverName, config.DriverName); + } + + [Fact] + public void DriverName_IdenticalAcrossInvocations_SingleSourceOfTruth() + { + // The single-source-of-truth property: every invocation of the system-config + // builder (regardless of transport mode at the caller) returns the same + // driver_name. Since ConnectionTelemetry.BuildSystemConfiguration is the + // protocol-agnostic factory called by both Thrift and SEA transports, this + // guarantees both Thrift and SEA telemetry records carry identical driver_name. + var thriftCaller = ConnectionTelemetry.BuildSystemConfiguration("1.2.3"); + var seaCaller = ConnectionTelemetry.BuildSystemConfiguration("1.2.3"); + + Assert.Equal(thriftCaller.DriverName, seaCaller.DriverName); + Assert.Equal(CanonicalDriverName, thriftCaller.DriverName); + } + } +} diff --git a/csharp/test/Unit/Telemetry/ConnectionTelemetryPartialInitTests.cs b/csharp/test/Unit/Telemetry/ConnectionTelemetryPartialInitTests.cs index 9431aca5..3075088e 100644 --- a/csharp/test/Unit/Telemetry/ConnectionTelemetryPartialInitTests.cs +++ b/csharp/test/Unit/Telemetry/ConnectionTelemetryPartialInitTests.cs @@ -19,6 +19,7 @@ using AdbcDrivers.HiveServer2.Spark; using DriverAuthFlowType = AdbcDrivers.Databricks.Telemetry.Proto.DriverAuthFlow.Types.Type; using DriverAuthMechType = AdbcDrivers.Databricks.Telemetry.Proto.DriverAuthMech.Types.Type; +using DriverModeType = AdbcDrivers.Databricks.Telemetry.Proto.DriverMode.Types.Type; using Xunit; namespace AdbcDrivers.Databricks.Tests.Unit.Telemetry @@ -60,7 +61,8 @@ public void AuthType_AndAuthMech_Consistent_OauthClientCredentials() }; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); string authType = ConnectionTelemetry.DetermineAuthType(properties); Assert.Equal(DriverAuthMechType.Oauth, connParams.AuthMech); @@ -79,7 +81,8 @@ public void AuthType_AndAuthMech_Consistent_OauthAccessTokenWithGrantType() }; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); string authType = ConnectionTelemetry.DetermineAuthType(properties); Assert.Equal(DriverAuthMechType.Oauth, connParams.AuthMech); @@ -101,7 +104,8 @@ public void AuthType_AndAuthMech_Consistent_OauthAccessTokenPassthrough_NoGrantT }; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); string authType = ConnectionTelemetry.DetermineAuthType(properties); Assert.Equal(DriverAuthMechType.Oauth, connParams.AuthMech); @@ -119,7 +123,8 @@ public void AuthType_AndAuthMech_Consistent_PatToken() }; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); string authType = ConnectionTelemetry.DetermineAuthType(properties); Assert.Equal(DriverAuthMechType.Pat, connParams.AuthMech); @@ -133,7 +138,8 @@ public void AuthType_AndAuthMech_Consistent_NoAuth() var properties = new Dictionary(); var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); string authType = ConnectionTelemetry.DetermineAuthType(properties); Assert.Equal(DriverAuthMechType.Pat, connParams.AuthMech); @@ -186,7 +192,8 @@ public void DriverConnectionParams_ConstantFlags_AlwaysPopulated() }; var connParams = ConnectionTelemetry.BuildDriverConnectionParams( - properties, Host, enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); + properties, Host, DriverModeType.Thrift, + enableDirectResults: true, useDescTableExtended: true, DefaultTimeoutMs); Assert.True(connParams.EnableArrow); Assert.True(connParams.EnableDirectResults); @@ -214,7 +221,7 @@ public void SafeBuildDriverConnectionParams_ReturnsBestEffort_WithMinimalPropert var properties = new Dictionary(); var connParams = ConnectionTelemetry.SafeBuildDriverConnectionParams( - properties, Host, + properties, Host, DriverModeType.Thrift, enableDirectResults: true, useDescTableExtended: false, connectTimeoutMilliseconds: DefaultTimeoutMs, activity: null); @@ -252,7 +259,7 @@ public void EndToEnd_MinimalProperties_AllAlwaysDerivableFieldsPopulated() var systemConfig = ConnectionTelemetry.SafeBuildSystemConfiguration( "1.2.3", activity: null); var connParams = ConnectionTelemetry.SafeBuildDriverConnectionParams( - properties, Host, + properties, Host, DriverModeType.Thrift, enableDirectResults: true, useDescTableExtended: true, connectTimeoutMilliseconds: DefaultTimeoutMs, activity: null); string authType = ConnectionTelemetry.SafeDetermineAuthType(properties, activity: null); diff --git a/docs/designs/PECO-3022-sea-telemetry-integration-design.md b/docs/designs/PECO-3022-sea-telemetry-integration-design.md index 9f02ecb4..2aad2b5c 100644 --- a/docs/designs/PECO-3022-sea-telemetry-integration-design.md +++ b/docs/designs/PECO-3022-sea-telemetry-integration-design.md @@ -255,7 +255,7 @@ public static IConnectionTelemetry Create( IReadOnlyDictionary properties, string host, string assemblyVersion, - IOAuthTokenProvider? oauthTokenProvider, + OAuthClientCredentialsProvider? oauthTokenProvider, string sessionId, // CHANGED: was TSessionHandle? sessionHandle DriverMode.Types.Type mode, // NEW: Thrift or Sea bool enableDirectResults, @@ -264,7 +264,9 @@ public static IConnectionTelemetry Create( Activity? activity); ``` -Thrift caller converts at the boundary: `sessionHandle.SessionId.Guid.ToString()`. SEA caller passes its `_sessionId` directly. +Thrift caller converts at the boundary: `sessionHandle.SessionId.Guid.ToString()` (with a null-check; an empty string is mapped to `null` `SessionId` inside `Create` to match the prior behavior). SEA caller passes its `_sessionId` directly. + +The `mode` parameter is also threaded through `BuildDriverConnectionParams` and `SafeBuildDriverConnectionParams` — both methods previously hardcoded `Mode = DriverMode.Types.Type.Thrift`. The literal is gone from `ConnectionTelemetry.cs`; only the `DatabricksConnection` (Thrift) call site supplies it. ### 5.3 `IConnectionTelemetry` — no surface change