Repository navigation
Web read latency: each web server records into its own accumulator (#4782) - #4839
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes #4782: the #4785 fix covered the test-only dispatch entry but not the accumulator the dispatch records into.
Why
CI run 36592791128, on #4829's branch, failed
ReadLatencyWebRecordingTests.ADispatchEntryThatThrowsA57014PostgresException_RecordsOneWebSample_WithOutcomeTimeout. The response was the expected 503, butAssert.Single(line 193) found no sample in the test's own accumulator.DarlingWebEndpointskept the read-latency accumulator and its logger in two statics,s_readLatencyands_readLatencyLogger. EveryMapAllcall overwrote them, and the record sites read them at request time. Six test classes callMapAll, and xUnit runs classes in parallel. When another class built its server between this test'sBuildServerand its request, the static pointed at that class's accumulator and the sample landed there. The test's class comment said that capturing its own accumulator "sidesteps that race entirely". It did not, because the product code never read the captured instance.Production calls
MapAllonce per process, so only test runs were exposed to the race.What changes
ReadLatencyRecorder.cs(new): a small public sealed class holding the accumulator and the logger a server was given.DarlingWebEndpoints.cs: both statics are deleted.MapAllbuilds one recorder from the accumulator and logger it was given, and both record sites close over it. The/api/read/*dispatch loop records through it, and the/api/compose/runroute passes it toRunComposedPanelAsync(new optional last parameter, so direct callers in tests still compile and record nothing).RecordWebReadLatencyandRecordComposeLatencytake the recorder.run_custom_view_panel: the tool calls the sameRunComposedPanelAsync, which used to find the accumulator in the static. Deleting the static would have dropped that tool's compose sample, so the tool now takes the recorder as an optional DI service parameter (never in the advertised schema), andConfigureMcpServicesregisters one over the same accumulator its per-call latency filter already records into. One production difference follows: only the web startup used to fill the static, so with the web dashboard off (its default) these runs recorded nothing. They are now counted ascomposesamples. With the dashboard on, the accumulator is the same process-wide instance as before.McpServiceParameterDiSeatCensusTests: the pinned set of DI service parameter types gainsReadLatencyRecorder, and the comment names Darling tests: a test-only read route leaks into other test classes running at the same time #4782.ReadLatencyWebRecordingTests.cs: the class comment now says what makes each server's samples its own.ExtraDispatchEntryScopebecame internal so the new class reuses it. Every assertion is unchanged.ReadLatencyPerServerRecordingTests, two facts. Each builds server A with accumulator A, then server B with accumulator B, then sends the request to A. One drives a 57014 dispatch entry through the/api/read/*loop and asserts exactly oneWebsample with outcomeTimeoutin A and none in B. The other posts a body with nopanelto/api/compose/runand asserts exactly oneComposesample (outcomeError) in A and none in B.McpToolLatencyRecordingTests.RunCustomViewPanel_RecordsOneComposeSample_IntoTheAccumulatorTheHostWasGiven: the MCP tool's run lands as oneComposesample in the accumulator the MCP host was built with, and noMcpsample.RunCustomViewPanel_RecordsNoMcpSamplesent aview_idargument that the tool does not declare. The unknown-argument guard refuses that before the tool or the latency filter runs, so the test held without the filter's skip for this tool ever being exercised. It now sends the tool'sspecargument. The assertion is unchanged.Test plan
ReadLatencyPerServerRecordingTests(both facts) and the newMcpToolLatencyRecordingTestsfact fail withAssert.Single() Failure: The collection did not contain any matching items.Total: 7, Failed: 3for the two classes; the other four pass.McpToolLatencyFilter.SkippedToolNameto another string failsRunCustomViewPanel_RecordsNoMcpSampleand the new MCP fact (Total: 5, Failed: 2). The plant is not in the branch.Darling.Testsbuilds with0 Warning(s),0 Error(s), after merging the latestorigin/dev.MapAllor namesReadLatencyAccumulator,DocCommentHygieneTests, and the MCP tool, schema and pin classes (McpServiceParameterDiSeatCensusTests,McpToolsListBudgetTests,McpAlwaysLoadedToolsTests,McpUnknownArgumentGuardTests,McpReadCommandTimeoutTests,DarlingMcpCustomViewToolsTests,DarlingComposeTests), with noDARLING_TEST_PG, on the final build:Total: 706, Errors: 0, Failed: 0, Skipped: 12.Darling.Testssuite, once, with noDARLING_TEST_PG:Total: 18004, Errors: 0, Failed: 0, Skipped: 1166, Not Run: 1. The skips are the live PostgreSQL classes, which needDARLING_TEST_PG. The one not-run test is explicit-only. The suite ran before the last two commits: one changes a single test's request, the other rewraps a comment. The classes those commits touch were re-run afterwards (the 706 above, thenTotal: 7, Failed: 0for the MCP-latency and per-server classes, thenTotal: 78, Failed: 0for the seat census andDocCommentHygiene).ReadLatencyWebRecordingTestsfacts were not run locally (noDARLING_TEST_PG); CI's PostgreSQL jobs run them.CHANGELOG
SECTION: Fixed
ENTRY:
run_custom_view_panelruns count in read latency with the web dashboard off ([Web read latency: each web server records into its own accumulator (#4782) #4839]): the run's timing went to a slot only the web dashboard filled. With the dashboard off (the default), these runs were left out. They are now recorded as composed-panel reads, the same as a panel run from the web dashboard.REF:
[Web read latency: each web server records into its own accumulator (#4782) #4839]: Web read latency: each web server records into its own accumulator (#4782) #4839