Repository navigation
The Azure per-database branch emits no phase split at all #2855
Description
Activity
Fixed by #2893, merged to
dev.The Azure SQL DB per-database branch now emits
connect:+open:+drain:+other:instead of one blended per-database total, withOpenDatabaseConnectionAsynctimed as a phase in its own right — the third decomposition this issue called for, since that branch opens a connection per database and neither other path does.Naming took option (b), a new
PerDatabase*Msprefix, and widened the pin's derivation in the same change. ReusingPerItem*Mswas rejected on the precedent already recorded inServerScopeOpenMs's own doc comment: the enumerated log site gates onPerItemPhasesMeasuredand prints an item name taken from the enumeration, so sharing those fields makes an Azure run print as an enumerated one. That is the same collision that gave the server-scoped path its own prefix.ServerScopePhaseSplitTests' derivation now admits the third prefix andMinimumPhaseStampswent 12 → 16.Two latent holes turned up on the way, both now pinned rather than just noted:
MinimumPhaseStamps = 12was already one behind the true 13. An abandoned collection cycle records that it stopped, not what it was doing #2864'sServerScopeLastReadMsmatched the derivation pattern the day it landed and was covered automatically — exactly as designed — but nobody raised the floor, so a deletion could have passed unnoticed. 16 is now the exact count, not a round number.- The measured flags were guarded by nothing at all.
PhaseSettersselects onlong, so noboolcould ever appear in it. A flag assigned after the await would leave a faulting phase declaring itself unmeasured and printing nothing, while the number stamped correctly and the arithmetic pin stayed green.MeasuredFlags_AreReachableFromExceptionHandlers_...closes that, derived onbool/*PhasesMeasuredthrough the same IL walk, and it covers the pre-existingPerItemPhasesMeasuredtoo.
Every stamp and the flag are set from a
finally, verified by IL walk rather than inspection (inHandler=1on all three setters, against a control setter atinHandler=0). The per-iteration reset matters and is deliberate: the loop reuses oneCollectorContextacross databases, so without it a faulting connect would print the previous database's split as its own.Scope — what this deliberately does not do:
- Emit and log only. Nothing is persisted. The per-database split is N:1 against
collection_log, which holds one row per collector run, and how to persist that is an open decision. It is the same cardinality question as Decide how to persist the per-database fetch split (plan_fetch/text_fetch), which is N:1 against collection_log #2860's — and whatever Decide how to persist the per-database fetch split (plan_fetch/text_fetch), which is N:1 against collection_log #2860 settles governs this split too, though note Decide how to persist the per-database fetch split (plan_fetch/text_fetch), which is N:1 against collection_log #2860 is scoped to the per-database fetch split specifically, so this one is a second instance rather than something it already covers. No migration; schema stays at 109. - A faulting connect stamps and flags, but nothing prints on the fault path. Filed as The Azure per-database split is stamped on the fault path but only printed on the success path #2896 rather than forced. The honest fix needs a second stopwatch or a second line shape, because the line decomposes
dbSqlMsandsqlSliceis declared inside thetrybelow the watermark read andBuildQuery— hoisting it would silently widendbSqlMs, which is not log-only: it feedssqlMs,fanout.Observe(...)andcollection_log.sql_duration_ms. Widening a persisted metric to satisfy a log requirement is the wrong trade, so it is recorded in code at the log site, in the pin header, and on The Azure per-database split is stamped on the fault path but only printed on the success path #2896 with the measurement that should decide between the two options. other:on this path also absorbs connection teardown — one close per database, inside the parent stopwatch. Named explicitly, because "a largeother:is itself the finding" only helps a reader who knows what is in it.
Not verified: no Azure SQL DB target was available, so the branch was never exercised end to end. What the per-database connect actually costs is now answerable, not answered — and the same applies to the PostgreSQL per-database collector that shares this branch. The Windows suites ran in CI only.
- added a commit that references this issue
on Sep 4, 2026 Correction: this split is live in production, and my closing note said otherwise
I closed this saying "no Azure SQL DB target was available, so the branch was never exercised end to end… What the per-database connect actually costs is now answerable, not answered." The first half is true and the conclusion is wrong.
The branch is not Azure-only.
RunsPerDatabaseis engine-unconditional —=> true;— for seven PostgreSQL collectors:pg_index_bloat,pg_index_usage_stats,pg_table_bloat_stats,pg_autovacuum_stats,pg_column_stats,pg_predicate_statsandpg_extension_availability. All seven reach this same loop on every PostgreSQL target, every cycle, where one connection per database per cycle is exactly whatconnect:was added to name.So the
connect:/open:/drain:split shipped here has been emitting in production on the Aurora fleet since it landed. What is genuinely un-exercised is the Azure arm specifically: all 84 SQL Server targets are RDS, which does not host Azure SQL DB (engine_edition = 3,service_objectiveNULL on probed members — the shipped collector's own witness, since that column is populated onlyWHEN EngineEdition = 5).Why the distinction matters rather than being pedantic: I filed the deferred fault-path half as #2896 with a stated expectation that it might not be worth building because the branch may never run. That reasoning was wrong on the same misreading, and #2896's own investigation is what caught it. It has since landed (#2922) — and found a latent hazard created by this issue's change: the per-iteration clear sat below the stopwatch, so a fault in the watermark read, the adaptive-shrink store write, or
BuildQueryreached the catch arms holding the previous database's flag and stamps. Reading the split there would have attributed one database'sconnect:to another. Now moved to the top of thetry.No action needed here — this issue's change is correct and the follow-up is closed. Recording the correction because "answerable, not answered" undersold what had actually shipped, and because anyone reading this issue to understand the branch's reach would have been misled by my note.
The Azure SQL DB per-database branch in
DarlingCollectorRunner.RunAsync(theOpenDatabaseConnectionAsyncloop) emits no phase split at all. It is uninstrumented, not mis-instrumented — distinct from #2854, which fixed stamps that existed but were skipped on throw.Today it measures only a blended per-database total:
So an Azure per-database run has exactly the attribution problem #2851 fixed for server-scoped collectors and #2164/#2312 fixed for the enumerated path: one number covering everything, which cannot say whether a slow database is bound by connect, by server-side work before the first row, or by streaming.
Why it is its own issue rather than part of #2854
The shape differs. This branch opens a connection per database, which neither other path does. Its split is therefore
connect: + open: + drain:, notopen: + drain:— a third decomposition, withOpenDatabaseConnectionAsyncas a phase in its own right. On Azure SQL DB that connect is a real cost (a fresh login per database, per cycle), and it is exactly the kind of term that currently hides inside a blended number.It needs new fields and a new log line, not a stamp move: new
CollectorContextmembers, a gating flag mirroringPerItemPhasesMeasured/ServerPhasesMeasured, and a per-database line this branch does not currently emit at all.Worth doing because the pattern keeps paying
Two independent ~20-40x in-process gaps have been found this week and both were only tractable once phases were split:
procedure_stats: 247 ms for the shipped query against a real target vs 4,644 mssql_duration_msp50, with the box at 4% CPU (procedure_stats is 9.2x slower on use1 than use2 for identical row counts, and is 69% of use1's collection body #2847)Neither was visible in a blended number. The Azure branch is the last collection path still reporting one.
When it lands, extend the pin
ServerScopePhaseSplitTestsnow derives its IL reachability set fromCollectorContext(PerItem*Ms/ServerScope*Ms,long, settable) and asserts a minimum count. New phase stamps named to that pattern are covered automatically; a stamp named outside it would not be, so either follow the convention or widen the derivation and raiseMinimumPhaseStamps.