docs: fix-telemetry-gaps design document by jadewang-db · Pull Request #331 · adbc-drivers/databricks

jadewang-db · 2026-03-10T23:59:14Z

🥞 Stacked PR

Use this link to review incremental changes.

stack/fix-telemetry-gaps-design [Files changed]
- stack/pr-phase1-thrift-telemetry-gaps [Files changed]
  - stack/pr-phase2-sea-telemetry [Files changed]

Summary

Comprehensive gap analysis of telemetry proto field coverage for the ADBC C# driver
Identifies that SEA (Statement Execution) connections have zero telemetry - completely unwired
Finds that ChunkDetails.SetChunkDetails() is defined but never called from CloudFetch pipeline
Documents missing fields: auth_type, WorkspaceId, runtime_vendor, client_app_name, and more
Proposes TelemetryHelper composition pattern to share telemetry logic between Thrift and SEA

Key decisions and alternatives considered

Composition via TelemetryHelper over abstract base class: Thrift has 4 intermediate inheritance layers (HiveServer2 → Spark → SparkHttp) that SEA doesn't share. C# single inheritance makes a shared base class infeasible without restructuring the Thrift chain. Static helper is the standard C# pattern for cross-hierarchy sharing.
5-phase implementation priority: SEA wiring first (zero coverage today), then missing fields, chunk details wiring, other behavioral changes, and finally E2E tests
ChunkDetails bridging: Aggregate per-chunk metrics in CloudFetchDownloader and expose via reader interface, rather than passing telemetry context deep into the download pipeline

Areas needing specific review focus

Is the TelemetryHelper composition approach acceptable, or should we consider restructuring the inheritance chain?
Are there additional SEA-specific telemetry fields we should track beyond the Thrift parity set?
Review the "Fields Intentionally Not Populated" table - are any of those actually needed for C#?
Is the phase ordering correct? Should chunk details wiring be higher priority?

🤖 Generated with Claude Code

eric-wang-1990 · 2026-03-12T21:25:43Z

docs/designs/fix-telemetry-gaps-design.md

+**Mapping:**
+| Auth Config | auth_type String |
+|---|---|
+| PAT | `"pat"` |


For this SEA/Thrift should be identical, we should have authMech and authFlow.

eric-wang-1990 · 2026-03-12T21:28:09Z

docs/designs/fix-telemetry-gaps-design.md

+    EnableArrow = true,  // Always true for ADBC driver
+    RowsFetchedPerBlock = GetBatchSize(),
+    SocketTimeout = GetSocketTimeout(),
+    EnableDirectResults = true,


Some parameters are not shared between Thrift and SEA, e.g. EnableDirectResults

eric-wang-1990 · 2026-03-12T21:31:48Z

docs/designs/fix-telemetry-gaps-design.md

