feat(csharp): add IStatementOperationObserver and fail-open implementations (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

Adds the IStatementOperationObserver interface that decouples statement classes from telemetry's data model, plus three implementations. Pure new code with no consumers yet — wired up in subsequent stack phases.

• IStatementOperationObserver — 8 methods shaped around the statement's lifecycle (OnExecuteStarted, OnExecuteSucceeded, OnPollCompleted, OnFirstBatchReady, OnConsumed, OnChunksDownloaded, OnError, OnFinalized). Contract is documented as fail-open: implementations MUST NOT throw, must be thread-safe, and OnFinalized is terminal + idempotent. The interface speaks the statement's language, not the telemetry proto's — future tracing/audit observers can plug in without touching statement code.
• TelemetryObserver — translates observer calls to mutations on a StatementTelemetryContext. On OnFinalized builds the OssSqlDriverTelemetryLog and enqueues to the session's TelemetryClient. Uses a private Safe(Action) helper to concentrate the try/catch in exactly one place; method bodies are clean one-liners. Idempotency via Interlocked.CompareExchange on an _emitted flag.
• NullObserver — singleton no-op. Used as the default field value so statement code never needs null-checks at callsites.
• SafeObserver — belt-and-suspenders decorator that wraps any inner observer with per-method try/catch. Optional; first-party observers already honor the contract internally.

Files touched (all new)

• csharp/src/Telemetry/IStatementOperationObserver.cs
• csharp/src/Telemetry/TelemetryObserver.cs
• csharp/src/Telemetry/NullObserver.cs
• csharp/src/Telemetry/SafeObserver.cs
• csharp/test/Unit/Telemetry/TelemetryObserverTests.cs
• csharp/test/Unit/Telemetry/NullObserverTests.cs
• csharp/test/Unit/Telemetry/SafeObserverTests.cs

Test plan

• NullObserver_AllMethods_AreNoOps, NullObserver_IsSingleton.
• TelemetryObserver_OnExecuteStarted_PopulatesContext, _OnExecuteSucceeded_RecordsStatementId, _OnFinalized_EnqueuesExactlyOnce, _OnFinalized_CalledTwice_EnqueuesOnce, _OnError_RecordsErrorAndFinalizes, _AllMethods_NeverThrow_WhenContextCorrupted, _OnChunksDownloaded_MergesIntoChunkDetails.
• SafeObserver_PropagatesNormalCallsToInner, _SwallowsExceptionsFromInner_LogsAtTrace.

Related

• Design: docs: add SEA telemetry integration design (PECO-3022) #455 — §5.1 (interface contract), §12 (fail-open implementation pattern).
• Builds on refactor(csharp): make ConnectionTelemetry.Create protocol-agnostic (PECO-3022) #460 (no functional dependency; co-stacks for clean review).

Part of PECO-3022.

[jadewang-db]

Code review of phase 2 (observer interface + 3 implementations + tests).

Summary. Clean, well-documented, fail-open design. Interlocked.CompareExchange correctly guards exactly-once finalize; tests cover the contract dimensions that matter (concurrency, null inputs, throwing dependencies, no-ambient-Activity). Pure new code with no consumers yet, so blast radius is contained.

Findings, all medium-or-lower — none are blockers, but the comment/code mismatch in TelemetryObserver.OnError is worth fixing in this PR because it will mislead the next reader.

1. TelemetryObserver.OnError — comment contradicts the code.
2. TelemetryObserver.Safe — uses ex.Message directly; should use SafeMessage(ex) for parity with OnError and SafeObserver.
3. Safe() / SafeMessage() duplicated across TelemetryObserver and SafeObserver with subtly different suppression-event payloads — converge or extract.
4. Contract says "any further calls" after OnFinalized are no-ops; today only further OnFinalized is gated, other methods still mutate _ctx. Inert in current callers, but a strict reading of the contract would gate all methods.

Nothing critical. Approving in spirit once #1 is addressed.

[jadewang-db]

[Medium] Comment contradicts the code.

Lines 164–166 say "do not capture ex.Message here unless we have explicit consent" — but line 168 then captures it: _ctx.ErrorMessage = SafeMessage(ex);.

The in-memory capture matches existing behavior in ConnectionTelemetry.cs:207, and BuildTelemetryLog does not serialize ErrorMessage into the proto today, so there is no actual leak. But the comment is misleading and the future risk is real: if anyone wires ErrorMessage into DriverErrorInfo.error_message later (once LPP review lands), they will reasonably assume the capture site already considered the privacy implications — when in fact the comment told them the opposite.

Pick one: either (a) drop the _ctx.ErrorMessage = SafeMessage(ex); line and rely on ErrorName only, matching what the comment says, or (b) update the comment to reflect that .Message is captured in-memory today, the proto field is gated separately in BuildTelemetryLog, and SafeMessage is used to neutralize exceptions that throw from their .Message property. Option (b) is consistent with ConnectionTelemetry and is probably what you want.

[jadewang-db]

✅ Done — went with option (b): updated the comment at TelemetryObserver.cs:165-170 to accurately describe what the code does. The new comment notes that .Message is captured in-memory only, that BuildTelemetryLog does not serialize ErrorMessage into the proto today (LPP review pending), and that SafeMessage guards against .Message throwing. Matches the existing pattern in ConnectionTelemetry.cs:207.

[jadewang-db]

[Medium] ex.Message here, but SafeMessage(ex) in OnError (and in SafeObserver).

The rest of this file — and SafeObserver.Safe — explicitly guard against exceptions whose .Message property throws. This site doesn't. The outer catch {} on line 234 saves us from a crash, but it also swallows the suppression event for that specific failure mode, so we lose telemetry visibility into observer failures exactly when an upstream exception is misbehaving.

Use SafeMessage(ex) for consistency.

[jadewang-db]

✅ Done — TelemetryObserver.Safe() now uses SafeMessage(ex) instead of ex.Message directly. Suppression telemetry is now preserved even when the inner exception has a throwing .Message property.

[jadewang-db]

[Low] Diverging suppression-event payloads between the two Safe() helpers.

SafeObserver.Safe tags the suppression event with observer.suppressed.source = "SafeObserver" (line 122). TelemetryObserver.Safe (TelemetryObserver.cs:227) emits the same event name (telemetry.observer.suppressed) but without a source tag.

This matters in two ways:

• A trace consumer can't tell which layer suppressed an exception when both decorators are in play (e.g. a third-party observer wrapped in SafeObserver wrapped over TelemetryObserver).
• These two helpers — plus SafeMessage — are essentially duplicated. Converging them into a single internal helper (e.g. TelemetryObserverSafety.Run(Action, string source)) would make the inconsistency impossible by construction. Optional, but worth doing while the surface is small.

[jadewang-db]

✅ Done — chose to converge rather than extract a shared helper (lighter for two small classes). Added observer.suppressed.source = "TelemetryObserver" to TelemetryObserver.Safe() so a trace consumer can tell the two suppression layers apart when both decorators are in play.

[jadewang-db]

[Low] Contract vs. implementation: post-OnFinalized non-OnFinalized calls.

This contract says: "any further calls — including additional OnFinalized calls — MUST be no-ops (idempotent)." That language covers all methods, not just OnFinalized.

TelemetryObserver only gates further OnFinalized via _emitted. After finalize, a stray OnPollCompleted / OnError / OnChunksDownloaded will still mutate _ctx. The mutations are inert today because the log has already been built and no one reads the context post-finalize — but a strict reading of the contract would have all observer methods check _emitted first and return early.

Two equally reasonable resolutions:

• Loosen the interface doc: "OnFinalized is the only terminal call that must be idempotent; other methods after finalize are silently dropped but their effect on observer state is implementation-defined."
• Tighten the implementation: add an if (Volatile.Read(ref _emitted) != 0) return; short-circuit at the top of every Safe(...) body in TelemetryObserver.

The contract-shipped-as-written is what later reviewers will hold us to, so picking one of these and aligning is worth doing now while there are no external consumers.

[jadewang-db]

✅ Done — tightened the implementation (the safer interpretation per the review comment). Added a private IsFinalized() helper (Volatile.Read(ref _emitted) != 0) and a gate at the top of every non-finalize lifecycle method body in TelemetryObserver. After OnFinalized() fires, all other lifecycle methods become true no-ops with no further mutation of _ctx.

[jadewang-db]

[Nit] Add a test for the post-finalize behavior that pins down whichever interpretation you pick in the contract-vs-impl issue (see comment on IStatementOperationObserver.cs:47).

E.g., something like OnFinalized_ThenLifecycleCalls_AreNoOps that asserts later OnPollCompleted / OnError calls do not enqueue a second log and do not affect any observable post-finalize state. Today this is implicitly true (because the second Enqueue is gated by _emitted), but having the assertion in the test suite locks it in.

Low priority.

[jadewang-db]

✅ Done — added TelemetryObserver_OnFinalized_ThenLifecycleCalls_AreNoOps. Captures the context fingerprint right after OnFinalized, then calls every non-terminal lifecycle method with values that would visibly mutate state if the new IsFinalized() gate were missing, and asserts both the enqueue count and every captured field are unchanged. Locks in the contract interpretation chosen in the previous fix.