Skip to content

Commit 98105c3

Browse files
Copilotsouvikghosh04
Copilot
and
souvikghosh04
committed
Address MCP telemetry review comments
- Add AUTHORIZATION_FAILED error code separate from AUTHENTICATION_FAILED - Use tool type to infer operations: built-in tools mapped by name, custom tools always "execute" - Map describe_entities to "describe" operation (not "read") - Extract error.code and error.message from CallToolResult when IsError=true - Catch all exceptions in ExtractCustomToolMetadata for best-effort metadata - Update MapExceptionToErrorCode to handle DataApiBuilderException auth/authz cases - Make _recordedActivities readonly in tests - Use McpTelemetryErrorCodes constants in tests - Rename FakeMcpTool to MockMcpTool - Test all DML tools (read, create, update, delete, describe, execute) - Focus tests on ExecuteWithTelemetryAsync rather than individual TrackXX methods - Custom tools (stored procedures) now correctly return "execute" operation Co-authored-by: souvikghosh04 <[email protected]>
1 parent f5e9d71 commit 98105c3

3 files changed

Lines changed: 198 additions & 86 deletions

File tree

‎src/Azure.DataApiBuilder.Mcp/Utils/McpTelemetryErrorCodes.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,15 @@ internal static class McpTelemetryErrorCodes
1414
public const string EXECUTION_FAILED = "ExecutionFailed";
1515

1616
/// <summary>
17-
/// Authentication or authorization failure error code.
17+
/// Authentication failure error code.
1818
/// </summary>
1919
public const string AUTHENTICATION_FAILED = "AuthenticationFailed";
2020

21+
/// <summary>
22+
/// Authorization failure error code.
23+
/// </summary>
24+
public const string AUTHORIZATION_FAILED = "AuthorizationFailed";
25+
2126
/// <summary>
2227
/// Database operation failure error code.
2328
/// </summary>

‎src/Azure.DataApiBuilder.Mcp/Utils/McpTelemetryHelper.cs‎

Lines changed: 113 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@
88
using Azure.DataApiBuilder.Core.Telemetry;
99
using Azure.DataApiBuilder.Mcp.Core;
1010
using Azure.DataApiBuilder.Mcp.Model;
11+
using Azure.DataApiBuilder.Service.Exceptions;
1112
using Microsoft.Extensions.DependencyInjection;
1213
using ModelContextProtocol.Protocol;
14+
using static Azure.DataApiBuilder.Mcp.Model.McpEnums;
1315

