Fixes #2855

[erikdarlingdata]

What

The Azure SQL DB per-database branch in DarlingCollectorRunner.RunAsync emitted no phase split at all — not a mis-stamped one, none — so a slow database could not be attributed even in principle. It was the last collection path still reporting one blended number, after #2164/#2312 (enumerated) and #2851 (server-scoped). Distinct from #2854, which moved stamps that already existed.

Each database now logs, on its own line:

  [alpha] query_store [demo] sql:1800ms = connect:1451ms + open:247ms + drain:36ms + other:66ms (149 rows, pg:12ms)

Why the shape is a THIRD decomposition

This branch opens a connection per database, which neither other path does — the enumerated path reuses one target connection across items, the server-scoped path opens one for the whole server. So OpenDatabaseConnectionAsync is a phase in its own right: on Azure SQL DB a fresh login per database per cycle, and the same branch serves the per-database PostgreSQL collectors, where one connection per database is unavoidable because a PostgreSQL session is bound to one database for its lifetime.

drain: is measured, not inferred, so other: is a real residual (command construction, the trailing probe-failure rowset) — the #2851 departure, for the same reason.

The line is not gated on batch.Count > 0, unlike the enumerated path's quiet-on-zero rule. That rule guards a ROW-COUNT line, whose payload is the row count. This is a PHASE line, and the connect is paid on a quiet database exactly as on a busy one; the server-scoped phase line is the closer sibling and prints for every measured run.

Naming: PerDatabase*Ms, and the pin widened in the same change

Reusing PerItem*Ms would have made an Azure run print as an enumerated one — the enumerated log site keys on its own flag and prints an item name. That is the exact collision ServerScopeOpenMs's doc comment records as the reason the server-scoped path was given its own prefix, so the precedent decided it.

The price of the honest name is that ServerScopePhaseSplitTests' derivation had to be widened here, since a stamp outside the pattern is a stamp nothing guards — worse than no stamp, that file's whole claim being that a new one is covered the day it appears.

MinimumPhaseStamps goes 12 → 16, the exact count rather than a round number. 12 was already one behind the true 13: #2864's ServerScopeLastReadMs matched the pattern the day it landed and was covered automatically — exactly as designed — but left room for a deletion to go unnoticed.

The flag, and a gap it exposed

Every stamp and the gating flag are set from a finally, so a login that times out or a per-database budget that fires mid-handshake still reports the phase it died in. The log line gates on context.PerDatabasePhasesFrom(dbSqlMs) — one shared expression, so "no split" and "no line" are one decision — never on a figure being non-zero: a pooled connect can legitimately measure 0ms, and that is precisely the database whose open: and drain: are worth reading.

While wiring that: the measured flags' handler reachability was guarded by nothing. The *Ms derivation selects on long, so no bool could ever appear in it. A flag assigned after the await leaves a faulting phase declaring itself unmeasured and printing nothing, while the number is stamped correctly and the arithmetic pin stays green. MeasuredFlags_AreReachableFromExceptionHandlers_... now derives the *PhasesMeasured setters and scans them through the same IL walk — which also covers the pre-existing PerItemPhasesMeasured (already correct, so it passes).

Verification

Windows CI is the arbiter for Darling.Tests. Locally (macOS) I compiled the real shipped ServerScopePhaseSplitTests.cs into a throwaway net10.0 xunit.v3 host — it has no Windows dependency — and ran it against the built assemblies: 14 passed, 0 failed (7 pre-existing + 7 new). A separate reflection/IL harness over the built PerformanceMonitor.Collectors.dll and PerformanceMonitor.Darling.Service.dll confirms 16 derived stamps and that the three new members match the pin's selector (long, settable, *Ms, PerDatabase*), with the harness asserting its loaded DLLs are byte-identical to the projects' own bin output.

Seven regressions proven red first, each on a different assertion:

variant	fails
a new stamp renamed out of the pattern	floor: Only 15 phase stamps ... expected at least 16
floor pushed past the count (17)	floor: Only 16 ... expected at least 17
the widening reverted	floor: Only 13 ... expected at least 16
connect stamp moved out of its finally	set_PerDatabaseConnectMs is never invoked from inside an exception handler (2 call site(s), all on success paths)
the flag moved out of its finally	set_PerDatabasePhasesMeasured is never set from inside an exception handler (2 call site(s), all on success paths)
gate keyed on connect > 0	two tests — Assert.Null() Failure ... has a value (prints a stale split) and Assert.NotNull() Failure ... does not have a value (drops a measured zero)
a value comparison added at the log site	Assert.DoesNotContain() Failure: Sub-string found

Not verified

