Add opt-in exception summarization for server-side logging - #1804
Add opt-in exception summarization for server-side logging#1804AkbarDizaji wants to merge 1 commit into
Conversation
e07f499 to
fb61315
Compare
CI: the earlier
|
| Commit | Failing leg | Test |
|---|---|---|
a9f02522 |
windows-latest, Debug | ClientConformanceTests.RunConformanceTest(scenario: "auth/offline-access-scope") |
e07f4999 |
windows-latest, Release | OAuth.AuthTests.CannotAuthenticate_WithInvalidClientMetadataDocument |
fb61315a |
— | all green |
The three commits differ only in comments, review fixes, and tests. A defect in the diff would not move between unrelated tests and then disappear.
The same tests fail on main. 13 of the last 40 Build and Test runs on main failed; 4 of the 5 I sampled failed on a windows-latest leg, on this same auth-over-in-memory-Kestrel cluster:
main@87f5b09b—ClientConformanceTests.RunConformanceTest(scenario: "auth/offline-access-scope"), the exact test that failed ona9f02522, plusOAuth.DcrFailureTests.DcrRejection_PropagatesToConsumer_WithStatusBodyAndSentParametersmain@81ae6ec6—OAuth.DcrFailureTests.DcrRejection_PropagatesToConsumer_WithStatusBodyAndSentParametersagain
This PR cannot reach that code path. The only structural risk was the new trailing optional parameter on McpSessionHandler's constructor, so I checked it directly rather than assuming:
McpSessionHandlerdeclares exactly one constructor, so there is no overload set and no overload resolution to perturb.- It is never constructed reflectively — no
ActivatorUtilities,Activator.CreateInstance, ortypeof(McpSessionHandler)anywhere insrc/ortests/. - There are exactly two call sites:
McpServerImpl.cs:158(passes the new argument explicitly) andMcpClientImpl.cs:59(relies on the default).McpClientImpl.csdoes not appear in this PR's diff at all — its eight positional arguments are byte-identical tomain, and the new parameter is appended last, so nothing before it shifts. _exceptionSummarizeris therefore alwaysnullon the client path, so both new wrappers take theelsebranch and emit the original calls unchanged.- The diff touches no file under
Authentication/,Client/, any transport, orModelContextProtocol.AspNetCore. The thrown message originates inClientOAuthProvider.cs:633, which is untouched.
Why it is Windows-specific. OAuthTestBase derives from KestrelInMemoryTest and hosts TestOAuthServer with listenOptions.UseHttps(), so every OAuth metadata request performs a real TLS handshake over an in-memory duplex pipe. When that handshake or request is slow, GetAuthServerMetadataAsync gets a TaskCanceledException, ClientOAuthProvider folds it into "Failed to find .well-known/openid-configuration or ...", and the Assert.StartsWith("Failed to handle unauthorized response") fails on the substituted message. The Windows runners are consistently the slowest in this repo — 17–18 min versus 12–13 min for ubuntu/macOS on this very PR — which fits a timing-sensitive handshake failing there first.
I found no existing issue for CannotAuthenticate_WithInvalidClientMetadataDocument or for the .well-known/openid-configuration message; #1701 tracks a different flaky Windows test. Happy to open one for this OAuth/TLS-over-in-memory-pipe cluster if that would be useful.
Update — ubuntu-latest, Debug on c7e65159. One leg failed on RawHttpConformanceTests.July2026Post_MissingRequiredCapability_Returns400 (Expected: BadRequest / Actual: OK, net8.0). That is #1772, "SEP-2575 HTTP status mapping is timing-sensitive under load", which is still open: StreamableHttpPostTransport.DeferredHeaderFlushGrace is a hardcoded 250 ms, so a dispatch slower than that window commits a default 200 and the MissingRequiredClientCapability error rides the committed status. #1795 proposed making the grace configurable and was closed unmerged, so the behavior is unchanged on main.
The same test failed the same way on the same leg on main @ 514cf68a, which is not in this branch's history. Both Windows legs, both macOS legs, and ubuntu-latest, Release passed on this head, and the full ModelContextProtocol.AspNetCore.Tests suite (593 tests) passes locally.
This PR cannot influence that mapping: it touches no HTTP or transport code, and the only difference from the previous all-green head (fb61315a) is inside ExceptionSummaryHelper.TrySummarize, which is unreachable unless a summarizer is configured — and no test in ModelContextProtocol.AspNetCore.Tests configures one.
Verification detailSupporting evidence for two decisions in the description, plus one environmental caveat. The shared EventId was verified against the generated code
I built with
Across the whole file the only differences from baseline are the seven summarized ids disappearing and being replaced by their raw counterparts' id and name. No unrelated event's id changed. The same result holds for the other six pairs:
Why the DI mapping is
|
| member | value |
|---|---|
ExceptionType |
InvalidOperationException |
Description |
Unknown |
AdditionalDetails |
-2146233079 |
ToString() |
InvalidOperationException:Unknown:-2146233079 |
Description is "Unknown" for any exception type no registered provider handles, so on its own it drops the exception type and conveys nothing. ToString() appends AdditionalDetails, which the package documents as a field that "may contain privacy-sensitive information and is therefore not suitable" for telemetry — with the same warning repeated on ToString() itself.
The mapping is therefore $"{summary.ExceptionType}: {summary.Description}", using the two members documented as free of privacy-sensitive information. A test asserts that the real AddExceptionSummarizer() path preserves the exception type and does not leak the exception message.
AOT: one environmental caveat
dotnet publish of ModelContextProtocol.AotCompatibility.TestApp emits zero IL2xxx/IL3xxx warnings with TrimmerSingleWarn=false, and the ILC step completes. The final native clang link then fails on my macOS machine for missing -lssl/-lcrypto. I reproduced that identically on a clean checkout of main, so it is a local toolchain issue and not something this branch introduces; CI's AOT leg is the authority there.
Adds McpServerOptions.ExceptionSummarizer, an optional delegate that maps an Exception to a sanitized description. When set, server-side failure paths log that description instead of attaching the raw Exception. When null (the default), every callsite logs exactly as before. The delegate runs only when the level of the event being written is enabled, matching the enabled-check the generated logging methods perform internally, and each summarized variant reuses its raw counterpart's EventName so both emit the same EventId. Core stays free of a hard dependency on Microsoft.Extensions.Diagnostics.ExceptionSummarization; the DI package takes the package reference and McpServerOptionsSetup populates the delegate from an optionally-registered IExceptionSummarizer. Fixes modelcontextprotocol#1690
fb61315 to
c7e6515
Compare
Fixes #1690.
What
Adds an opt-in way to log a sanitized description of an exception instead of the raw
Exceptionon server-side failure paths. Most providers render an exception as its message plus stack trace, which can carry sensitive runtime data; this gives hosts one place to sanitize that without replacing their provider.Behavior
Opt-in only. If the delegate is
null, every callsite invokes the existing[LoggerMessage]method with the rawException— theelsebranch of each new conditional is byte-for-byte the original code.When a summary is produced the entry carries that string only; attaching the raw
Exceptiontoo would defeat the purpose. The delegate runs only when the event's level is enabled, matching the check the generated logging methods perform internally, so it costs nothing on a filtered-out level. If it throws or returnsnull, the raw exception is logged instead, so a faulty summarizer can never fail the session.Design decision — and a question for maintainers
ModelContextProtocol.Coredepends only onMicrosoft.Extensions.Logging.Abstractionsplus polyfills, so I keptMicrosoft.Extensions.Diagnostics.ExceptionSummarizationout of it: Core exposes a plainFunc<Exception, string>?onMcpServerOptions, and the DI package takes the package reference and populates the delegate from an optionally-resolvedIExceptionSummarizer(GetService, neverGetRequiredService). That package shipsnetstandard2.0assets as of 10.8.0, so a direct Core reference would also work across every TFM here without conditional-TFM guards. If you'd rather Core reference it directly and exposeIExceptionSummarizeron the options, I'm happy to switch.Shared EventId
Unlike the existing
*Sensitivepairs, which a consumer selects by choosing a log level, the raw and summarized forms of an event are selected by server configuration — so they are one logical event, and each summarized method setsEventName = nameof(<RawMethod>)to emit the same EventId and name. That keeps EventId-based filters and alerts working when a summarizer is configured. The*Sensitivevariants do carry distinct ids, so if you'd prefer these follow that convention, I'll drop theEventNamelines.Callsites changed
McpSessionHandler:LogRequestHandlerException,LogMessageHandlerException/LogMessageHandlerExceptionSensitive.McpServerImpl:ToolCallError(2 callsites),GetPromptError,ReadResourceError,MrtrHandlerError.McpSessionHandleris shared by client and server; the summarizer reaches it through a new optional trailing constructor parameter that onlyMcpServerImplpasses, so client sessions are untouched.Raw-exception log callsites intentionally NOT changed
The issue scopes this to server-side handler failure paths.
McpServerImpl.SubscriptionNotificationFailedTransportBase.*(LogTransportConnectFailed,LogTransportSendFailed,LogTransportMessageParseFailed(Sensitive),LogTransportReadMessagesFailed,LogTransportShutdownFailed,LogTransportCleanupReadTaskFailed,LogTransportEndpointEventParseFailed(Sensitive))McpServerOptionsdoes not reach them.StreamableHttpPostTransport.LogStoreStreamDisposalFailed/LogDeferredHeaderFlushFailedStdioClientTransport.*,StdioClientSessionTransport,StreamClientSessionTransport,SseClientSessionTransport,StreamableHttpClientSessionTransportMcpClientImpl.LogClientInitializationErrorClientOAuthProvider/ModelContextProtocol.Core/Authentication[LoggerMessage]callsites found there anyway.ModelContextProtocol.AspNetCore.StatefulSessionManager.LogSessionDisposeErrorTests
17 tests in
tests/ModelContextProtocol.Tests/Server/ExceptionSummarizationTests.cs, on the repo'sLoggedTest/MockLoggerProviderinfrastructure: the raw and summarized paths, throwing andnull-returning delegates, level gating (a counting summarizer records zero invocations when the level is filtered out, with controls asserting one when enabled), EventId stability across both forms, and the DI wiring.Verification
Full solution build across
net10.0/net9.0/net8.0/netstandard2.0: 0 warnings, 0 errors with warnings-as-errors on. Full test suite: 2950 passed, 0 failed. AOT publish withTrimmerSingleWarn=false: zero IL2xxx/IL3xxx warnings. The repo tracks noPublicAPI.*.txtfiles, and the new property is not[Experimental], soModelContextProtocol.ExperimentalApiRegressionTestis unaffected.