1416
namespace Azure.DataApiBuilder.Mcp.Utils
1517
{
@@ -42,7 +44,7 @@ public static async Task<CallToolResult> ExecuteWithTelemetryAsync(
4244
{
4345
// Extract telemetry metadata
4446
string? entityName = ExtractEntityNameFromArguments(arguments);
45-
string? operation = InferOperationFromToolName(toolName);
47+
string? operation = InferOperationFromTool(tool, toolName);
4648
string? dbProcedure = null;
4749

4850
// For custom tools (DynamicCustomTool), extract stored procedure information
@@ -65,8 +67,21 @@ public static async Task<CallToolResult> ExecuteWithTelemetryAsync(
6567
// and return CallToolResult with IsError=true instead of throwing)
6668
if (result.IsError == true)
6769
{
68-
activity?.SetStatus(ActivityStatusCode.Error, "Tool returned an error result");
70+
// Extract error code and message from the result content
71+
(string? errorCode, string? errorMessage) = ExtractErrorFromCallToolResult(result);
72+
73+
activity?.SetStatus(ActivityStatusCode.Error, errorMessage ?? "Tool returned an error result");
6974
activity?.SetTag("mcp.tool.error", true);
75+
76+
if (!string.IsNullOrEmpty(errorCode))
77+
{
78+
activity?.SetTag("error.code", errorCode);
79+
}
80+
81+
if (!string.IsNullOrEmpty(errorMessage))
82+
{
83+
activity?.SetTag("error.message", errorMessage);
84+
}
7085
}
7186
else
7287
{
@@ -86,29 +101,84 @@ public static async Task<CallToolResult> ExecuteWithTelemetryAsync(
86101
}
87102

88103
/// <summary>
89-
/// Infers the operation type from the tool name using keyword matching.
90-
/// Matching follows a fixed precedence order: read > create > update > delete > execute.
91-
/// The first matching keyword wins. For example, a tool named "get_deleted_items" will be
92-
/// inferred as "read" (matches "get" before "delete"). Built-in tool names (read_records,
93-
/// create_record, update_record, delete_record, describe_entities) are unambiguous.
94-
/// Custom tool names derived from stored procedures may match heuristically.
95-
/// If no keyword matches, defaults to "execute".
104+
/// Infers the operation type from the tool instance and name.
105+
/// For built-in tools, maps tool name directly to operation.
106+
/// For custom tools (stored procedures), always returns "execute".
96107
/// </summary>
108+
/// <param name="tool">The tool instance.</param>
97109
/// <param name="toolName">The name of the tool.</param>
98110
/// <returns>The inferred operation type.</returns>
99-
public static string InferOperationFromToolName(string toolName)
111+
public static string InferOperationFromTool(IMcpTool tool, string toolName)
100112
{
113+
// Custom tools (stored procedures) are always "execute"
114+
if (tool.ToolType == ToolType.Custom)
115+
{
116+
return "execute";
117+
}
118+
119+
// Built-in tools: map tool name to operation
101120
return toolName.ToLowerInvariant() switch
102121
{
103-
string s when s.Contains("read") || s.Contains("get") || s.Contains("list") || s.Contains("describe") => "read",
104-
string s when s.Contains("create") || s.Contains("insert") => "create",
105-
string s when s.Contains("update") || s.Contains("modify") => "update",
106-
string s when s.Contains("delete") || s.Contains("remove") => "delete",
107-
string s when s.Contains("execute") => "execute",
108-
_ => "execute"
122+
"read_records" => "read",
123+
"create_record" => "create",
124+
"update_record" => "update",
125+
"delete_record" => "delete",
126+
"describe_entities" => "describe",
127+
"execute_entity" => "execute",
128+
_ => "execute" // Fallback for any unknown built-in tools
109129
};
110130
}
111131

132+
/// <summary>
133+
/// Extracts error code and message from a CallToolResult's content.
134+
/// MCP tools may return errors as JSON with "code" and "message" properties.
135+
/// </summary>
136+
/// <param name="result">The tool result to extract error info from.</param>
137+
/// <returns>A tuple of (errorCode, errorMessage).</returns>
138+
private static (string? errorCode, string? errorMessage) ExtractErrorFromCallToolResult(CallToolResult result)
139+
{
140+
string? errorCode = null;
141+
string? errorMessage = null;
142+
143+
if (result.Content != null)
144+
{
145+
foreach (ContentBlock block in result.Content)
146+
{
147+
// Check if this is a text block with JSON error information
148+
if (block is TextContentBlock textBlock && !string.IsNullOrEmpty(textBlock.Text))
149+
{
150+
try
151+
{
152+
using JsonDocument doc = JsonDocument.Parse(textBlock.Text);
153+
JsonElement root = doc.RootElement;
154+
155+
if (root.TryGetProperty("code", out JsonElement codeEl))
156+
{
157+
errorCode = codeEl.GetString();
158+
}
159+
160+
if (root.TryGetProperty("message", out JsonElement msgEl))
161+
{
162+
errorMessage = msgEl.GetString();
163+
}
164+
165+
// If we found error info, we can break
166+
if (errorCode != null || errorMessage != null)
167+
{
168+
break;
169+
}
170+
}
171+
catch
172+
{
173+
// Not JSON or doesn't have expected structure, skip
174+
}
175+
}
176+
}
177+
}
178+
179+
return (errorCode, errorMessage);
180+
}
181+
112182
/// <summary>
113183
/// Maps an exception to a telemetry error code.
114184
/// </summary>
@@ -119,7 +189,11 @@ public static string MapExceptionToErrorCode(Exception ex)
119189
return ex switch
120190
{
121191
OperationCanceledException => McpTelemetryErrorCodes.OPERATION_CANCELLED,
122-
UnauthorizedAccessException => McpTelemetryErrorCodes.AUTHENTICATION_FAILED,
192+
DataApiBuilderException dabEx when dabEx.SubStatusCode == DataApiBuilderException.SubStatusCodes.AuthenticationChallenge
193+
=> McpTelemetryErrorCodes.AUTHENTICATION_FAILED,
194+
DataApiBuilderException dabEx when dabEx.SubStatusCode == DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed
195+
=> McpTelemetryErrorCodes.AUTHORIZATION_FAILED,
196+
UnauthorizedAccessException => McpTelemetryErrorCodes.AUTHORIZATION_FAILED,
123197
System.Data.Common.DbException => McpTelemetryErrorCodes.DATABASE_ERROR,
124198
ArgumentException => McpTelemetryErrorCodes.INVALID_REQUEST,
125199
_ => McpTelemetryErrorCodes.EXECUTION_FAILED
@@ -145,40 +219,43 @@ public static string MapExceptionToErrorCode(Exception ex)
145219

146220
/// <summary>
147221
/// Extracts metadata from a custom tool for telemetry purposes.
222+
/// Returns best-effort metadata; failures in configuration access must not prevent tool execution.
148223
/// </summary>
149224
/// <param name="customTool">The custom tool instance.</param>
150225
/// <param name="serviceProvider">The service provider.</param>
151226
/// <returns>A tuple containing the entity name and database procedure name.</returns>
152227
public static (string? entityName, string? dbProcedure) ExtractCustomToolMetadata(DynamicCustomTool customTool, IServiceProvider serviceProvider)
153228
{
154-
try
229+
// Access public properties instead of reflection
230+
string? entityName = customTool.EntityName;
231+
232+
if (entityName == null)
155233
{
156-
// Access public properties instead of reflection
157-
string? entityName = customTool.EntityName;
234+
return (null, null);
235+
}
158236

159-
if (entityName != null)
237+
try
238+
{
239+
// Try to get the stored procedure name from the runtime configuration
240+
RuntimeConfigProvider? runtimeConfigProvider = serviceProvider.GetService<RuntimeConfigProvider>();
241+
if (runtimeConfigProvider != null)
160242
{
161-
// Try to get the stored procedure name from the runtime configuration
162-
RuntimeConfigProvider? runtimeConfigProvider = serviceProvider.GetService<RuntimeConfigProvider>();
163-
if (runtimeConfigProvider != null)
243+
RuntimeConfig config = runtimeConfigProvider.GetConfig();
244+
if (config.Entities.TryGetValue(entityName, out Entity? entityConfig))
164245
{
165-
RuntimeConfig config = runtimeConfigProvider.GetConfig();
166-
if (config.Entities.TryGetValue(entityName, out Entity? entityConfig))
167-
{
168-
string? dbProcedure = entityConfig.Source.Object;
169-
return (entityName, dbProcedure);
170-
}
246+
string? dbProcedure = entityConfig.Source.Object;
247+
return (entityName, dbProcedure);
171248
}
172249
}
173-
174-
return (entityName, null);
175250
}
176-
catch (Exception ex) when (ex is InvalidOperationException || ex is ArgumentException)
251+
catch (Exception)
177252
{
178-
// If configuration access fails due to invalid state or arguments, return null values
179-
// This is expected during startup or configuration changes
180-
return (null, null);
253+
// If configuration access fails for any reason (including DataApiBuilderException
254+
// when runtime config isn't set up), fall back to returning only the entity name.
255+
// Telemetry metadata extraction is best-effort and must not prevent tool execution.
181256
}
257+
258+
return (entityName, null);
182259
}
183260
}
184261
}

0 commit comments

Comments
 (0)