+        +GetInfo() QueryResult
+    }
+    class StatementTelemetryContext {
+        +OperationType OperationTypeEnum


What are OperationTypeEnum consist of?

eric-wang-1990 · 2026-03-12T21:35:06Z

docs/designs/fix-telemetry-gaps-design.md

+- No `TelemetrySessionContext` creation
+- No `TelemetryClient` initialization
+- `StatementExecutionStatement` has no telemetry context creation or `EmitTelemetry()` calls
+- `DriverMode` is hardcoded to `THRIFT` in `DatabricksConnection.BuildDriverConnectionParams()` - there is no code path that ever sets `SEA`


User should be able to override this from adbc.databricks.protocol?

eric-wang-1990 · 2026-03-12T21:36:51Z

docs/designs/fix-telemetry-gaps-design.md

+
+| Field | Status | Notes |
+|---|---|---|
+| `WorkspaceId` | **NOT SET** | Declared in TelemetrySessionContext but never populated during InitializeTelemetry() |


How do we plan to populate this?
For non-spog it is usually resolvable from the host, I wonder if Thrift ever expose this information at all?
For spog it will be passed in as query parameter as part of httpPath

eric-wang-1990 · 2026-03-12T21:37:45Z

docs/designs/fix-telemetry-gaps-design.md

+- Would still need duplicated field declarations in each class
+- Awkward pattern for C# compared to Java
+
+**Option C: Composition via TelemetryHelper (chosen)**


Yes I agree with Composition is the right direction

Copilot

Pull request overview

Adds a design document that analyzes current telemetry coverage in the Databricks ADBC C# driver and proposes a phased plan to close identified gaps (notably SEA/Statement Execution having no telemetry today), including field-by-field proto alignment and E2E validation strategy.

Changes:

Introduces a comprehensive telemetry gap analysis against the telemetry proto schema (including SEA vs Thrift coverage).
Proposes a TelemetryHelper composition approach to share telemetry logic across the divergent Thrift and SEA class hierarchies.
Defines an implementation plan (phased) including chunk-metrics (ChunkDetails) wiring and E2E tests asserting proto field population.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-12T21:42:21Z

docs/designs/fix-telemetry-gaps-design.md

+No new concurrency concerns. All changes follow existing patterns:
+- `TelemetrySessionContext` is created once per connection (single-threaded)
+- `StatementTelemetryContext` is created once per statement execution (single-threaded within statement)
+- `TelemetryClient.Enqueue()` is already thread-safe


The Concurrency section claims “No new concurrency concerns,” but the proposed Phase 4 work aggregates per-chunk metrics inside CloudFetchDownloader, which downloads chunks concurrently. The design should call out that chunk-metric aggregation must be thread-safe (e.g., interlocked updates or locking) to avoid races when multiple download tasks complete in parallel.

Suggested change

No new concurrency concerns. All changes follow existing patterns:

- `TelemetrySessionContext` is created once per connection (single-threaded)

- `StatementTelemetryContext` is created once per statement execution (single-threaded within statement)

- `TelemetryClient.Enqueue()` is already thread-safe

Most changes follow existing concurrency patterns, but Phase 4 introduces one new consideration: per-chunk metric aggregation inside `CloudFetchDownloader` must be thread-safe because chunks are downloaded concurrently. Implementations MUST use appropriate synchronization (e.g., interlocked updates, locks, or thread-safe data structures) when updating shared metric state from multiple download tasks.

- `TelemetrySessionContext` is created once per connection (single-threaded)

- `StatementTelemetryContext` is created once per statement execution (single-threaded within statement)

- `TelemetryClient.Enqueue()` is already thread-safe, and `CloudFetchDownloader` must ensure all per-chunk metric updates are also thread-safe

Copilot · 2026-03-12T21:42:21Z

docs/designs/fix-telemetry-gaps-design.md

+
+## Objective
+
+Ensure the ADBC C# driver reports **all** proto-defined telemetry fields to the Databricks backend, matching the JDBC driver's coverage. Close gaps in field population, expand coverage to metadata operations, and add E2E tests verifying every proto field.


The Objective states the driver should report all proto-defined telemetry fields, but later sections explicitly list proto fields that are intentionally left unset (e.g., many DriverConnectionParameters fields). Consider tightening the Objective wording to “all applicable proto fields” or explicitly scope which fields are expected to be populated vs intentionally omitted, to avoid a contradictory goal statement.

Suggested change

Ensure the ADBC C# driver reports **all** proto-defined telemetry fields to the Databricks backend, matching the JDBC driver's coverage. Close gaps in field population, expand coverage to metadata operations, and add E2E tests verifying every proto field.

Ensure the ADBC C# driver reports **all applicable** proto-defined telemetry fields to the Databricks backend, matching the JDBC driver's coverage, while explicitly documenting any proto fields that are intentionally left unset. Close gaps in field population, expand coverage to metadata operations, and add E2E tests verifying every applicable proto field.

Copilot · 2026-03-12T21:42:21Z

docs/designs/fix-telemetry-gaps-design.md

+| Aspect | Thrift (DatabricksConnection) | SEA (StatementExecutionConnection) |
+|---|---|---|
+| Base class | SparkHttpConnection | TracingConnection |
+| Session creation | `OpenSessionWithInitialNamespace()` Thrift RPC | `CreateSessionAsync()` REST API |


The Thrift session creation entry references OpenSessionWithInitialNamespace(), but that method name does not exist in the C# codebase (the Thrift path builds a TOpenSessionReq and handles TOpenSessionResp). Consider updating this row to reference the actual method(s)/RPCs used (e.g., OpenSession / CreateSessionRequest() / HandleOpenSessionResponse()), so the design doc stays aligned with current implementation.

Suggested change

| Session creation | `OpenSessionWithInitialNamespace()` Thrift RPC | `CreateSessionAsync()` REST API |

| Session creation | `OpenSession` Thrift RPC (`TOpenSessionReq` / `TOpenSessionResp`) | `CreateSessionAsync()` REST API |

Copilot · 2026-03-12T21:42:22Z

docs/designs/fix-telemetry-gaps-design.md

+No new configuration parameters are needed. All changes use existing connection properties and runtime information.
+


The Configuration section says no new configuration parameters are needed, but earlier the doc proposes reading adbc.databricks.client_app_name to populate client_app_name. That’s either a new optional connection property (should be documented here and added to DatabricksParameters in the implementation), or the design should be adjusted to derive the value from existing inputs (e.g., process name / User-Agent) without introducing a new key.

Suggested change

No new configuration parameters are needed. All changes use existing connection properties and runtime information.

This design introduces a single **optional** connection property to control telemetry:

- `adbc.databricks.client_app_name` (string, optional): If set, this value is propagated to the `client_app_name` field in telemetry events. If not set, the driver derives `client_app_name` from existing inputs (for example, the process name and/or User-Agent), preserving current behavior.

All other changes use existing connection properties and runtime information; no mandatory new configuration is required.

Comprehensive gap analysis of telemetry proto field coverage including: - SEA connections have zero telemetry (highest priority) - ChunkDetails.SetChunkDetails() defined but never called - Missing fields: auth_type, WorkspaceId, runtime_vendor, client_app_name - Composition via TelemetryHelper chosen over abstract base class - E2E test strategy for all proto fields across both protocols Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…A second) Co-authored-by: Isaac

