refactor(csharp): route DatabricksStatement telemetry through IStatementOperationObserver (PECO-3022) - #462
Draft
jadewang-db wants to merge 9 commits into
Draft
Conversation
This was referenced May 19, 2026
jadewang-db
changed the title
added 9 commits
May 22, 2026 20:06
…erMode\n\nTask ID: task-1.1-refactor-connection-telemetry-create
- High: preserve fail-open contract — wrap the TSessionHandle Guid byte[] conversion in InitializeTelemetry with try/catch. Pre-refactor the same conversion lived inside ConnectionTelemetry.Create's outer try/catch so a malformed session GUID degraded to NoOp telemetry; moving it to the transport boundary lost that guarantee. - Medium: remove SafeBuildSystemConfiguration_..._FallbackPath — it passed string.Empty for assemblyVersion expecting a throw, but CreateSystemConfiguration coalesces empty string. The catch block is never reached. The CanonicalConstant_HasExpectedLiteralValue test already pins both branches by construction. - Low: rename Create_ThrowingHttpClient_ReturnsNullConnectionTelemetry to ...ReturnsNoOpConnectionTelemetry — actual return is the NoOp singleton. - Low: document the test's implicit assumption that the empty-host throws inside HttpClientFactory/TelemetryClientManager so future defensive handling there doesn't silently turn this into a non-test. - Low: add Create_EmptySessionId_MapsToNullInContext to pin the string.Empty -> null SessionId mapping at ConnectionTelemetry.cs:133. Co-authored-by: Isaac
…on\n\nTask ID: task-2.1-observer-interface-and-null-impl
…ryContext + enqueue\n\nTask ID: task-2.2-telemetry-observer-impl
…server\n\nTask ID: task-2.3-safe-observer-decorator
…\n\nTask ID: task-3.1-refactor-databricks-statement-observer
…elemetryContext downcast
jadewang-db
force-pushed
the
stack/pr-phase3-databricks-statement-observer
branch
from
May 22, 2026 20:07
fa799fc to
bc5e031
Compare
CurtHagenlocher
pushed a commit
to CurtHagenlocher/databricks
that referenced
this pull request
May 24, 2026
) ## 🥞 Stacked PR Use this [link](https://github.com/adbc-drivers/databricks/pull/455/files) to review incremental changes. - [**stack/PECO-3022-sea-telemetry-design**](adbc-drivers#455) [[Files changed](https://github.com/adbc-drivers/databricks/pull/455/files)] - [stack/pr-phase1-connection-telemetry-create-refactor](adbc-drivers#460) [[Files changed](https://github.com/adbc-drivers/databricks/pull/460/files/c0dfbed80fd09c91b9e2e5ed8050a268435618bd..d3190aeb6f2f1c727b359d0ef40d26be2280c73e)] - [stack/pr-phase2-observer-interface](adbc-drivers#461) [[Files changed](https://github.com/adbc-drivers/databricks/pull/461/files/b2340d9d32da68d5b81756e0a77008f05aadd45b..b0d9f02236bf7f99e93132cfe6ed4dd119fc1e73)] - [stack/pr-phase3-databricks-statement-observer](adbc-drivers#462) [[Files changed](https://github.com/adbc-drivers/databricks/pull/462/files/b0d9f02236bf7f99e93132cfe6ed4dd119fc1e73..fa799fcd0cc2209982eae2890e16e26854a0649e)] - [stack/pr-phase4-sea-connection-telemetry](adbc-drivers#463) [[Files changed](https://github.com/adbc-drivers/databricks/pull/463/files/f0e344aa4d9d341302ee9b3a5217a6794d8ba189..25b5e6eaf2f2f04941d07bbbe582845254630950)] - [stack/pr-phase5-sea-statement-telemetry](adbc-drivers#464) [[Files changed](https://github.com/adbc-drivers/databricks/pull/464/files/9dc278a249433000b03e2d3a4cdf7daa151caa69..8109047005a89804f243beb3a6c689f43b539506)] --------- ## Summary of the design This design closes the gap where the SEA (REST) transport in the C# ADBC Databricks driver emits **zero** client telemetry — no session events, no per-statement operation events, no error events, no chunk metrics. SEA traffic is currently invisible to the `eng_lumberjack` driver-telemetry pipeline. The design: - Introduces a small **observer interface** (`IStatementOperationObserver`) between statement classes and the telemetry implementation. The interface is shaped around the statement's lifecycle (`OnExecuteStarted`, `OnPollCompleted`, …) rather than telemetry's data model. Contract: fail-open — implementations must not throw. - **Refactors `ConnectionTelemetry.Create`** to accept `string sessionId` (dropping its Thrift `TSessionHandle` coupling) and a `DriverMode mode` parameter. Both transports use the same factory. - Wires observer callbacks at the SEA hookpoints in `StatementExecutionConnection` and `StatementExecutionStatement`. - **Reuses all existing telemetry infrastructure as-is**: `TelemetryClient`, `TelemetryClientManager`, `CircuitBreakerTelemetryExporter`, `DatabricksTelemetryExporter`, `FeatureFlagCache`, `TelemetrySessionContext`, `StatementTelemetryContext`. ## Key decisions and alternatives considered - **Observer interface over static helper or instance emitter.** The gap-fix doc proposed a static `TelemetryHelper`. This design supersedes that with an observer-shaped interface because: (a) it gives one-line callsites with no parameter threading, (b) it decouples statement classes from telemetry types so future tracing/audit observers can plug in without touching statement code, (c) it's trivially mockable for tests. The fail-open contract pushes all try/catch into the implementations exactly once — callsites stay clean. - **Composition, not inheritance.** Considered (and rejected) three inheritance variants: SEA-inherits-Thrift (drags in entire HiveServer2 chain — semantically wrong), shared base above both (blocked structurally — `HiveServer2Connection`'s parent is in the Apache package we don't own), and interface + extension methods (functionally identical to the static helper). C# single inheritance + unowned Apache base classes make pure inheritance impractical. - **Decorator at AdbcConnection boundary rejected.** A wrapper around the whole connection cannot see deep telemetry signals (chunk timing, poll count, first-batch latency) — those live inside statement classes. Wrong granularity. - **Refactor `Create` signature rather than add overload.** Changes the canonical `ConnectionTelemetry.Create` to take `string sessionId`. Thrift converts at the call boundary (`sessionHandle.SessionId.Guid.ToString()`). Eliminates the Thrift leak from the telemetry API permanently. - **Scoped strictly to SEA, plus `DriverMode.Sea` setter.** The cross-cutting Thrift gaps (workspace_id, auth_type, metadata-ops instrumentation, retry_count) are owned by the separate gap-fix workstream and will land independently. The only cross-cutting change pulled in here is unhooking the hardcoded `DriverMode.Types.Type.Thrift` in `BuildDriverConnectionParams` — without it SEA records would be silently mislabeled as Thrift. ## Areas needing specific review focus 1. **Observer interface shape & fail-open contract** (`§5.1`) — the observer methods, naming, and the requirement that implementations never throw. Specifically: are the 8 methods the right cut, or is finer granularity (e.g. split `OnChunksDownloaded` from `OnConsumed`) preferred? 2. **`ConnectionTelemetry.Create` signature change** (`§5.2`) — replaces `TSessionHandle?` with `string sessionId` and adds a `DriverMode mode` parameter. This touches a stable API used by the existing Thrift path; the Thrift call site must convert at the boundary. 3. **Result-format mapping for SEA** (`§8`) — SEA does not expose a typed `ResultFormat`. The mapping table from wire `disposition` × manifest state → proto `ExecutionResult.Format` is a judgment call; please review. 4. **Chunk-metrics dependency on gap-fix** (`§9`, `§16`) — this design assumes the `CloudFetchDownloader → ChunkMetrics → CloudFetchReader.GetChunkMetrics()` plumbing from the gap-fix workstream lands first or concurrently. If gap-fix is delayed, SEA ships with `ChunkMetrics.Empty` and backfills later. Is that acceptable, or should we sequence differently? 5. **Open questions** (`§17`): polling-granularity semantics for `PollCount`, `is_internal_call` semantics for SEA `USE SCHEMA`, and whether SEA's `operation_type` should always be `EXECUTE_STATEMENT_ASYNC` or map to sync-emulated variants. ## Related - Builds on the architecture in [`csharp/doc/telemetry-design.md`](../blob/stack/PECO-3022-sea-telemetry-design/csharp/doc/telemetry-design.md) - Supersedes the `TelemetryHelper` static-helper proposal in [`docs/designs/fix-telemetry-gaps-design.md`](../blob/stack/PECO-3022-sea-telemetry-design/docs/designs/fix-telemetry-gaps-design.md) for the new SEA code; Thrift-side gap-fix work continues independently - Jira: [PECO-3022](https://databricks.atlassian.net/browse/PECO-3022) This pull request and its description were written by Isaac. --------- Co-authored-by: Jade Wang <jade.wang+data@databricks.com>
CurtHagenlocher
pushed a commit
to CurtHagenlocher/databricks
that referenced
this pull request
Jun 6, 2026
…PECO-3022) (adbc-drivers#460) ## 🥞 Stacked PR Use this [link](https://github.com/adbc-drivers/databricks/pull/460/files) to review incremental changes. - [stack/PECO-3022-sea-telemetry-design](adbc-drivers#455) [[Files changed](https://github.com/adbc-drivers/databricks/pull/455/files)] [MERGED] - [**stack/pr-phase1-connection-telemetry-create-refactor**](adbc-drivers#460) [[Files changed](https://github.com/adbc-drivers/databricks/pull/460/files)] - [stack/pr-phase2-observer-interface](adbc-drivers#461) [[Files changed](https://github.com/adbc-drivers/databricks/pull/461/files/2454e7cd10de8ed68bd6b1df4583a5e55a99057c..63365ea628a7d24192348a715eefe1af8b568df3)] - [stack/pr-phase3-databricks-statement-observer](adbc-drivers#462) [[Files changed](https://github.com/adbc-drivers/databricks/pull/462/files/63365ea628a7d24192348a715eefe1af8b568df3..bc5e0318b86afe4be7d2beb22cc975d2c9dbe87a)] - [stack/pr-phase4-sea-connection-telemetry](adbc-drivers#463) [[Files changed](https://github.com/adbc-drivers/databricks/pull/463/files/bc5e0318b86afe4be7d2beb22cc975d2c9dbe87a..44b20018e9c58c114b05be973e9d7c2e5dd6adf5)] - [stack/pr-phase5-sea-statement-telemetry](adbc-drivers#464) [[Files changed](https://github.com/adbc-drivers/databricks/pull/464/files/44b20018e9c58c114b05be973e9d7c2e5dd6adf5..f7be471df206da95f9cd9368b7b8b1ab38795a07)] --------- ## 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 - [x] All existing telemetry unit tests pass unchanged. - [x] Existing Thrift integration tests continue to emit `driver_connection_params.mode = DRIVER_MODE_THRIFT`. - [x] New unit tests cover string sessionId + both DriverMode values. ## Related - Design: adbc-drivers#455 — `docs/designs/PECO-3022-sea-telemetry-integration-design.md` §5.2 + §10. - Stack root: [stack/PECO-3022-sea-telemetry-design](adbc-drivers#455). Part of PECO-3022. --------- Co-authored-by: Jade Wang <jade.wang+data@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🥞 Stacked PR
Use this link to review incremental changes.
Summary
Mechanical refactor of the Thrift
DatabricksStatementto consume theIStatementOperationObserverintroduced in #461 instead of its own private telemetry hooks. Behavior is unchanged — verified by the existing Thrift telemetry test suite.CreateTelemetryContext,CreateMetadataTelemetryContext,RecordSuccess,RecordError,EmitTelemetry) with calls into the new observer field.((DatabricksConnection)Connection).TelemetrySessioncast that previously coupled the statement to the connection's concrete type — the observer is now injected at statement construction.IsCompressed/ResultFormatto the observer through a proper interface call, removing a downcast-to-TelemetryObserver.Contextleak.Files touched
csharp/src/DatabricksStatement.cs— private hooks removed, observer field wired, reader-inspection hook added.csharp/test/Unit/Telemetry/DatabricksStatementObserverRefactorTests.cs(new) — pins the post-refactor behavior.Test plan
OssSqlDriverTelemetryLogfor a known statement before/after the refactor (verified via diff-of-golden-record approach in the existing suite).((DatabricksConnection)Connection).TelemetrySessioncast removed; observer comes fromDatabricksConnection.CreateStatement.Related
IStatementOperationObserver).Part of PECO-3022.