Repository navigation
On Azure SQL Database, the long-query trace runs only in monitored databases and no longer drops a session another registration still uses - #4944
Conversation
…abase, in Lite and the Darling Viewer While the long-query completion trace is off, both apps show a banner that ended with "Enabling it creates the Extended Events session on this server". On Azure SQL Database the collector creates a database-scoped session in each monitored database instead, so the banner was wrong for that target. The banner text now comes from the target. LongQueryCompletionsCollector.SessionScopeSentence gives the last sentence for both apps: each monitored database on Azure SQL Database, this server everywhere else. Lite reads the target from the tab's engine edition, which it resolves before every tab load. The Viewer reads it from the server's stored engine edition. The Darling README and the CollectorScheduleDefaults comment for the collector now say the same.
|
Blind read first, then the body. The mechanism holds end to end: I traced the claim the banner now makes back to the code that performs it in both SKUs, not just to the sentence that describes it.
I also chased the four ways this shape has bitten before and found none of them here:
Three non-blocking notes, none of which I think should hold the PR:
|
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — the old banner ended "…session on this server; disabling it drops the session." — the removed
Textattributes,Lite/Controls/ServerTab.xaml:3120andViewerServerTab.xaml:3751(LEFT side). - VERIFIED — "wrong on Azure SQL Database… creates a database-scoped session in each monitored database (
RunsPerDatabase)" —LongQueryCompletionsCollector.cs:135, and both lifecycles actually do it:Lite/Services/RemoteCollectorService.LongQueryCompletions.cs:115and:213(enable and drop, per database, master skipped),DarlingXeSessions.cs:583into:648. - VERIFIED — "
SessionScopeSentence(bool)holds the last sentence. Both apps use it." —LongQueryCompletionsCollector.cs:143-145, called atServerTab.LongQueries.cs:29andViewerServerTab.LongQueries.cs:26; literals byte-identical to the asserted ones (grep -cis 1 in all four files). - VERIFIED — "Lite: the tab resolves the edition before every tab load" —
ServerTab.Refresh.cs:214awaitsRefreshEngineEditionAsync()ahead of theswitchreachingcase 20at:243; pinned byAzureSqlDatabaseCallSiteTests.TheTab_FallsBackToTheStoredEdition_BeforeEveryLoad. - VERIFIED — "Darling Viewer: the server's stored engine edition. An unknown edition gets the server sentence." —
ViewerServerTab.LongQueries.cs:39reads_server.EngineEdition(servers.sql_engine_edition,ViewerDataService.cs:96);TheViewerBanner_OnEveryOtherEdition_KeepsTheServerSentencecarriesInlineData(UnknownEngineEdition). - VERIFIED — "Both XAML files drop the fixed
Text. The refresh sets it instead." — safely:Visibility="Collapsed"stays in markup and the sole writer of thatVisibilityper SKU is the same method that now writesTextfirst. - VERIFIED — "Darling README gets one sentence for Azure SQL Database" —
Darling/README.md:741, string-identical to the new pin. - VERIFIED — "The
CollectorScheduleDefaultscomment says the same." —CollectorScheduleDefaults.cs:77. - VERIFIED — "the web viewer's text says nothing about where the session lives" —
wwwroot/js/pages/server-tabs.js:1324is only "This collector is opt-in and off by default."; a repo-wide grep for the server-scope phrasings finds no other claiming surface. - VERIFIED — "existing README pins in
DarlingCliCommandsTestsandDropXeSessionsVerbTestsstill pass" —DarlingCliCommandsTests.cs:2124-2125wants substrings the edit leaves intact;DropXeSessionsVerbTests.cs:809-812anchors on "a server removed while the collector was on keeps the session." and the new sentence is inserted before that anchor, with both required substrings still ~228 and ~278 chars into its 500-char window. - VERIFIED — "the old words, split into two sentences" — semicolon to period only,
LongQueryCompletionsCollector.cs:145. - UNVERIFIED — "RED on dev, with the failing lines" (three named assertions) — needs the tests checked out against the pre-change tree and run; two cannot compile there by the body's own admission.
- UNVERIFIED — "Lite.Tests 7160 / Darling.Tests 19878, 0 failed" —
net10.0-windowsWPF suites; this review ran on Linux with no build.
Riskiest lines
Darling/PerformanceMonitor.Darling.Viewer/ViewerServerTab.LongQueries.cs:39— edition source with no fallback: a registrysql_engine_editionof 0 on a real Azure SQL Database prints the server sentence. Call site pinned byTheViewerBanner_TakesItsTextFromTheTarget_NotFromAFixedServerSentence, unknown-edition answer byTheViewerBanner_OnEveryOtherEdition_KeepsTheServerSentence; UNPINNED for whether the column is populated (rests onDarlingObservability.cs:40's connect-probe upsert, untested here).Lite/Controls/ServerTab.LongQueries.cs:40— reads_isAzureSqlDatabaseat loader time, so correctness lives in another file's ordering. Pinned byAzureSqlDatabaseCallSiteTests.TheTab_FallsBackToTheStoredEdition_BeforeEveryLoadplusTheLiteBanner_TakesItsTextFromTheTarget_NotFromAFixedServerSentence.Lite/Controls/ServerTab.xaml:3120andViewerServerTab.xaml:3751— the banner has no markup text now, so any path showing it without the loader renders empty. Pinned negatively by the two_TakesItsTextFromTheTarget_facts and by there being oneVisibilitywriter per SKU; UNPINNED for "the XAML carries noTextat all", so a re-added literal passes both.
Traps near this diff
- "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — both halves ship together with their own tests:
ServerTab.LongQueries.cs:25withLite.Tests/LongQueryBannerSessionScopeTests.cs,ViewerServerTab.LongQueries.cs:23withDarling/Darling.Tests/LongQueryBannerSessionScopeTests.cs. - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — the shared half is aliased: one
SessionScopeSentenceatLongQueryCompletionsCollector.cs:143used by both SKUs; only the menu path stays per-SKU, and it genuinely differs between the two apps. - "A roster-based census does not see the file it does not roster (#3699)" — does not apply — the nearest roster,
AzureSqlDatabaseNotCollectedSurfaceTests.TheSubTabRefreshButtons_LearnTheEngineEdition_BeforeTheirLoaderRuns, covers handlers that bypassRefreshVisibleTabAsync;RefreshLongQueriesAsync's only caller isServerTab.Refresh.cs:243, inside it, so there is no bypass to roster. - "
InternalsVisibleToon PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply —PerformanceMonitorLite.csproj:44grantsLite.TestsandPerformanceMonitor.Darling.Viewer.csproj:66grantsDarling.Tests;Lite.Tests'using Darling.Testsresolves to the linkedCSharpSourceWalker.cs(Lite.Tests.csproj:40), not a project reference. - "dorny/paths-filter v4 evaluates each pattern as an INDEPENDENT predicate … Keep the carve-out INSIDE the include:
dir/**/!(*.md)" — does not apply as a defect, but it is why the new README pin cannot fire on a README-only PR:build.yml:181isDarling/**/!(*.md)whiledocsis**/*.md.build.yml:140-147records that as Erik's 2026-09-24 ruling withLiteRuntimePrerequisiteDocsTestsas the accepted precedent. - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — does not apply — both new members hold only string concatenation and one equality comparison (ServerTab.LongQueries.cs:25-29,ViewerServerTab.LongQueries.cs:23-26). - "
.gitattributessaystext=auto eol=crlf: a diff that touches every line of a file is a line-ending conversion until proven otherwise" — does not apply — all five touched or added.csfiles are UTF-8 CRLF and every hunk is local.
Divergence (blind read vs PR body)
One, cosmetic. The body describes the loader change as setting Text; the diff also clips the space after the equals sign on the one pre-existing line it touches in each SKU — the Visibility assignment at ViewerServerTab.LongQueries.cs:40 and ServerTab.LongQueries.cs:41. Nothing else: every mechanism the body names is implemented, and every surface the diff touches is accounted for. Unmentioned but not charged: the helpers took different signatures (bool in Lite, int in Darling), and Darling.Tests' BannerElement is internal while Lite.Tests open-codes the same logic inline.
Reviewer checklist
- Run both suites on Windows and confirm the stated counts; nothing was built here, so the new
using PerformanceMonitor.Collectors;in two WPF partials and theinternalvisibility of both helpers are unverified compiles. - Check the two test files out against dev and confirm the three named assertions go RED for the stated reasons.
- On a live Azure SQL Database in Darling, open Long Queries with the trace off and confirm
servers.sql_engine_editionis 5 so the banner reads "in each monitored database"; then the same in Lite against a target whose connection check failed, so the stored-edition fallback answers. - Eyeball both banners: the markup carries no
Textnow, so confirm no path shows an empty warning block and that the arrow and ellipsis render as glyphs, not mojibake, from BOM-less UTF-8 C#.
…they drive No behaviour change. The new tests fail here on purpose: they describe the fix in the next commit. - A shared helper in Collectors plans where the trace's database-scoped session is created and dropped, counts failed drop passes, and carries the databases a drop failed in. Nothing calls it yet. - Lite and Darling list the trace's databases and work in one database through hooks a test can replace. Darling's worker bookkeeping for the trace is a static method the tests call, and the runner takes the databases monitored as their own servers.
…s, retries a failed drop, and leaves separately monitored databases alone In both Lite and Darling, for the database-scoped session on Azure SQL Database: - Trace on: the session is created in each monitored database (Darling: narrowed by the collector's database scope, resolved by the same runner delegate as the read loop), and dropped from listed databases outside that set. - Trace off: the session is dropped from every listed database, exclusions and scope included. - A database monitored as its own server is left to that registration: never created in, never dropped, and the logical server's long-query read skips it. - A failed listing or a failed drop is retried on the next cycle instead of being counted done. After 5 failed passes in a row it is counted done, with one warning that names where the session may remain. A drop failure in one database no longer stops the others, and Lite logs it at Warning. - Darling's ON arm no longer counts the trace applied when the database list could not be read. The always-on deadlock and blocked-process sessions keep their inventory lifecycle. The banner, README, Lite's schedule description and Darling's CLI help say where the session is created and dropped.
…inventory, and its lifecycle test as an LF reader Both listings are expression-bodied members that open with a property pattern, so the member scan stops at the pattern's closing brace. What falls outside the range is two calls and no string literal. The lifecycle test's always-on guard slices a method body on an anchor that spans line breaks.
…ession Lite's read fails there every cycle. Darling's reads zero rows that look like a quiet trace. The comment said both fail.
…own server follows that server's setting, when enabling too The exception sat only on the disable sentence. Enabling skips that database as well, so it is now a sentence of its own that covers both.
…database another registration still uses Adds the registrations each app passes to the shared plan, KeptElsewhere, the co-owner part of the state key, and the tests that pin the rule in Lite and Darling. KeptElsewhere and the co-owners still count no other registration here, and the state key still joins its parts with the names' separator, so the behavior is as before and the new tests show it.
…e another registration of the server has the trace on KeptElsewhere names the databases where another enabled registration of the same logical server, of any read-only intent, keeps the session: a registration of one database keeps its own, and a logical server's registration keeps each database it creates the session in. Both drops in both apps skip them. The state key carries those registrations, so a skipped drop runs once the other one turns its trace off, and its parts now use their own separator, so a scope and an exclusion list cannot read two ways. The banner and the Darling README say so.
Blind-read notes, all non-blocking1. The drop pass now pays a connection per excluded / out-of-scope database, and each failure is a Warning. On the ON path the drop list is 2. The read-skip has no behavioural pin in either SKU.
3.
4. CHANGELOG. |
There was a problem hiding this comment.
Verdict: LGTM
Detail is in the two inline comments and the top-level note; this is the ledger.
Claims
- VERIFIED — the rule lives once in
LongQueryTraceDatabasesand both apps plan through it —Planis the only create/drop planner on either side:DarlingXeSessions.cs:692/:728/:792,RemoteCollectorService.LongQueryCompletions.cs:196/:319/:568 - VERIFIED — off drops from every listed database, exclusions and scope not applied,
masternever touched —applyExclusions: false+databaseScope: null(DarlingCollectorRunner.cs:6634,RemoteCollectorService.cs:1116);CanTouchand the off arm atLongQueryTraceDatabases.cs:172-181 - VERIFIED — Darling's lifecycle scope is the delegate the read loop resolves —
DatabaseScopeForreturns_databaseScope(...)(DarlingCollectorRunner.cs:400), resolved at dispatch at:1978 - VERIFIED — a database monitored as its own server is neither managed nor read by the logical server's registration, for this collector only —
LongQueryTraceDatabases.cs:170,DarlingCollectorRunner.cs:2237,RemoteCollectorService.DefinitionRunner.cs:285; the oneoverride => trueisLongQueryCompletionsCollector.cs:144 - VERIFIED —
KeptElsewhereignores read-only intent,SeparatelyMonitoredDatabaseshonours it, coverage is exact —LongQueryTraceDatabases.cs:236-243and:252vsAzureMasterScope.cs:49, pinned byOn_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExcludein both SKUs - VERIFIED — the state key's parts, its four separators, the 5-pass cap and its resets —
LongQueryTraceDatabases.cs:117/:122-125/:203-207/:337-350; resets atRemoteCollectorService.LongQueryCompletions.cs:361,DarlingWorker.cs:4155/:10462 - VERIFIED — the dev defect named: a failed listing while enabling latched "applied" —
return null;becomesthrow;atDarlingXeSessions.cs:716, and that return is what let the worker setLongQueryTraceApplied = enabled - VERIFIED — Lite warns rather than debugs each failed drop (
RemoteCollectorService.LongQueryCompletions.cs:433vsDarlingXeSessions.cs:837); the registry is published before the runner is built (DarlingWorker.cs:2619vs:2649); both banners read the target's edition with the space after=restored (ServerTab.LongQueries.cs:40,ViewerServerTab.LongQueries.cs:39) and the sentence pair exists only atLongQueryCompletionsCollector.cs:151-153 - VERIFIED — the stated mutation and the changed pins — without
CanTouch(database) &&the ON drop admitsmasterand the own-server database, failing bothOn_ADatabaseMonitoredAsItsOwnServer_...tests; one unscoped listing remains (DarlingXeSessions.cs:121); theSwallowedItemFailureTeststext pins match:728-731/:761-764/:787/:800/:824byte for byte - UNVERIFIED — the run counts (Lite 21/0 and Darling 64/0/4 at 464e314; Lite 7169/0 and Darling 19888/0/1320/1 at 7cc63c1) and "0 build warnings" — needs a build and test run;
dotnet buildwas unavailable here and Lite/Viewer are Windows-only WPF - UNVERIFIED — every RED-at-2e98575db / RED-at-d1a84993e assertion, and that the pre-fix single-separator
StateKeycollapsed two settings onto one key — intermediate commits; only HEAD is readable, where all are green by construction. A single separator does collapse both strings, so the mechanism is sound - UNVERIFIED — the Microsoft Learn read-only-replica facts and the one the body labels "inferred" (a primary drop removes the session from the replicas) — external documentation
Riskiest lines
PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs:189— the ON-path drop set; a wrongownedElsewhereorkeptremoves a session another registration is still reading, the defect class this PR closes — pinned byOn_TheOutsideDrop_LeavesAnExcludedDatabase_ThatASecondRegistrationsTraceCoversandOn_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExcludein both lifecycle suites, which drive the real reconcileLite/Services/RemoteCollectorService.LongQueryCompletions.cs:80— the Azure verdict every protective set hangs off, re-derived at:181and:314from a second read of the same mutableSqlEngineEdition; a disagreement runs the OFF drop with emptyseparatelyMonitoredand emptykeptElsewhere— UNPINNEDDarlingCollectorRunner.cs:2239/RemoteCollectorService.DefinitionRunner.cs:287— the read-skip, a capture-coverage change: an own-server database with its own trace off now has no capture at all — pinned only by the source-textTheLongQueryRead_SkipsTheDatabasesMonitoredAsTheirOwnServers, which fails on the line's deletion but not on the filter being fed the wrong set
Traps near this diff
- "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: plan/key/retry/exception shared in
LongQueryTraceDatabases.cs, read filter shared viaAzureSweepScope.cs:49and applied in both runners, banners throughLongQueryCompletionsCollector.cs:151,applyExclusionsin both (DarlingCollectorRunner.cs:6591,RemoteCollectorService.cs:1060), Lite's lack of a scope explicit atRemoteCollectorService.LongQueryCompletions.cs:528 - "A default interface member compiles a parity gap away ... CONTRIBUTING's two-store parity rule names
IAlertDeliverer; its members are required." (#3640) — does not apply: the new default is onICollectorDefinition<TRow>(ICollectorDefinition.cs:54), not a per-store seam; that interface already carries defaults of this shape (:118) and every collector reaches it through oneCollectorDefinitionBase.cs:84 - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, handled as the roster already does: both newListLongQueryTraceDatabasesAsyncmembers have that shape, are declared with their reason atTsqlConventionGuardTests.cs:739/:749, and the stranded arms hold no string literal - "A pin that encodes the lie ... Ask what a pin would fail ON" and "A roster-based census does not see the file it does not roster (#3699)" — do not apply as gaps: the lifecycle tests drive the real reconcile, the two guard tests declare themselves source-text pins in their own summaries, and the new file is rostered at
RepoFileAdoptionTests.cs:679 - "
OPTION(RECOMPILE)on collector queries" and "missing-index requests are never a finding's driver", plus the Collectors/Analysis/Alerting/Storage trap groups — do not apply: no T-SQL statement is added (only the exclusion clause becomes conditional,SqlServerTargetProvider.cs:142andRemoteCollectorService.cs:1116; the DDL fromDarlingXeSessions.cs:842on is unchanged), no index recommendation, no delta family, rate column, fact, scorer, alert condition or paged payload
Divergence (blind read vs PR body)
None material. Three readings narrower or wider than the body's, none contradicting it:
- "If the database listing fails, the reconcile throws" holds, but Lite's ON-side catch is
catch (SqlException ex)alone (RemoteCollectorService.LongQueryCompletions.cs:190), so a non-SqlExceptionfailure reaches the general handler raw. Same outcome, so the claim stands; the wrapping is just not universal. - "On, drop outside the set: No" holds for every case except the one the body then discloses as "One limit remains" — consistent, but stated in two separate sections.
- The body does not mention the ON-path drop pass's cost against excluded-and-inaccessible databases, nor the third derivation of the Azure verdict at
RemoteCollectorService.LongQueryCompletions.cs:402. Both are in my top-level comment.
Reviewer checklist
- Build and run both suites: every count and RED/GREEN-at-commit claim is unverified here. Confirm
Lite.Testsstill compilesDarling.Tests'CSharpSourceWalker, that the new static/instanceKnownEngineEditions.SeparatelyMonitoredDatabasesoverloads resolve without ambiguity, and thatDarlingSelfAlertTests.CapturingLoggeris reachable fromLongQueryTraceLifecycleTests. - On a live Azure SQL Database logical server with one database excluded and the login denied access to it, turn the trace on: expect 5 failed logins and exactly one "Stopped retrying" Warning, and check that reconnect re-arming the counter (
DarlingWorker.cs:10462) is not a per-reconnect loop on a flapping server. - With a second registration of one database on that server (read-only-intent and read-write), toggle each trace and confirm from
sys.database_event_sessionsthat no drop removes the other registration's session, and that the read-skip leaves no database both unread and session-bearing. - Open Long Queries in Lite and the Viewer the instant an Azure target connects: the banner's last sentence must be the per-database one, now that it comes from the edition at refresh time rather than static XAML.
…p, and for Darling creating a session dropped from outside
…Debug; Darling creates a session dropped from outside within an hour
…eates a missing long-query session again within an hour
Non-blocking: the ownership rule creates a coverage hole the UI does not announceFlagging as an observation, not a finding — the code documents it as deliberate ( After this change, a logical server's registration neither creates the trace's session in, nor reads, a database monitored as its own server. That database is covered only while its own registration's trace is on, and The always-on pair resolves the same conflict the other way: The reasons given for the stronger rule are sound as far as they go ( Other claims checked against the tree, for the recordVerified from the tree, as the body states them:
One small bookkeeping slip in the body: Could not be checked from a read-only tree: the full-suite counts (Lite 7181/0, Darling 19904/0/1320), the 0-build-warnings claim, the per-commit RED/GREEN sequencing, and the Microsoft Learn facts about read-only replicas sharing one session definition. |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Two findings, both inline. The claim-by-claim notes that do not fit here are in the top-level comment.
Claims
- VERIFIED — one shared rule, both apps compute the same sets, and no create or drop touches master or a separately monitored database —
LongQueryTraceDatabases.cs:72-95is the only Plan;CanTouchat lines 78-80 applied at 87 and 92; called fromDarlingXeSessions.cs:736andRemoteCollectorService.LongQueryCompletions.cs:212. - VERIFIED — while the trace is off the exclusions and the scope do not apply, and the lifecycle shares the read loop's scope resolver — new
applyExclusions: falseoverloads atDarlingCollectorRunner.cs:6591andRemoteCollectorService.cs:1060;DarlingCollectorRunner.cs:31consumed atDarlingXeSessions.cs:577. - VERIFIED — a failed listing now throws, Lite logs a failed drop at Warning, and only a cleanup pass that ran resets the count —
DarlingXeSessions.cs:584-586;RemoteCollectorService.LongQueryCompletions.cs:547-563;retry.Reset()inside the drop branch at line 127, not besideMarkLongQueryTraceAppliedat 130; thepassguard atDarlingWorker.cs:4188. - VERIFIED — the registry is published before the runner is built (
DarlingWorker.cs:2627vs 2657),KeptElsewhereignores read-only intent whileAzureMasterScope.cs:49honours it, the two whitespace slips are gone at this head, and every test the body names exists and asserts what the body says. - UNVERIFIED — the full-suite counts and the 0-build-warnings claim — needs a Windows build and test run; these are net10.0-windows WPF projects and this tree is read-only Linux.
- UNVERIFIED — the per-commit RED/GREEN ledger — only the merged head is present; reproducing it needs those commits built.
- UNVERIFIED — "a drop on the primary removes it from the replicas too (inferred)" and the Learn facts about the session definition reaching replicas — external docs plus a live Azure tier. The body labels the first "inferred", which is honest.
- REFUTED — "while the trace is on, it also runs the create side once an hour", read with "after 5 failed cleanup passes in a row … the app stops retrying on every cycle" as the whole retry story. A
CreateOnlypass that throws is re-selected on every sweep, uncapped: the general catch atDarlingWorker.cs:4231-4252leavesLongQueryTraceApplied,LongQueryTraceAppliedKeyandLongQueryTraceAppliedAtUtcuntouched, so the gate at 4168-4172 stays true forever. The pass then repeats at sweep cadence with a warning and one connection per monitored database each time. The cap arms only offLongQueryTraceDropException; the create side has none.
Riskiest lines
Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:4168— the hourly-create gate reads a stamp no failure path advances, so the one new cadence degrades to per-sweep on exactly the failure it exists to recover from — UNPINNED (On_ASessionDroppedFromOutside_IsCreatedAgainAnHourLater_AndNothingIsDroppedandOn_WithinTheHour_TheLatchedReconcileRunsNothingdrive only the succeeding pass)PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs:92— the single predicate behind every session drop; an error in any of its three terms strands a session or destroys a co-owned one — pinned byOff_DropsTheSessionEverywhere_ExcludedAndOutOfScopeDatabasesIncluded,On_ADatabaseMonitoredAsItsOwnServer_AndScopedOut_IsNotDropped,On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExcludeand the Lite twinsLite/Services/RemoteCollectorService.LongQueryCompletions.cs:82— gatesseparatelyMonitored,registrationsandKeptElsewhere, while lines 197 and 395 re-read the same live mutable status and re-decide the Azure branch for themselves — UNPINNED (every rig sets the edition once in its constructor and never moves it)
Traps near this diff
- "A default interface member compiles a parity gap away … CONTRIBUTING's two-store parity rule names
IAlertDeliverer; its members are required." — does not apply — the new defaulted member is onICollectorDefinition<TRow>(ICollectorDefinition.cs:54), not a seam CONTRIBUTING names; that interface already carries the same shape at line 118,CollectorDefinitionBase.cs:84supplies the virtual, and the inherited answer is the correct one for every other collector — the one needing the opposite overrides atLongQueryCompletionsCollector.cs:145. - "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — both SKUs got the plan, the read skip (
DarlingCollectorRunner.cs:62-65/RemoteCollectorService.DefinitionRunner.cs:285-288), the banner (ViewerServerTab.LongQueries.cs:21-27/ServerTab.LongQueries.cs:21-25), the retry cap, the schedule text and a mirrored test class; the remaining asymmetries are ones the stores genuinely differ on and each is commented. - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667) — a member the walk cannot read whole is one no census can see into." — applies and is handled — both newListLongQueryTraceDatabasesAsyncmembers are that shape and both are inKnownTruncatedRangeswith the reason inline (TsqlConventionGuardTests.cs:1460-1465,1529-1531); re-derivingDeclarationEnd(CSharpMemberMap.cs:333-404) shows it skips braces inside parentheses, so the newDropLongQueryTraceInEachAsyncin each app is read whole and correctly absent. - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — the varying sentence is aliased through
LongQueryCompletionsCollector.SessionScopeSentence(LongQueryCompletionsCollector.cs:154-156), and each app's test asserts the literal rather than the other app's copy (Lite.Tests/LongQueryBannerSessionScopeTests.cs:48,Darling.Tests/LongQueryBannerSessionScopeTests.cs:42). - "OPTION(RECOMPILE) on collector queries … A statement added to an existing batch needs its own hint — one on a neighbouring statement does not cover it." — does not apply — no collector query is added. The only new T-SQL path is the unchanged guarded drop via
BuildDropSessionSql(RemoteCollectorService.LongQueryCompletions.cs:594), and thesys.databasesenumeration atRemoteCollectorService.cs:1122is pre-existing text whose only change is which parameters the exclusion filter binds — still fully parameterised, andBuildDatabaseExclusionFilterat line 1241 already handled the new null.
Divergence (blind read vs PR body)
1. The retry story has a hole its own machinery does not cover. Every failure path the body enumerates is a cleanup failure, and the cap, the hourly clock and the Debug downgrade are all written against LongQueryTraceDropException. The body then adds a second recurring pass, the hourly create, and says only what it does when it succeeds. My blind read of DarlingWorker.cs:4154-4252 says a create pass that throws leaves every input to its own gate unchanged, so it reruns on the next sweep and the one after, forever. "Darling's hourly create pass: No. It only creates." is true and is not the risk; the cadence claim is, and neither the body nor any test addresses it. This drives the verdict.
2. The body treats "is this Azure SQL Database?" as one fact; Lite's code reads it three times. The body is careful that the two lists agree, and separatelyMonitored really is threaded into the callees. But the predicate those lists hang off is re-derived at three sites from the live mutable connection status, and the first bypasses KnownEngineEditions, the one class written because the raw live edition is unreliable right after a start or a failed check (KnownEngineEditions.cs:34-37). "The same set that the alert sweep uses" is true of the list and not of the gate in front of it.
Smaller non-blocking notes are in the top-level comment. Everything else matched, including the claims I expected to find overstated.
Reviewer checklist
- Reproduce the uncapped create retry on a live Azure SQL Database target: latch the trace on, revoke ALTER ANY EVENT SESSION (or break the master hop), and watch the log for an hour. Expect one reconcile warning per sweep rather than one per hour.
- Confirm the full-suite runs and the 0-build-warnings claim on Windows with the .NET 10 SDK — neither test project builds on this read-only Linux tree.
- Settle the read-only-replica question on a real tier with read scale-out on: does a drop on the primary remove the definition from the replicas? The body calls this "inferred" and
KeptElsewhererests on it. - Open a Lite server tab on an Azure SQL Database registration whose connection status is not yet populated and confirm the first reconcile does not take the Azure drop path with empty registrations.
- Look at the Long Queries tab of a master registration on a logical server with separately registered databases whose own traces are off, and decide whether the missing databases need a note the way the always-on lists got one.
… read once, the verb's search and the exclusion clause The new tests that pin behavior still to come fail at this commit. The seams that hold the exclusion clause of each app's Azure database list, and the verb's search plan, are in place with the behavior the code had before.
…ition once, --drop-xe-sessions searches excluded databases Darling: an hourly create pass, or the attempt after the cap, that throws something other than a failed drop moves its hourly clock, so later sweeps do not repeat it and its warning. Lite: the reconcile resolves the engine edition once and passes it to the ensure and the drop. Darling --drop-xe-sessions: for the long-query session it searches every online database, the exclusions not applied, except master, a database monitored as its own server and a database another registration of the server keeps it in. It counts every other registration as keeping it, because it cannot read their schedules. The other sessions keep the monitored list. Removes LongQueryTraceDropRetry.ConsecutiveFailures, which nothing read.
Divergence: the
|
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Claims
- VERIFIED — one shared rule,
LongQueryTraceDatabases, so both apps compute the same sets —Plancalled fromDarlingXeSessions.cs:727/:740andRemoteCollectorService.LongQueryCompletions.cs:218/:408/:532 - VERIFIED — the off-side drop ignores exclusions and scope —
applyExclusions: falsereaches the query in both apps (DarlingCollectorRunner.cs:6666,RemoteCollectorService.cs:1149);Plan'sif (!enabled)arm never readsmonitoredDatabases - VERIFIED — Darling's lifecycle takes the #3477 scope from the read loop's own delegate —
DatabaseScopeForreturns_databaseScope(...)(DarlingCollectorRunner.cs:1184), passed atDarlingXeSessions.cs:947; pinned byCollectorDatabaseScopeRungTests:499-500 - VERIFIED —
SkipsSeparatelyMonitoredDatabasesis true only on this collector, and both read loops filter on it —LongQueryCompletionsCollector.cs:170; defaults atICollectorDefinition.cs:51,CollectorDefinitionBase.cs:84; filters atDarlingCollectorRunner.cs:2235,RemoteCollectorService.DefinitionRunner.cs:285 - VERIFIED — 5 failed passes, one Warning naming the databases, then hourly at Debug —
DropAttemptCap,LongQueryTraceDropRetry.RecordFailure/RetryDue,GiveUpWarning, theafterTheCaplog switch in bothDropLongQueryTraceInEachAsync - VERIFIED — Lite's count resets only after a cleanup pass that ran and succeeded —
retry.Reset()inside the drop branch (RemoteCollectorService.LongQueryCompletions.cs:135,:145), not besideMarkLongQueryTraceApplied; pinned byOn_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttempt - VERIFIED — Darling's hourly pass only creates, and does not reset the cleanup count —
CreateOnlyreturns before the outside-drop;if (pass != LongQueryTracePass.CreateOnly) Reset()atDarlingWorker.cs:4188 - VERIFIED — the state key tells scope from exclusions — four level separators in
LongQueryTraceDatabasesconsumed byStateKey/CoOwners; pinned byTheStateKey_TellsWhereTheScopeEndsAndTheExclusionsBegin - VERIFIED — the registry is published before the runner is built, so the first reconcile sees the co-owners —
_registryState.Publish(initialServers)atDarlingWorker.cs:2627, runner at:2644, both reads (:4058,:2668) live on top - VERIFIED — the quoted banner/README/CLI/description text is what the tree carries, and the
=spacing matches dev —LongQueryCompletionsCollector.cs:151-153,Darling/README.md:741,DarlingCliCommands.cs:5117,ScheduleManager.cs:754,collection_schedule.json:20,ServerTab.LongQueries.cs:40,ViewerServerTab.LongQueries.cs:39 - UNVERIFIED — "a drop on the primary removes it from the replicas too (inferred)" and the read-only-intent reasoning on it — needs a read-scale-out database: drop on the primary, read
sys.database_event_sessionson the replica - UNVERIFIED — the suite counts (7181/0, 19904/0/1320) and the per-commit RED-to-GREEN narrative — both suites are
net10.0-windowsWPF; this ran on Linux against a read-only tree, so nothing was built or run
Riskiest lines
Darling/PerformanceMonitor.Darling.Service/DarlingXeSessionCleanup.cs:577—plan.Visitednow includes the excluded databases, and a failure to open one becomes aProblemsentry that forces exit 2 (:381) even when every session found was dropped — UNPINNED (TheLongQuerySearch_VisitsAnExcludedDatabase_...pins the plan, not the exit code)Darling/PerformanceMonitor.Darling.Service/DarlingXeSessions.cs:724— the enable-side listing failure wentreturn nulltothrow: right for the retry, but uncapped at two warnings per sweep where the drop-side twin three lines down is capped — retry pinned byOn_AFailedListing_IsNotMarkedApplied; cadence UNPINNEDPerformanceMonitor.Collectors/LongQueryTraceDatabases.cs—Plan'slistedDatabases.Where(d => CanTouch(d) && !keep.Contains(d) && !kept.Contains(d)): a wrongkeptleaves sessions forever or removes one another registration reads — pinned byOff_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOn,On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExcludeand their Lite twins
Traps near this diff
- "A default interface member compiles a parity gap away ... left Lite's deliverer and fourteen fakes inheriting an answer nobody wrote (#3640)" — applies in shape, not as a defect: the default body is at
ICollectorDefinition.cs:51, but both runners read the member (DarlingCollectorRunner.cs:2235,RemoteCollectorService.DefinitionRunner.cs:285) and the only collector needing another answer overrides it (LongQueryCompletionsCollector.cs:170) - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, handled: the two new ones are rostered with the reason (TsqlConventionGuardTests.cs:1465,:1531). Nothing else new has the shape —DropLongQueryTraceInEachAsync's braces sit at paren depth 1, whichDeclarationEndskips (CSharpMemberMap.cs:357-359), and the localKeptElsewhere(...) =>has no access modifier forDeclarationHeadto match (:84-89) - "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit (#3594)" — applies and holds: shared planner, read skip,
applyExclusionsoverload (RemoteCollectorService.cs:1060/DarlingCollectorRunner.cs:6591), cap, both banners, both descriptions. Asymmetries are app-justified — Lite has no scope (...LongQueryCompletions.cs:108) and needs no hourly create because it ensures every cycle - "
InternalsVisibleToon PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply: the new internals are in the Viewer and Lite, each visible to its own suite (PerformanceMonitor.Darling.Viewer.csproj:66,PerformanceMonitorLite.csproj:44), and the cross-project helper arrives byCompile Include(Lite.Tests.csproj:42) - "A test that reads the clock twice measures the CI scheduler (#3676). One anchor owns both ends of an interval" — does not apply: neither rig reads a real clock —
utcNowis a parameter (DarlingWorker.cs:4127) and Lite injectsLongQueryTraceUtcNowForTests(...LongQueryCompletions.cs:116) - "Integer division in a rate column ... truncates every rate under 1/s to 0", and the rest of the SQL family — do not apply: the only T-SQL touched is moved verbatim into its own member (
RemoteCollectorService.cs:1153), exclusions still parameterized throughBuildDatabaseExclusionFilter; no new query, no arithmetic, no ordinal sort
Divergence (blind read vs PR body)
The body's lifecycle account matches the diff on every claim graded above, including the two dev defects it names. The gap is scope: it says the PR "changes where Lite and Darling create and drop that session, and it corrects the text that describes it", and mentions --drop-xe-sessions only for one warning sentence. The diff also rewrites where that verb searches — SqlServerXeSessionCleanupTarget takes the registry, PlanAzureSearch splits the search into an always-on set and a long-query set built with the exclusions not applied, FindSessionsAsync filters each found name through plan.Reports, and the connector plus the injected connect delegate change arity. Three new DropXeSessionsVerbTests and one new TargetProviderTests go with it, and neither file appears in any round's "Existing tests that changed" list. That is where the finding lives rather than a bookkeeping complaint: a reader working from the body would not know the verb now opens databases the registration excludes, so would not ask what a failure there does to the exit code. Details and two non-blocking notes are in the top-level comment.
Reviewer checklist
- Build both suites on Windows and run them — the counts, the 0-warning claim and the RED-to-GREEN narrative are what a read-only Linux tree cannot touch
- On a real Azure SQL Database logical server, exclude one database the login cannot open (or a paused serverless one), turn the trace on, run
--drop-xe-sessions <server>, and record the exit code and stderr - With the trace off there, watch one Darling reconnect and count the connections the off-side drop opens: it now visits every online database, exclusions included
- Drop the session on the primary of a read-scale-out database and read
sys.database_event_sessionson the replica, to settle the inferred claim - With the trace on and a login that cannot read
master, watch Darling's log and judge the two-warnings-per-sweep cadence
… no longer fails the verb; document the create-side retry cadence The long-query search opens databases the registration excludes, so one the login cannot open (no user there, a paused serverless database) made the verb exit 2 even after it dropped every session it found. Such a database is now a note on stderr that names it and says a session left there needs a manual drop, and the exit code is unchanged. A monitored database that cannot be opened still exits 2. The usage text and the README say so. The Azure create side's comment now says why its listing failure rethrows and retries on every sweep, and that only the drop side is capped.
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — the rule lives in one place and both apps compute the same sets —
LongQueryTraceDatabases.cs:83is the onlyPlan; callers atRemoteCollectorService.LongQueryCompletions.cs:218/:428/:552,DarlingXeSessions.cs:260/:303/:385,DarlingXeSessionCleanup.cs:580. - VERIFIED — off-side drop ignores exclusions and scope —
applyExclusions: false/databaseScope: nullatRemoteCollectorService.cs:1151,DarlingCollectorRunner.cs:6668,:6682; pinned byOff_DropsTheSessionEverywhere_ExcludedDatabasesIncludedand..._ExcludedAndOutOfScopeDatabasesIncluded. - VERIFIED — a separately-monitored database is neither created, dropped nor read by the logical server —
CanTouchatLongQueryTraceDatabases.cs:77; read cut byLongQueryCompletionsCollector.cs:144atRemoteCollectorService.DefinitionRunner.cs:285andDarlingCollectorRunner.cs:2237. - VERIFIED — no drop removes a session another registration keeps, whatever its read-only intent —
OtherOwnersfilters onEnabled && TraceOn && Hostonly; pinned both directions byOff_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOnandReadOnlyRegistration_Off_LeavesItsDatabase_WhileTheLogicalServersTraceIsOnAndCoversIt. - VERIFIED — "the coverage is exact" for two registrations that both exclude a database —
Keepschecks the owner's own exclusions; pinned byOn_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude. - VERIFIED — each state-key part has its own separator — four control characters at
LongQueryTraceDatabases.cs:~40; pinned byTheStateKey_TellsWhereTheScopeEndsAndTheExclusionsBegin. - VERIFIED — Darling's hourly create pass, drop only on connect / key change / after-cap —
DarlingWorker.cs:4158ladder,DarlingXeSessions.cs:377early return;passreaches only the Azure arm (:597). - VERIFIED — a failed listing now throws instead of returning —
DarlingXeSessions.cs:289; pinned byOn_AFailedListing_IsNotMarkedApplied. - VERIFIED — Lite logs a failed drop at Warning now, not Debug —
RemoteCollectorService.LongQueryCompletions.cs:~565. - VERIFIED — Lite's count resets only after a cleanup pass that ran and succeeded —
retry.Reset()only at:135and:145; pinned byOn_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttempt. - VERIFIED — Lite reads the edition once and remembers it —
:90plusKnownEngineEditions.cs:134; pinned byOff_AnEditionThatChangesDuringTheReconcile_StillLeavesTheDatabasesAnotherRegistrationKeeps, which drives the blank-then-5 flip through the clock hook. - VERIFIED — the verb searches excluded databases for that session only, with a second master listing only when exclusions exist —
DarlingXeSessionCleanup.cs:605,PlanAzureSearchat:573. - VERIFIED —
LongQueryTraceDropRetry.ConsecutiveFailuresis gone — no such member in the tree. - VERIFIED —
SkipsSeparatelyMonitoredDatabasesis true only on this collector — sole=> trueatLongQueryCompletionsCollector.cs:144. - VERIFIED — both
Visibility =lines match dev at this head — no diff line for either; the=awaitform exists only in intermediate commits. - VERIFIED — both truncated members added to
KnownTruncatedRanges—TsqlConventionGuardTests.cs:1460,:1529, with the stranded text named. - UNVERIFIED — every RED/GREEN and pass-count figure — test-run outputs; no SDK runs here and Lite/Viewer are WPF. I confirmed each cited test exists and reads on the behaviour claimed.
- UNVERIFIED — "0 build warnings" — needs a build. The one readable risk: this PR makes
cref="GetAzureDatabaseListAsync"atBlockedProcessReport.cs:315ambiguous, but no project setsGenerateDocumentationFile, so CS0419 cannot fire. - UNVERIFIED — "full suites not run at this head" — self-declared disclosure, not checkable from the tree.
- UNVERIFIED — the Learn read-scale-out facts and the body's own inferred "a drop on the primary removes it from the replicas too".
Riskiest lines
PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs:83— drops over a list with exclusions and scope deliberately off;CanTouchandkeptare all that prevent removing a session another registration reads — pinned byOff_LeavesTheSessionOfADatabaseMonitoredAsItsOwnServerand the twoOff_Leaves...co-owner tests in both SKUs.Lite/Services/RemoteCollectorService.LongQueryCompletions.cs:90— one edition read drives both halves, via aKnownEngineEditionsnever seeded from the store; a blank status takes the server-scoped arm, asengineEdition == 5did on dev — pinned byOff_AnEditionThatChangesDuringTheReconcile_StillLeavesTheDatabasesAnotherRegistrationKeeps.Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:4158— the pass ladder; wrong here is a drop that never runs or a warning every sweep — pinned byOn_WithinTheHour_TheLatchedReconcileRunsNothing,On_ASessionDroppedFromOutside_IsCreatedAgainAnHourLater_AndNothingIsDropped, and the two...RunsAgainAnHourLater_NotOnEverySweeptests.
Traps near this diff
- "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — every arm lands twice (banner
ServerTab.LongQueries.cs:21/ViewerServerTab.LongQueries.cs:17; read cut:285/:2237; plan; cap;applyExclusionsseamRemoteCollectorService.cs:1149/DarlingCollectorRunner.cs:6665). The two single-SKU items are stated: Darling's hourly create pass (Lite ensures every cycle,RemoteCollectorService.cs:542) and--drop-xe-sessions(Lite has no such verb). - "A default interface member compiles a parity gap away:
DeliverAndReportAsyncwith a default body left Lite's deliverer and fourteen fakes inheriting an answer nobody wrote (#3640)." — does not apply —ICollectorDefinition.cs:54has a default body, but the gap is a host that never asks, and both hosts ask (:285,:2237). Non-blocking comment left: the body is dead anyway,CollectorDefinitionBase.cs:84binds. - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, and is handled — both newListLongQueryTraceDatabasesAsyncmembers are entered atTsqlConventionGuardTests.cs:1460/:1529with reasons. - "
OPTION(RECOMPILE)on collector queries … A statement added to an existing batch needs its own hint" — does not apply — no new T-SQL; the enumeration moves verbatim intoRemoteCollectorService.cs:1153andSqlServerTargetProvider.cs:148is unchanged. - "Truncation inferred from a full page … Fetch
limit + 1" — does not apply — no paged read or MCP payload;LongQueryTraceDropException.Databasescarries the names that failed, not a count. - "The engine must remember the observation stamp it last fired on, per key, beside — never folded into — its last-seen stamp" (#3579/#3636) — does not apply as a defect; this is that shape done right,
LongQueryTraceDropRetrykeyed onStateKeyand separate from the applied latch. - "A test that reads the clock twice measures the CI scheduler (#3676). One anchor owns both ends of an interval." — does not apply — both rigs inject the clock (
LongQueryTraceUtcNowForTests,Rig.Clock). - "
CASE WHEN interval > 0 THEN delta / interval ELSE 0 ENDspells 'unknowable' as 0" — does not apply — no rate, delta or aggregate anywhere in this diff.
Divergence (blind read vs PR body)
None that changes a verdict. Three narrower readings, none a defect:
- The body calls
LongQueryTraceAppliedAtUtc"the time of the last pass that created the session"; it is stamped on every successful pass including the off-side one (DarlingWorker.cs:4188). Harmless —CreateOnlyis gated onenabled(:4167). - "Cross-registration: No" reads as one guard; it is two with different coverage —
CanTouch/ownedElsewherefor a read-write separate registration,KeptElsewherefor a read-only one (whichAzureMasterScopeomits). I walked all four on/off combinations of both shapes and found no hole. - The verb's self-exclusion by
selfIdholds only becauseDarlingServerConnector.cs:426stampsServerIdfromconfig.ServerIdandDarlingCliCommands.cs:6156passes the same resolved list. On the store-unreachable fallback the ids collapse and nothing is kept — failing toward dropping more, the safe direction. Unmentioned, not wrong.
Reviewer checklist
- Build both projects and run the two full suites at this head — the body discloses they were not run, and this LGTM rests on test names, not a green run.
- Live Azure logical server with a database also registered as its own server: toggle off then on, confirm
sys.database_event_sessionsmatches the monitored set and the separate database is untouched both ways. - The read-only-intent case on a tier that has a real read-only replica — "a drop on the primary removes it from the replicas too" is marked inferred and is what makes
KeptElsewhere's intent-blindness correct. - A logical server where every user database is registered separately: watch one
long_query_completionsrun row, since the list is then empty and a zero-row read is the shape this codebase fights. -
--drop-xe-sessionswith an unopenable excluded database (note on stderr, exit 0) and an unopenable monitored one (exit 2). - Kill a database-scoped session under a running Darling service; confirm it returns within the hour, not at the next reconnect.
Its findings are fixed at 5712458, where the review reads LGTM.
What changes
On Azure SQL Database, the opt-in long-query trace uses a database-scoped Extended Events session. This PR changes where Lite and Darling create and drop that session, and it corrects the text that describes it. It also widens where Darling's
--drop-xe-sessionssearches for that session to the databases the server excludes. An excluded database the verb cannot open is reported in a note and does not fail the command. On every other engine the session is server-scoped, and that path does not change. The always-on deadlock and blocked-process sessions do not change either.Each fact in this body is observed in the code or in a test, unless it says "documented" (Microsoft Learn, linked below) or "inferred".
The rule is in one place,
LongQueryTraceDatabasesinPerformanceMonitor.Collectors, so both apps compute the same sets:master.The monitored set in each app:
ExcludedDatabases, or the one database that a registration names.DarlingCollectorRunner.DatabaseScopeFor, which calls the same delegate as the read loop.The trace is opt-in because of its overhead, so it must not run in databases that the user left out. The always-on sessions keep their inventory rule (#3477). One list provisions both of them, and each of their collectors can have its own scope. A comment beside the always-on listing in
DarlingXeSessions.csnames both rules.When the apps apply the rule
LongQueryTraceDatabases.StateKey) holds the on or off state, the separately monitored databases, and the other registrations that keep the session. While the trace is on, it also holds the database scope and the exclusions. A change to any of them runs the plan again, so a newly excluded or out-of-scope database loses its session.Failures and retries
On_AFailedListing_IsNotMarkedAppliedshowed this.LongQueryTraceDropExceptionthat names each database that failed, and the next cycle tries again.LongQueryTraceDropRetry, and each app passes the time in, so the tests drive it.A session dropped from outside
LongQueryCompletionsCollector.BuildQuery) joinssys.dm_xe_session_targetstosys.dm_xe_sessions, or their database-scoped forms, on the session name. An absent session gives zero rows and no error (observed in the code). So the read's error arm for errors 297 and 15151 never sees it, and there is no error on which to clear the latch.ServerLoopState.LongQueryTraceAppliedAtUtcholds the time of the last pass that created the session. A reconnect resets it with the latch.LongQueryTracePass.Full,CreateOnlyorRetryAfterCap. Only the Azure SQL Database arm reads it, because the server-scoped arm has no drop while the trace is on. On other engines the hourly pass runs the same create check as a connect.Databases monitored as their own servers
The rule uses the existing helper
AzureMasterScope.SeparatelyMonitoredDatabases. Lite calls it throughKnownEngineEditions. Darling calls it with the live server registry, which is the same set that the alert sweep uses.Both read loops now skip the same databases, for this collector only. The new member
SkipsSeparatelyMonitoredDatabasesis true only onLongQueryCompletionsCollector. Assume that the logical server's registration reads such a database while that database's own trace is off. Lite's read then fails there on every cycle (#4731), and Darling's read returns zero rows that look like a quiet trace. While that database's own trace is on, the events are stored twice.The helper has two rules. For session ownership:
Source for the documented facts: Read queries on replicas. Its section "Monitor read-only replicas with Extended Events" covers the session's definition. Its introduction says that the Basic, Standard and General Purpose tiers have no replicas and no read scale-out. It also says that with read scale-out turned off, a connection reaches the primary whatever its
ApplicationIntent.Registrations that share a session
So the drop follows its own rule,
LongQueryTraceDatabases.KeptElsewhere. A drop skips a database while another registration keeps the session there. That registration must be enabled, belong to the same logical server, and have the trace on. Its read-only intent does not matter. A registration keeps the session in these databases:master, the databases monitored as their own servers, and the databases that it excludes.The rule applies to every drop: the drop while the trace is off, and the drop outside the set while it is on. It applies to a logical server's registration and to a registration of one database. Creates do not change.
The coverage is exact. Suppose two logical server registrations both exclude a database. Neither one keeps the session there for the other, so a session left there is still dropped.
The list of databases monitored as their own servers is the same for every registration of a logical server, including a registration of one database. Each app computes it as the logical server's registration sees it:
KnownEngineEditions.SeparatelyMonitoredDatabasesOnServerin Lite, andDarlingWorker.LongQueryTraceServerSeparatelyMonitoredin Darling. A registration's own list does not work here. For a registration of one database, that list is empty. So its own database counts as kept by the logical server's registration, which never creates the session there. The drop is then skipped, and the session stays.Each app builds the registrations beside its list of separately monitored databases:
GetScheduleForServer) and its exclusions. Lite has no database scope.ResolveSchedule, its exclusions, and its scope fromDatabaseScopeFor. The registry holds only monitored servers, so each one is enabled.The state key holds the other registrations that keep the session (
LongQueryTraceDatabases.CoOwners). A registration of one database adds its database. A logical server's registration adds its exclusions, its scope and the list of separately monitored databases. So when the other registration turns its trace off, or changes what it covers, the skipped drop runs on the next pass.The two open cases from round 2 are closed
master. In both apps, each one's drops now skip the databases that the other one keeps. Lite accepts such a pair when the two differ in read-only intent, as the text of its duplicate check says. Darling accepts it too.ServerIdHelper.BuildStorageNameadds:ROto the identity. The comment onActualIdentityCollisionsays that a read-only-intent registration beside a read-write one is legitimate. So the tests for this case run in both apps.One limit remains. A registration of one database with read-only intent and the trace on, connected to a read-only replica, cannot create the session there. Its trace works only while a registration on the primary keeps the session's definition (documented).
Can any path drop a session that holds unread events for a database that the user still monitors?
RecreateDatabaseScopedXeSessionAsyncin Lite andEnsureOneDatabaseScopedAsyncin Darling.Text
ScheduleManager.cs,collection_schedule.json) and Darling's CLI help now say that on Azure SQL Database the session is created in each monitored database.LongQueryCompletionsCollector.SessionScopeSentencestill holds its last sentences. Only the Azure SQL Database sentences above changed.--drop-xe-sessionsprints a warning, and the README quotes it. Both now say that Darling creates a missing long query completions session again within an hour. Before, they said only "on its next connect".=inServerTab.LongQueries.csandViewerServerTab.LongQueries.cs. An earlier commit in this PR removed those spaces, and a later one put them back. At this head both lines match dev.Out of scope
Lite and Darling use the same session name, so the two apps share one session in a database that both monitor. A separate change handles that.
Tests
The hourly attempt and the hourly create pass
The new tests pass a time to the reconcile, and Darling's tests pass a pass kind, so they cannot compile at 824ea3d. Commit 7e5c34e adds the clock, the pass kind and the tests, with the behavior of 824ea3d. At that commit the cap forgets its state when it gives up, and the warning keeps its old last sentence. Darling never picks the create pass. Commit 13dcb7a is the fix.
Lite, RED at 7e5c34e:
Off_AfterTheCap_TriesAgainOnceAnHour_AtDebug_WithNoSecondWarning:Assert.Contains() Failure: Sub-string not found. It fails first on the warning's last sentence. Its later checks: nothing runs within the hour, one attempt runs an hour after the cap, and the cycle after it runs nothing. A second failure an hour later logs at Debug too, and no Warning follows the cap's one.AfterTheCap_AnAttemptThatSucceeds_EndsTheHourlyAttempts:Assert.Equal() Failure: Collections differOn_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttempt:Assert.Equal() Failure: Collections differDarling, RED at 7e5c34e:
Off_AfterTheCap_TriesAgainOnceAnHour_AtDebug_WithNoSecondWarning:Assert.EndsWith() Failure: String end does not match. It makes the same checks as the Lite test, and checks Darling's reconnect sentence.AfterTheCap_AnAttemptThatSucceeds_StartsTheCountAgain_AndEndsTheHourlyAttempts:Assert.Equal() Failure: Collections differOn_ASessionDroppedFromOutside_IsCreatedAgainAnHourLater_AndNothingIsDropped:Assert.Contains() Failure: Item not found in set. The registration excludes gamma, so a pass that runs the drop side drops gamma again. The test checks that nothing is dropped.Guard, which passes at both commits:
On_WithinTheHour_TheLatchedReconcileRunsNothing(Darling). Within the hour, a latched reconcile lists nothing and touches no database.Red-watch for Lite's reset: put the old reset back, so that every finished reconcile resets the count. Then
On_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttemptfails, and the other 20 tests in its class pass. With the fix, all 21 pass.Existing tests that changed:
PostgresEngineGateBehaviorTests(Darling): the session reconcile takes the pass kind, and the gate tests passFull.TheSweep_PassesTheRegistrationsAndTheServersOwnList_FromTheLiveRegistry(Darling): the sweep now passesDateTime.UtcNowto the reconcile.Registrations that share a session
The new tests call members that do not exist at 7cc63c1, so they cannot compile there. Commit d1a8499 adds the registrations,
KeptElsewhere, the co-owner part of the state key, and the tests. At that commit,KeptElsewhereand the co-owners count no other registration. The state key joins its parts as before. So the behavior is the same as at 7cc63c1. Commit 464e314 is the fix.Lite, RED at d1a8499:
Off_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOnReadOnlyRegistration_Off_LeavesItsDatabase_WhileTheLogicalServersTraceIsOnAndCoversItOff_LeavesTheDatabasesOfASecondLogicalServerRegistration_WhileItsTraceIsOnOn_TheOutsideDrop_LeavesAnExcludedDatabase_ThatASecondRegistrationsTraceCoversOff_OnceTheOtherRegistrationTurnsItsTraceOff_TheNextPassDropsTheLiteBanner_OnAzureSqlDatabase_SaysEachMonitoredDatabase_NotThisServerDarling, RED at d1a8499:
Off_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOnReadOnlyRegistration_Off_LeavesItsDatabase_WhileTheLogicalServersTraceIsOnAndCoversItOff_LeavesTheDatabasesOfASecondLogicalServerRegistration_WhileItsTraceIsOnOn_TheOutsideDrop_LeavesAnExcludedDatabase_ThatASecondRegistrationsTraceCoversOff_OnceTheOtherRegistrationTurnsItsTraceOff_TheNextPassDropsTheStateKey_TellsWhereTheScopeEndsAndTheExclusionsBeginTheViewerBanner_OnAzureSqlDatabase_SaysEachMonitoredDatabase_NotThisServerTheReadme_SaysTheSessionIsPerDatabase_OnAzureSqlDatabaseGuards, which pass at both commits:
ReadOnlyRegistration_Off_DropsItsDatabase_WhenTheLogicalServerExcludesIt(each app): the coverage is exact, so the read-only registration still drops a database that the logical server excludes.On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude(each app): a second logical server registration does not cover every database.DatabaseRegistration_Off_DropsItsDatabase_WhileTheLogicalServerLeavesItToIt(each app): the list of separately monitored databases is the logical server's view. The Darling test builds it with the worker's own function.On_ADatabaseMonitoredAsItsOwnServer_AndScopedOut_IsNotDropped(Darling) andOn_ADatabaseMonitoredAsItsOwnServer_AndExcluded_IsNotDropped(Lite): the other registration's trace is off, so only the plan's own-server rule keeps that database. WithCanTouch(database) &&deleted from the drop outside the set inLongQueryTraceDatabases.Plan, both tests fail. With it restored, both pass.TheRegistrations_AreTheLiveOnesOfTheSameLogicalServer_WithTheirTraceSettingAndScope(Darling): the worker's registration function.TheSweep_PassesTheRegistrationsAndTheServersOwnList_FromTheLiveRegistry(Darling): a source pin on how the sweep passes them, because the sweep needs a live registry.Existing tests that changed:
PostgresEngineGateBehaviorTests(Darling): the reconcile now takes the registrations and the logical server's list. The gate tests pass empty ones.SwallowedItemFailureTests(Darling): its pin on the create loop's text includes the newkeptElsewhereargument.LongQueryBannerSessionScopeTests(both apps): the new banner and README sentence.Round 1
The round 1 tests call test hooks that do not exist on dev, so they cannot compile there. Commit 2e98575 adds the hooks and the tests with no change in behaviour. At that commit, 8 of 9 Lite tests and 9 of 10 Darling tests failed, each on the behaviour that it pins. Commit 821b82e is the fix. The hooks are null in production.
Lite,
LongQueryTraceLifecycleLiteTests, RED at 2e98575:Off_DropsTheSessionEverywhere_ExcludedDatabasesIncluded:Assert.Equal() Failure: Collections differOff_AFailedListing_IsNotMarkedDone_AndTheNextCycleListsAgain:Assert.Null() Failure: Value of type 'Nullable<bool>' has a valueOff_OneDatabaseRefusesTheDrop_TheOthersAreStillDropped_AndTheNextCycleRetries:Assert.Null() Failure: Value of type 'Nullable<bool>' has a valueOff_TheFifthFailedPassInARow_IsMarkedDone_WithOneWarningThatNamesTheDatabase:Assert.Null() Failure: Value of type 'Nullable<bool>' has a valueOff_LeavesTheSessionOfADatabaseMonitoredAsItsOwnServer:Assert.Equal() Failure: Collections differ at index 1On_AnExclusionChange_DropsTheSessionFromTheNewlyExcludedDatabase:Assert.Equal() Failure: Collections differOn_DoesNotCreateTheSessionInADatabaseMonitoredAsItsOwnServer:Assert.Equal() Failure: Collections differ at index 1TheLongQueryRead_SkipsTheDatabasesMonitoredAsTheirOwnServers:Assert.Contains() Failure: Sub-string not foundDarling,
LongQueryTraceLifecycleTests, RED at 2e98575:Off_DropsTheSessionEverywhere_ExcludedAndOutOfScopeDatabasesIncluded:Assert.Equal() Failure: Collections differOff_AFailedListing_IsNotMarkedApplied_AndTheNextSweepListsAgain:Assert.Null() Failure: Value of type 'Nullable<bool>' has a valueOff_OneDatabaseRefusesTheDrop_TheOthersAreStillDropped_AndTheNextSweepRetries:Assert.Null() Failure: Value of type 'Nullable<bool>' has a valueOff_TheFifthFailedPassInARow_IsMarkedApplied_WithOneWarningThatNamesTheDatabase:Assert.Null() Failure: Value of type 'Nullable<bool>' has a valueOff_LeavesTheSessionOfADatabaseMonitoredAsItsOwnServer:Assert.Equal() Failure: Collections differ at index 1On_AScopeChange_DropsTheSessionFromTheDatabaseNowOutOfScope:Assert.Equal() Failure: Collections differOn_AFailedListing_IsNotMarkedApplied:Assert.Null() Failure: Value of type 'Nullable<bool>' has a valueOn_DoesNotCreateTheSessionInADatabaseMonitoredAsItsOwnServer:Assert.Equal() Failure: Collections differ at index 1TheLongQueryRead_SkipsTheDatabasesMonitoredAsTheirOwnServers:the per-database read filters its list after listing itThe guard
TheAlwaysOnSessions_StillFollowTheInventory, in each app, passed at 2e98575 and passes now. It checks that the always-on create step still lists every inventoried database with no scope.Existing tests that changed in round 1:
SwallowedItemFailureTests(Darling): the create loop now runs over the plan's create list, and the drops go throughLongQueryTraceDatabases.DropEachAsync. The test follows the new code. It still checks that only the create side is scored.CollectorDatabaseScopeRungTests(Darling):DarlingXeSessions.cshas one unscoped listing fewer, because the trace's on-side listing now passes the scope. The test also checks that the lifecycle gets the scope throughDatabaseScopeFor.LongQueryBannerSessionScopeTests(both apps): the new banner and README text.TsqlConventionGuardTests(Darling): its member scan stops short in the two newListLongQueryTraceDatabasesAsyncmembers, in Lite and in Darling. Each is an expression-bodied member that opens with a property pattern, the shape that the list already carries for Major store upgrade drops operator settings: postgresql.auto.conf is not carried to the new cluster #4253. Two method calls and no string literal fall outside the range. Both members are added toKnownTruncatedRanges.RepoFileAdoptionTests(Darling): the new Darling test file reads a repo file through the LF reader, because one anchor spans a line break. The file is added to the declared list.Last round: hourly passes that throw, the edition read, the verb's search, the exclusion clause
Seen means the code or a test showed it. Inferred means it follows from the code and no test ran it.
LongQueryTraceAppliedAtUtcan hour old, so every later sweep ran another pass and logged another Warning. An attempt after the cap that threw something other than a failed drop never calledRecordFailure, soRetryDuestayed true. Now the create-only pass sets the clock. The attempt after the cap callsRecordFailureand, while the trace is on, sets the clock too. The second part goes one step past the plain fix: without it, the sweep after a throwing attempt runs a create-only pass, because that clock is still an hour old. A full pass keeps its retry on each sweep. The session-missing fault stays set until a pass succeeds (seen in the first new test).LongQueryTraceDropException(the listing and each drop are wrapped), so a non-drop exception there can only come from the create, which runs every cycle anyway.RetryDuestaying true then only keeps the due attempt for when the create works again. No test pins this.RemoteCollectorServicenow has its ownKnownEngineEditions, resolves the edition once, and passes the answer to the create and the drop. Those two were the only callers. A blank status now falls back to the edition this app last saw for the server, so the first reconcile after a start with a blank status is judged as before.Runtime.Target.IsAzureSqlDb(the worker's reconcile and registrations, the session reconcile, the runner's separately-monitored delegate, and the verb's target).IsAzureSqlDbisinit-only onCollectorTargetInfo, so it cannot change under one runtime.--drop-xe-sessionsapplies the exclusions to the long-query session. Seen: the verb listed databases with the server's exclusions, so it never searched an excluded database, and the long-query session stayed there. Now, for that session only, it searches every online database, the exclusions not applied, as the off-side reconcile does. It skips master, a database monitored as its own server, and a database where another registration of the logical server keeps the session. It reuses the worker's rule (LongQueryTraceRegistrations,LongQueryTraceServerSeparatelyMonitored,LongQueryTraceDatabases.PlanandKeptElsewhere). The deadlock and blocked-process sessions keep today's list. The verb gets the registry's servers but cannot read another registration's long-query schedule, so it counts every other registration of the logical server as keeping the session. Inferred: when two registrations of one logical server both have the trace off, each run of the verb leaves the session in the databases the other covers. The two doc comments, the README and the verb's usage text now name the sets. It does a second listing of master only when the server has exclusions.applyExclusionsternary. The ternary now sits in its own small member (DarlingCollectorRunner.AzureDatabaseListPlan,RemoteCollectorService.BuildAzureDatabaseListQuery), with no change in behavior. The new test in each app checks that withapplyExclusionsfalse and excluded databases set, the query has noNOT INclause and no parameters, and with it true, it has the clause and two parameters.LongQueryTraceDropRetry.ConsecutiveFailuresis removed. Seen: nothing in the product read it. One Darling test asserted it, and that line is gone. The test still asserts the next attempt is cleared and nothing runs an hour later.An excluded database the verb cannot open
Seen: the long-query search opens the databases the server excludes. Before, a database that refused the connection set exit code 2, even when every session found was dropped. Before this PR the verb never opened an excluded database. A login with no user there, or a paused serverless database, then stopped a script that removes the server next. Now
AzureSearchPlan.Unsearchedfiles a monitored database it cannot open as a problem, with exit 2 as before. It files an excluded one as a note on stderr, and the exit code stays the same. The usage text and the README say so.The summary of
ReconcileLongQueryCompletionsAzureAsyncnow says why the create side has no cap. It retries on every sweep, so the fault is recorded again and the run readsSESSION_MISSING. A CREATE that every database refuses has the same cadence. Only the drop side is capped. Lite and Darling log a Warning on every cycle while the long-query session cannot be created #4964 tracks the repeated Warnings this logs.RED, with the note's record and seam in place and the old behavior (not a separate commit),
DropXeSessionsVerbTests4 failed of 92:AnExcludedDatabaseTheLoginCannotOpen_IsANoteOnStderr_AndTheRunStillSucceeds:Expected: 0, Actual: 2ADatabaseSearchedOnlyForTheLongQuerySession_IsFiledAsANote_AndAMonitoredOneAsAProblem:Assert.False() FailureANoteFromTheSearch_IsPrintedOnStderr_AndLeavesTheExitCodeAndTheDropsAlone: expected the note, got an empty stringANoteWithNothingFound_StillSaysNothingWasDroppedOnlyInThePlacesSearched:Sub-string not foundThe guard
AMonitoredDatabaseTheLoginCannotOpen_StillFailsTheRun_WithTheUnavailableCodepasses before and after.RED at f07fc9e (the tests-first commit, with the seams in place and the old behavior):
LongQueryTraceLifecycleTests.On_AnHourlyCreatePassThatThrows_RunsAgainAnHourLater_NotOnEverySweep:Assert.Equal() Failure: Values differLongQueryTraceLifecycleTests.On_TheAttemptAfterTheCap_ThatThrowsAnotherError_RunsAgainAnHourLater_NotOnEverySweep:Assert.Equal() Failure: Values differLongQueryTraceLifecycleLiteTests.Off_AnEditionThatChangesDuringTheReconcile_StillLeavesTheDatabasesAnotherRegistrationKeeps:Assert.Empty() Failure: Collection was not emptyDropXeSessionsVerbTests.TheLongQuerySearch_VisitsAnExcludedDatabase_AndSkipsOneMonitoredAsItsOwnServer:Assert.Equal() Failure: Collections differ at index 0DropXeSessionsVerbTests.TheLongQuerySearch_SkipsADatabaseAnotherRegistrationOfTheServerKeeps:Assert.Equal() Failure: Collections differ at index 0DropXeSessionsVerbTests.ATargetWithItsOwnNames_SearchesNoDatabaseForTheLongQuerySession:Assert.Empty() Failure: Collection was not emptyThe two exclusion-clause tests describe behavior the code already had, so they pass at that commit. Proof that they pin it: with each ternary inverted,
TargetProviderTests.TheRunnersAzureDatabaseList_AppliesTheServersExclusionsOnlyWhenAsked(Darling) andLongQueryTraceLifecycleLiteTests.TheAzureDatabaseList_AppliesTheServersExclusionsOnlyWhenAsked(Lite) both fail withAssert.DoesNotContain() Failure: Sub-string found. Both ternaries are restored.At cf5bd03 with 0 build warnings in both projects, these classes pass: Darling
LongQueryTraceLifecycleTests28 total, 0 failed;DropXeSessionsVerbTests87 total, 0 failed;TargetProviderTests30 total, 0 failed;PostgresEngineGateBehaviorTests8 total, 0 failed, 4 skipped. LiteLongQueryTraceLifecycleLiteTests23 total, 0 failed;LongQueryBannerSessionScopeTests3 total, 0 failed. Those runs came before the README and usage-text edit of the verb. That edit is covered only by the full run below.Existing tests that changed in this round:
DropXeSessionsVerbTests(the injected connector now also receives the registry's servers).Full suites
At 5712458 (dev merged in), with 0 build warnings in both projects:
DropXeSessionsVerbTests,DropXeSessionsHelpGuidanceTests,DropXeSessionsLiveTests,LongQueryTraceLifecycleTests,TsqlConventionGuardTestsandDocCommentHygieneTests, 227 run, 0 failed, 1 skipped (a live test that needs a SQL Server).CHANGELOG
No entry in this PR; the release owner adds it under [Unreleased].
Proposed entry (changed in the last round, because the verb now does something new for a user):