Skip to content

Add opt-in exception summarization for server-side logging - #1804

Open
AkbarDizaji wants to merge 1 commit into
modelcontextprotocol:mainfrom
AkbarDizaji:exception-summarization-logging
Open

Add opt-in exception summarization for server-side logging#1804
AkbarDizaji wants to merge 1 commit into
modelcontextprotocol:mainfrom
AkbarDizaji:exception-summarization-logging

Conversation

@AkbarDizaji

@AkbarDizaji AkbarDizaji commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #1690.

What

Adds an opt-in way to log a sanitized description of an exception instead of the raw Exception on 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.

// Explicit delegate
builder.Services.AddMcpServer(options => options.ExceptionSummarizer = ex => ex.GetType().Name);

// Or via the standard .NET abstraction — picked up automatically
builder.Services.AddExceptionSummarizer(b => b.AddHttpProvider());
builder.Services.AddMcpServer();

Behavior

Opt-in only. If the delegate is null, every callsite invokes the existing [LoggerMessage] method with the raw Exception — the else branch 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 Exception too 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 returns null, the raw exception is logged instead, so a faulty summarizer can never fail the session.

Design decision — and a question for maintainers

ModelContextProtocol.Core depends only on Microsoft.Extensions.Logging.Abstractions plus polyfills, so I kept Microsoft.Extensions.Diagnostics.ExceptionSummarization out of it: Core exposes a plain Func<Exception, string>? on McpServerOptions, and the DI package takes the package reference and populates the delegate from an optionally-resolved IExceptionSummarizer (GetService, never GetRequiredService). That package ships netstandard2.0 assets 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 expose IExceptionSummarizer on the options, I'm happy to switch.

Shared EventId

Unlike the existing *Sensitive pairs, 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 sets EventName = nameof(<RawMethod>) to emit the same EventId and name. That keeps EventId-based filters and alerts working when a summarizer is configured. The *Sensitive variants do carry distinct ids, so if you'd prefer these follow that convention, I'll drop the EventName lines.

Callsites changed

McpSessionHandler: LogRequestHandlerException, LogMessageHandlerException / LogMessageHandlerExceptionSensitive.

McpServerImpl: ToolCallError (2 callsites), GetPromptError, ReadResourceError, MrtrHandlerError.

McpSessionHandler is shared by client and server; the summarizer reaches it through a new optional trailing constructor parameter that only McpServerImpl passes, so client sessions are untouched.

Raw-exception log callsites intentionally NOT changed

The issue scopes this to server-side handler failure paths.

Callsite Why excluded
McpServerImpl.SubscriptionNotificationFailed Notification-delivery plumbing, not a user handler failure; not listed in the issue. Trivial to add if you want it.
TransportBase.* (LogTransportConnectFailed, LogTransportSendFailed, LogTransportMessageParseFailed(Sensitive), LogTransportReadMessagesFailed, LogTransportShutdownFailed, LogTransportCleanupReadTaskFailed, LogTransportEndpointEventParseFailed(Sensitive)) Transport-layer I/O failures, below the MCP session; shared by client and server, and McpServerOptions does not reach them.
StreamableHttpPostTransport.LogStoreStreamDisposalFailed / LogDeferredHeaderFlushFailed Same: transport-level, not handler-level.
StdioClientTransport.*, StdioClientSessionTransport, StreamClientSessionTransport, SseClientSessionTransport, StreamableHttpClientSessionTransport Client-side.
McpClientImpl.LogClientInitializationError Client-side.
ClientOAuthProvider / ModelContextProtocol.Core/Authentication Client-side auth; no raw-exception [LoggerMessage] callsites found there anyway.
ModelContextProtocol.AspNetCore.StatefulSessionManager.LogSessionDisposeError Session-disposal plumbing, outside the handler paths the issue names.

Tests

