Throw fewer exceptions from AppLens #1472
Conversation
In AppLens calls there are a number of operations that may "fail" in non-exceptional ways: we may not be able to find the specified resource, or we find multiple resources of the same name, or the resource type isn't supported, etc. Currently we have several operation-specific type hierarchies representing this, with concrete types for the success and failure cases sharing an abstract base type. Since the pattern is always the same this change replaces them with a general purpose `Result<T>` base type and `Success<T>` and `Failure<T>` concrete types. `T` represents the type of the data returned in a successful operation, while `Failure<T>` exposes a `Message` field containing more information about the failure.
The term "result" is somewhat overloaded in the AppLens code. To better distinguish actual data produced by an operation from the success or failure of the operation itself, this change renames `DiagnosticResult` to `AppLensInsights`. This is a better reflection of what the type holds, anyway.
There are a few places where we're turning a `Failure<>` into an exception. This is generally not what we want to do. For one thing, if the issue were truly exceptional we should have already thrown the exception rather than returning `Failure<>`. Also, a local "failure" doesn't always mean we should treat the overall tool call as failed; we may just need to provide more guidance to the LM so it can try again. Here we replace a couple of `throw`s with `Failure<>`s, and update `IAppLensService/AppLensService` to return `Result<AppLensInsight>`. The calling code in `ResourceDiagnoseCommand` is, in turn, updated to return the strings from any `Failure<>` results.
tmeschter
force-pushed
the
dev/tomescht/260108-RemoveAppLensExceptions
branch
from
8db8b53 to
075cc0c
Compare
tmeschter
requested review from
alzimmermsft,
chidozieononiwu,
Copilot,
fanyang-mono,
hallipr,
jongio and
sandeep-sen
There was a problem hiding this comment.
Pull request overview
This pull request refactors the AppLens tool to reduce exception throwing by introducing a Result pattern for error handling. Instead of throwing exceptions for common failures (like resource not found), the code now returns structured failure results that can be communicated to the language model as informative messages.
Changes:
- Introduced a generic Result pattern with Success and Failure types to replace custom result types
- Updated AppLensService to return Result instead of throwing exceptions for common failure scenarios
- Modified ResourceDiagnoseCommand to handle both success and failure results appropriately
- Updated tests to work with the new Success wrapper type
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.AppLens/src/Models/AppLensModels.cs | Introduces Result, Success, and Failure records; removes old custom result types; renames DiagnosticResult to AppLensInsights; adds Message field to ResourceDiagnoseCommandResult |
| tools/Azure.Mcp.Tools.AppLens/src/Services/IAppLensService.cs | Updates interface signature to return Result instead of DiagnosticResult |
| tools/Azure.Mcp.Tools.AppLens/src/Services/AppLensService.cs | Implements new Result pattern throughout; converts exception throws to failure returns; updates method signatures and return types |
| tools/Azure.Mcp.Tools.AppLens/src/Commands/Resource/ResourceDiagnoseCommand.cs | Adds switch expression to handle Success and Failure cases; maps results to appropriate command responses |
| tools/Azure.Mcp.Tools.AppLens/src/Models/AppLensJsonContext.cs | Updates JSON serialization context to reference AppLensInsights instead of DiagnosticResult |
| tools/Azure.Mcp.Tools.AppLens/tests/Azure.Mcp.Tools.AppLens.UnitTests/Resource/ResourceDiagnoseCommandTests.cs | Updates test mocks and assertions to use Success wrapper |
| servers/Azure.Mcp.Server/CHANGELOG.md | Documents the bug fix for AppLens exceptions |
| { | ||
| Success<AppLensInsights> success => ResponseResult.Create(new(success.Data, Message: null), AppLensJsonContext.Default.ResourceDiagnoseCommandResult), | ||
| Failure<AppLensInsights> failure => ResponseResult.Create(new ResourceDiagnoseCommandResult(null, failure.Message), AppLensJsonContext.Default.ResourceDiagnoseCommandResult), | ||
| _ => throw new InvalidOperationException($"Unexpected result type from ${nameof(service.DiagnoseResourceAsync)}.") |
There was a problem hiding this comment.
The string interpolation syntax is incorrect. It uses $ followed by ${nameof(...)}, which will not perform the interpolation correctly. The correct syntax should use {nameof(...)} inside a string prefixed with $. This will result in an error message that literally contains "${nameof(service.DiagnoseResourceAsync)}" instead of the actual method name "DiagnoseResourceAsync".
| _ => throw new InvalidOperationException($"Unexpected result type from ${nameof(service.DiagnoseResourceAsync)}.") | |
| _ => throw new InvalidOperationException($"Unexpected result type from {nameof(service.DiagnoseResourceAsync)}.") |
| context.Response.Results = result switch | ||
| { | ||
| Success<AppLensInsights> success => ResponseResult.Create(new(success.Data, Message: null), AppLensJsonContext.Default.ResourceDiagnoseCommandResult), | ||
| Failure<AppLensInsights> failure => ResponseResult.Create(new ResourceDiagnoseCommandResult(null, failure.Message), AppLensJsonContext.Default.ResourceDiagnoseCommandResult), | ||
| _ => throw new InvalidOperationException($"Unexpected result type from ${nameof(service.DiagnoseResourceAsync)}.") | ||
| }; |
There was a problem hiding this comment.
The new failure path introduced in the Result pattern (when DiagnoseResourceAsync returns a Failure result) lacks test coverage. While the tests check for exceptions being thrown, there are no tests that verify the behavior when the service returns a Failure result containing an error message, such as when a resource is not found or when the AppLens session creation fails. Consider adding tests that mock the service to return Failure results and verify that the command properly handles these cases and returns appropriate responses to the user.
There was a problem hiding this comment.
Does this change create larger implications on what we determine to be a successful or failed MCP tool call? For example, does the failure to find a resource change to a successful tool call but the operation itself failed due to the resource not existing.
2 participants
What does this PR do?
The AppLens tool has a very low success rate. Looking through the code there are several common, non-exceptional cases where we're throwing an exception that escapes the tool. The MCP server infrastructure captures these exceptions and treats them as failures.
For example, we will currently throw an exception if the resource can't be found. This is pretty normal--the user may have mistyped the name, or the resource exists but not in the specified subscription. Rather than throwing exceptions we should simply return a message telling the LM that the resource wasn't found and to check the spelling.
Most of the changes here are concerned with bubbling existing messages up to the LM in this manner. This is not a complete solution to the problem but does address some low-hanging fruit. Beyond this we'll need to invest in better telemetry for AppLens so that we can see where things tend to go sideways.
GitHub issue number?
https://github.com/microsoft/mcp-pr/issues/172
Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline