docs: add SEA telemetry integration design (PECO-3022)

[jadewang-db]

🥞 Stacked PR

Use this link to review incremental changes.

• stack/PECO-3022-sea-telemetry-design [Files changed]
  • 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 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
• Supersedes the TelemetryHelper static-helper proposal in docs/designs/fix-telemetry-gaps-design.md for the new SEA code; Thrift-side gap-fix work continues independently
• Jira: PECO-3022

This pull request and its description were written by Isaac.

[eric-wang-1990]

Three inline comments on the observer contract — see lines 167, 171, 216.

[eric-wang-1990]

Thread-safety contract contradicts the impl sketch.

This line says methods may be called from any thread; implementations must be thread-safe, but §11 then says StatementTelemetryContext is single-statement scope with reads only at OnFinalized time on the calling thread, and the §12 example uses plain _ctx.PollCount = count; — no Interlocked, no volatile, no lock. Plain scalar writes are not thread-safe in general (torn long writes on 32-bit, no happens-before with the finalize read).

The three sections don't reconcile. Pick one:

• Tighten the contract (recommended): callers must serialize calls per-statement; only OnFinalized may race (between the error-path and the dispose-path) and is idempotent via Interlocked.Exchange on the emitted flag. This matches actual usage — a single statement is driven by one ExecuteQueryAsync caller plus its reader's Dispose, and statements aren't shared.
• Or make the writes actually thread-safe (Interlocked / volatile / lock), and accept the cost.

The second option is overkill for the actual usage. Recommend the first; the contract sentence here should change to call out that only OnFinalized is required to tolerate concurrent calls.

[jadewang-db]

Good catch — tightened the contract in §5.1 to match §11/§12 (commit 9f2c497).

Concretely:

• Lifecycle methods are NOT required to be thread-safe. The single statement is driven by one ExecuteQueryAsync caller plus its reader's Dispose, statements are not shared across threads, so plain scalar writes are correct. The StatementTelemetryContext field writes preceding OnFinalized are happens-before the final read because they're serialized by the caller — no Interlocked/volatile/lock needed.
• OnFinalized is the one method that MUST be idempotent, since error-path and dispose-path can both reach it. Enforced via Interlocked.Exchange on a single _emitted flag; subsequent calls become no-ops.

Updated §5.1 interface-level comment, §11 Concurrency section, and the §15 test names (added OnFinalized_CalledFromErrorAndDispose_EnqueuesOnce to cover the documented race).

[eric-wang-1990]

OnExecuteSucceeded name and timing are misleading.

§6 says this fires "after response received" from ExecuteStatementAsync. But SEA's ExecuteStatementAsync returns a statementId with status commonly PENDING/RUNNING — the statement hasn't succeeded yet, just been accepted. Suggest renaming:

void OnStatementSubmitted(string statementId);   // or OnExecuteAccepted

This also has a knock-on effect on §8: the doc says SeaResultFormatMapper.Map(disposition, manifest, response) is called at OnExecuteSucceeded time, but the manifest is generally only fully populated when polling reaches SUCCEEDED — at the initial response it can be null. Either:

• Resolve result format at the terminal poll (then §8's table makes sense and the mapper has a real manifest to read), or
• Document explicitly that for SEA, format is resolved after OnPollCompleted (status=SUCCEEDED), not on the initial response.

This may want a separate OnResultFormatResolved(format) callback, or fold the format into OnPollCompleted's args.

[jadewang-db]

Agreed on both points — renamed and moved format resolution (commit 9f2c497).

• Renamed OnExecuteSucceeded → OnStatementSubmitted(string statementId). SEA's execute returns PENDING/RUNNING, so the previous name was misleading. Added a short "naming rationale" note in §5.1 so future readers see why.
• Moved result-format resolution to the terminal poll. SeaResultFormatMapper.Map(disposition, manifest, response) now runs in PollUntilCompleteAsync when status=SUCCEEDED (manifest is populated), and the format is folded into the existing terminal callback: OnPollCompleted(int count, long latencyMs, ExecutionResult.Types.Format resultFormat). Picked the fold over a separate OnResultFormatResolved to keep the interface smaller — the manifest-populated moment IS the terminal poll, so one callback expresses both facts.

Updated: §4.1 class diagram, §5.1 interface, §6 integration-points table (renamed "Execute returned" → "Execute accepted"), §7 sequence diagram (with status=PENDING/RUNNING annotated and the mapper note at terminal poll), §8 mapper text, §12 example, §15 tests.

[eric-wang-1990]

How does the reader get the IStatementOperationObserver?

This row says OnConsumed / OnChunksDownloaded fire from reader Dispose, but the doc never specifies how CloudFetchReader / InlineArrowStreamReader obtain the observer. These readers are shared between Thrift and SEA today, so the wiring matters. Three options:

1. Pass _observer into the reader constructor — couples shared readers to the observer interface.
2. Statement subscribes to a reader event/callback (reader.OnDisposed += ..., reader.OnFirstBatch += ...) — lighter coupling, keeps the shared readers protocol-agnostic.
3. Wrap the reader in a thin decorator owned by StatementExecutionStatement that emits on Dispose — zero changes to shared reader classes.

Option 3 (or 2) seems preferable since the readers are shared infrastructure. Whatever the choice, please pick one and add it to §4.4 / §6 — without it the reviewer can't evaluate the blast radius of this design on the shared cloud-fetch code path.

[jadewang-db]

Picked option 3 (decorator) — documented in new §4.5 (commit 9f2c497).

Rationale: CloudFetchReader and InlineArrowStreamReader are shared with the Thrift path, so coupling them to the observer interface (option 1) would be invasive for shared infrastructure. Option 2 (reader-side events) still requires shared-reader changes. Option 3 keeps the shared readers completely unchanged.

StatementExecutionStatement wraps the underlying reader in a thin ObservingArrowReader : IArrowArrayStream decorator that it owns:

• ReadNextRecordBatchAsync fires OnFirstBatchReady on the first call (latency from decorator construction).
• DisposeAsync fires OnConsumed, and OnChunksDownloaded(_inner.GetChunkMetrics()) when the inner is a CloudFetchReader.
• Lifetime is bound to the QueryResult.Stream returned to the caller.

This also keeps Thrift and SEA symmetric — DatabricksStatement can use the same decorator if it wants to emit those callbacks through the observer interface.

The §6 integration-points table now reflects this (the "First batch ready" and "Results consumed" rows point at ObservingArrowReader), and §7's sequence diagram shows the wrap step explicitly. Added ObservingArrowReader_* unit tests to §15.