• No figure has been measured against a live Azure SQL DB target, and nothing available here can reach one. The branch itself was not exercised end to end; what the connect actually costs is now answerable, not answered.
• Darling.Tests / Lite.Tests as suites were not run locally (net10.0-windows). Only the one pin file above was executed, in a net10.0 host.

Scope

Visibility, not a speedup: no query, schedule, collector or connection behaviour changes. Emit-and-log only — a per-database split is N:1 against collection_log where the server-scoped split #2859 persists is 1:1, and #2860 already holds that cardinality question open for the per-database fetch split. No column, no rung; schema stays at 109.

Fixes #2855

🤖 Generated with Claude Code

[erikdarlingdata]

One deferral, filed rather than dropped: #2896.

Every stamp and the flag are set from a finally, so a database whose connect times out does stamp its phases and does declare itself measured. It just never prints them — the log site is on the success path, and a fault reaches one of the three catch arms instead (the budget arm's WARNING, or the generic arm's Skipping database ... at Debug).

Not fixed here because the line decomposes dbSqlMs, and that parent is out of scope in the catch: sqlSlice is declared inside the try, below the watermark read and BuildQuery. Hoisting it would silently widen dbSqlMs, which is not log-only — it feeds sqlMs, fanout.Observe(...) and collection_log.sql_duration_ms. That leaves a second stopwatch or a second line shape, and #2896 records both plus the measurement that should decide between them.

Stated in the code too, at the log site and in the pin's header, rather than left for a reader to infer that "stamped from finally" means "reported on the fault path".

[claude]

Review

Went through the diff (CollectorContext.cs, DarlingCollectorRunner.cs, ServerScopePhaseSplitTests.cs, CHANGELOG.md) line by line. This is a pure C#/Darling observability change — no T-SQL, no schema rung, no Lite files touched.

Correctness

• Traced the connect/open/drain instrumentation against the pre-existing using chain: the refactor from using (var dbConnection = await OpenDatabaseConnectionAsync(...)) to a try/finally-wrapped local preserves disposal semantics exactly — OpenDatabaseConnectionAsync already disposes and rethrows on its own failure (line ~3599), so no leak is introduced on a failed connect, and nothing double-disposes on a failed ExecuteReaderAsync.
• Verified the arithmetic: PerDatabaseOtherMsFrom clamps correctly and every test's hand-computed residual (50, 0, 50, 0) matches the shipped Math.Max(0, dbSqlMs - connect - open - drain) expression.
• Verified the reflection-derived counts by hand: 10 PerItem*Ms + 3 ServerScope*Ms + 3 PerDatabase*Ms = 16, matching the new MinimumPhaseStamps; 2 *PhasesMeasured bools (PerItem, PerDatabase) matching MinimumMeasuredFlags. ServerPhasesMeasured is correctly excluded — it's a CollectorRunResult record parameter, not a CollectorContext property, so it's genuinely out of the IL scan's reach as the comment claims.
• Reset-then-stamp ordering in the loop is correct: PerDatabase* fields are cleared at the top of each database's iteration, before the connect, so a database whose earlier phase throws can never leak a stale split into the next database's log line — and no code path reads the fields between a mid-iteration throw and the next iteration's reset.
• The one known gap (a per-database connect that times out stamps its phases/flag from the finally but never reaches the log line, since the catch arms have no access to the sqlSlice/dbSqlMs needed to print the line) is explained in-line, in the PR description, and covered by an explicit test-file section stating what the pins do not claim. Agreed this is a reasonable, honestly-scoped boundary rather than an oversight.

Lite/Darling parity