jadewang-db force-pushed the stack/fix-telemetry-gaps-design branch from a169f36 to 0a27469 Compare March 11, 2026 00:20

jadewang-db requested a review from eric-wang-1990 March 11, 2026 00:21

eric-wang-1990 reviewed Mar 12, 2026

View reviewed changes

eric-wang-1990 requested a review from Copilot March 12, 2026 21:38

Copilot started reviewing on behalf of eric-wang-1990 March 12, 2026 21:38 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

This was referenced Mar 13, 2026

Extract TelemetryHelper for shared telemetry logic\n\nTask ID: task-2.1-extract-telemetry-helper #346

Draft

feat(csharp): fix Phase 1 Thrift telemetry gaps with E2E test coverage #345

Open

Jade Wang and others added 3 commits March 13, 2026 22:04

docs: move E2E tests to Phase 1 (test-first approach)

2867946

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

docs: consolidate implementation into 2 phases (Thrift gaps first, SE…

425a554

…A second) Co-authored-by: Isaac

jadewang-db force-pushed the stack/fix-telemetry-gaps-design branch from 094dc6e to 425a554 Compare March 13, 2026 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: fix-telemetry-gaps design document#331

docs: fix-telemetry-gaps design document#331
jadewang-db wants to merge 3 commits intomainfrom
stack/fix-telemetry-gaps-design

jadewang-db commented Mar 10, 2026 •

edited

Loading

Uh oh!

eric-wang-1990 Mar 12, 2026

Uh oh!

eric-wang-1990 Mar 12, 2026

Uh oh!

eric-wang-1990 Mar 12, 2026

Uh oh!

eric-wang-1990 Mar 12, 2026

Uh oh!

eric-wang-1990 Mar 12, 2026

Uh oh!

eric-wang-1990 Mar 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		## Objective

		Ensure the ADBC C# driver reports all proto-defined telemetry fields to the Databricks backend, matching the JDBC driver's coverage. Close gaps in field population, expand coverage to metadata operations, and add E2E tests verifying every proto field.

	Ensure the ADBC C# driver reports all proto-defined telemetry fields to the Databricks backend, matching the JDBC driver's coverage. Close gaps in field population, expand coverage to metadata operations, and add E2E tests verifying every proto field.
	Ensure the ADBC C# driver reports all applicable proto-defined telemetry fields to the Databricks backend, matching the JDBC driver's coverage, while explicitly documenting any proto fields that are intentionally left unset. Close gaps in field population, expand coverage to metadata operations, and add E2E tests verifying every applicable proto field.

	\| Session creation \| `OpenSessionWithInitialNamespace()` Thrift RPC \| `CreateSessionAsync()` REST API \|
	\| Session creation \| `OpenSession` Thrift RPC (`TOpenSessionReq` / `TOpenSessionResp`) \| `CreateSessionAsync()` REST API \|

		No new configuration parameters are needed. All changes use existing connection properties and runtime information.

-No new configuration parameters are needed. All changes use existing connection properties and runtime information.
+This design introduces a single **optional** connection property to control telemetry:
+- `adbc.databricks.client_app_name` (string, optional): If set, this value is propagated to the `client_app_name` field in telemetry events. If not set, the driver derives `client_app_name` from existing inputs (for example, the process name and/or User-Agent), preserving current behavior.
+All other changes use existing connection properties and runtime information; no mandatory new configuration is required.

Conversation

jadewang-db commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🥞 Stacked PR

Summary

Key decisions and alternatives considered

Areas needing specific review focus

Uh oh!

eric-wang-1990 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

eric-wang-1990 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

eric-wang-1990 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

eric-wang-1990 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

eric-wang-1990 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

eric-wang-1990 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jadewang-db commented Mar 10, 2026 •

edited

Loading