17 tests in tests/ModelContextProtocol.Tests/Server/ExceptionSummarizationTests.cs, on the repo's LoggedTest / MockLoggerProvider infrastructure: the raw and summarized paths, throwing and null-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 with TrimmerSingleWarn=false: zero IL2xxx/IL3xxx warnings. The repo tracks no PublicAPI.*.txt files, and the new property is not [Experimental], so ModelContextProtocol.ExperimentalApiRegressionTest is unaffected.

@AkbarDizaji
AkbarDizaji force-pushed the exception-summarization-logging branch 2 times, most recently from e07f499 to fb61315 Compare August 9, 2026 07:09
@AkbarDizaji

AkbarDizaji commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

CI: the earlier windows-latest failures are pre-existing flakiness, not this change

Current head (fb61315a) is green on all legs, including both Windows ones: run 31300526347 (windows-latest, Release). I couldn't use "Re-run failed jobs" — that needs write access to this repo — so here is the evidence instead.

The two failures hit different tests, and both were on superseded commits.

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 @ 87f5b09bClientConformanceTests.RunConformanceTest(scenario: "auth/offline-access-scope"), the exact test that failed on a9f02522, plus OAuth.DcrFailureTests.DcrRejection_PropagatesToConsumer_WithStatusBodyAndSentParameters
  • main @ 81ae6ec6OAuth.DcrFailureTests.DcrRejection_PropagatesToConsumer_WithStatusBodyAndSentParameters again

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:

  • McpSessionHandler declares exactly one constructor, so there is no overload set and no overload resolution to perturb.
  • It is never constructed reflectively — no ActivatorUtilities, Activator.CreateInstance, or typeof(McpSessionHandler) anywhere in src/ or tests/.
  • There are exactly two call sites: McpServerImpl.cs:158 (passes the new argument explicitly) and McpClientImpl.cs:59 (relies on the default). McpClientImpl.cs does not appear in this PR's diff at all — its eight positional arguments are byte-identical to main, and the new parameter is appended last, so nothing before it shifts.
  • _exceptionSummarizer is therefore always null on the client path, so both new wrappers take the else branch and emit the original calls unchanged.
  • The diff touches no file under Authentication/, Client/, any transport, or ModelContextProtocol.AspNetCore. The thrown message originates in ClientOAuthProvider.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.

@AkbarDizaji

Copy link
Copy Markdown
Contributor Author

Verification detail

Supporting evidence for two decisions in the description, plus one environmental caveat.

The shared EventId was verified against the generated code

[LoggerMessage] methods in this repo declare no explicit EventId, so the generator derives the numeric id from a hash of the event name, which defaults to the method name. Without EventName, each summarized variant would therefore emit a different id than the raw method it stands in for, and any consumer filtering or alerting on EventId would silently stop matching the moment a summarizer was configured.

I built with /p:EmitCompilerGeneratedFiles=true /p:CompilerGeneratedFilesOutputPath=<dir> and diffed all 102 EventId(...) constructions in the emitted LoggerMessage.g.cs against a pre-change baseline. Taking the request-handler pair as the example:

raw summarized
before EventId(975074943, nameof(LogRequestHandlerException)) EventId(1230764256, nameof(LogRequestHandlerExceptionSummarized))
after EventId(975074943, nameof(LogRequestHandlerException)) EventId(975074943, "LogRequestHandlerException")

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: LogMessageHandlerException (1556437893), LogMessageHandlerExceptionSensitive (1183127983), ToolCallError (1433779783), GetPromptError (955764025), ReadResourceError (1073205151), MrtrHandlerError (732164654).

EventName is declared only on the summarized variant of each pair, which is sufficient to make both emit the same id and name.

Why the DI mapping is ExceptionType + Description

The obvious mapping, ExceptionSummary.Description alone, turns out to be close to useless, and ToString() is unsafe for this purpose. Running the built-in summarizer from AddExceptionSummarizer() on new InvalidOperationException("secret connection string ..."):

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
@AkbarDizaji
AkbarDizaji force-pushed the exception-summarization-logging branch from fb61315 to c7e6515 Compare August 9, 2026 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optional exception summarization for server-side logging

1 participant