• No parity gap: PerItemOpenMs/ServerScopeOpenMs/*PhasesMeasured are already Darling-only (confirmed zero references under Lite/), and the new PerDatabase* fields follow the same "zero-value, unmeasured-by-Lite" contract documented on the shared CollectorContext. Lite simply never sets PerDatabasePhasesMeasured, so PerDatabasePhasesFrom returns null and the log line is silently absent there — consistent with how the two existing decompositions already behave.

Security / perf

• No new I/O, no user input, no string formatting of untrusted data (server/database names are only ever from the target's own catalog, unchanged from existing code). Emit-and-log only, as the PR states — no behavior change to queries, schedules, or connections.

No blocking issues found. Nice adversarial test coverage (all 7 regressions in the PR description check out against the shipped code).

[claude]

Reviewed for correctness, Lite/Darling parity, security, and performance.

Correctness: Traced the new connect/open/drain instrumentation in DarlingCollectorRunner.cs (Azure per-database branch) end to end:

• sqlSlice already started before OpenDatabaseConnectionAsync pre-PR, so dbSqlMs (feeding sqlMs, fanout.Observe, and collection_log.sql_duration_ms) is unchanged — this really is emit-and-log only, as claimed.
• Each stamp/flag is set from its own finally, so a faulting connect/open/drain still records what it measured, per the Four enumerated-path phase stamps are skipped when the phase they time throws #2854 lesson this PR cites.
• The per-iteration reset (PerDatabaseConnectMs/OpenMs/DrainMs/PhasesMeasured cleared before the connect) correctly prevents a stale split from a previous database leaking onto a database whose own connect throws.
• Disposal order (reader → command → connection) is unchanged from the pre-PR using chain despite the refactor into try/finally + using var.
• The loop is sequential (foreach over databases, confirmed no parallelism), so the shared mutable context fields are safe to stamp this way.
• Verified the MinimumPhaseStamps = 16 and MinimumMeasuredFlags = 2 pins by counting matching properties on CollectorContext directly (10 PerItem*Ms + 3 ServerScope*Ms + 3 PerDatabase*Ms = 16; exactly PerItemPhasesMeasured/PerDatabasePhasesMeasured = 2) — the derivation checks out.
• The literal strings the new ThePerDatabaseSplitSumsToItsParent/TheAzureLogSiteGatesOnTheSharedExpression tests assert against (context.PerDatabasePhasesFrom(dbSqlMs) is { } dbPhases, the log format string) match the shipped source exactly.

Lite/Darling parity: OpenDatabaseConnectionAsync and this whole per-database branch are Darling-only (not present under Lite/), so there's no Lite counterpart to update. The new CollectorContext fields follow the same "silently reads as default on the store that doesn't set them" pattern already established for ServerScope*Ms/PerItem*Ms, which is consistent with the two-store parity rules in CONTRIBUTING.md (this is additive instrumentation, not a store-facing seam like IAlertStateStore).

Security/Perf: No new external input handling, no schema/query changes, no persistence — only structured logging (parameterized, no string interpolation into the message template) and a few extra Stopwatchs per database, which is negligible.

No issues found. This is a well-scoped, well-tested change.

[claude]

Reviewed the diff (CHANGELOG.md, DarlingCollectorRunner.cs, CollectorContext.cs, ServerScopePhaseSplitTests.cs).

Correctness: The connect/open/drain try/finally structure is sound — each phase stamp is written from its own finally, so a fault at any stage still records how far it got, and PerDatabasePhasesMeasured rides the same finally as the connect stamp so the two can never disagree. Verified by hand:

• The per-database fields are reset to zero/false at the top of every loop iteration (before sqlSlice starts), so a fault on database N can't leak a stale split into database N+1's log line.
• DbConnection openedConnection / DbDataReader openedReader are only read after their respective try/finally completes without throwing, so there's no unassigned-variable path and no risk of a partially-open resource escaping its using scope.
• The gate is PerDatabasePhasesFrom(dbSqlMs) is { } dbPhases, keyed off the PerDatabasePhasesMeasured flag rather than > 0, which correctly avoids the Four enumerated-path phase stamps are skipped when the phase they time throws #2854-style bug of suppressing a legitimately-zero pooled connect.
• Arithmetic checks out: MinimumPhaseStamps 12 → 16 matches the actual derived count (10 PerItem*Ms + 3 ServerScope*Ms including An abandoned collection cycle records that it stopped, not what it was doing #2864's ServerScopeLastReadMs + 3 new PerDatabase*Ms), and MinimumMeasuredFlags = 2 matches the two *PhasesMeasured bool properties on CollectorContext (ServerPhasesMeasured is a method-local on the server-scoped path, correctly excluded).
• The PR is upfront that the fault path (a connect that times out) stamps the phases but never reaches the log line, since sqlSlice/dbSqlMs aren't available in the catch block. That's a documented, pre-existing-pattern limitation (mirrors the enumerated path's Four enumerated-path phase stamps are skipped when the phase they time throws #2854 note) rather than a regression.

Lite/Darling parity: No parity gap. This entire phase-split logging mechanism (PerItem*Ms, ServerScope*Ms, and now PerDatabase*Ms) is Darling-only instrumentation — grepped Lite/ and confirmed none of the *PhasesMeasured flags or phase-split log lines exist there today, so there's no counterpart being left behind. CollectorContext is shared, but the new properties are harmless no-ops on Lite (never set, PerDatabasePhasesFrom always returns null there).

Schema/persistence: Correctly emit-and-log only — no collect.* or config.* changes, schema stays at V109, consistent with the PR's stated scope and the open N:1 cardinality question in #2860.

Style: T-SQL style section doesn't apply (no .sql changes in this PR). C# looks consistent with repo conventions (XML docs, PascalCase, nullable PerDatabasePhaseSplit?).

No correctness, security, or performance issues found. Nice test coverage — the seven red-first regression cases listed in the PR description line up with what's actually pinned in ServerScopePhaseSplitTests.cs.