From d8502eb45bf75d23608c10c7fab2e93b0c68993a Mon Sep 17 00:00:00 2001 From: Soares Date: Wed, 5 Aug 2026 09:37:03 -0300 Subject: [PATCH] feat: add correlation_id and request path to agw error outputs --- src/sap_cloud_sdk/agentgateway/_customer.py | 50 +++++-- src/sap_cloud_sdk/agentgateway/_lob.py | 67 +++++++-- tests/agentgateway/unit/test_lob.py | 146 ++++++++++++++------ 3 files changed, 198 insertions(+), 65 deletions(-) diff --git a/src/sap_cloud_sdk/agentgateway/_customer.py b/src/sap_cloud_sdk/agentgateway/_customer.py index bf85299c..ff150479 100644 --- a/src/sap_cloud_sdk/agentgateway/_customer.py +++ b/src/sap_cloud_sdk/agentgateway/_customer.py @@ -618,13 +618,14 @@ async def _list_server_tools( url: str, auth_token: str, timeout: float, + correlation_id: str, ) -> list[MCPTool]: """List tools from a single MCP server. Args: url: MCP server endpoint URL. auth_token: Authorization token. - dependency: Integration dependency (for metadata). + correlation_id: Outbound x-correlation-id for this request. Returns: List of MCPTool objects from this server. @@ -632,10 +633,15 @@ async def _list_server_tools( Raises: AgentGatewaySDKError: If server does not provide serverInfo.name. """ + logger.debug( + "Listing tools from server [correlation-id=%s, path=%s]", + correlation_id, + url, + ) async with httpx.AsyncClient( headers={ "Authorization": f"Bearer {auth_token}", - "x-correlation-id": str(uuid.uuid4()), + "x-correlation-id": correlation_id, }, timeout=timeout, ) as http_client: @@ -653,7 +659,8 @@ async def _list_server_tools( and init_result.serverInfo.name ): raise AgentGatewaySDKError( - f"MCP server at '{url}' did not provide serverInfo.name. " + f"MCP server at '{url}' did not provide serverInfo.name " + f"[correlation-id={correlation_id}]. " "This is required by the MCP protocol." ) @@ -672,22 +679,27 @@ async def _list_server_tools( ] -def _log_mcp_server_error(ord_id: str, exc: BaseException) -> None: +def _log_mcp_server_error(ord_id: str, exc: BaseException, correlation_id: str) -> None: # Unwrap ExceptionGroup from anyio to surface the real HTTP error body if isinstance(exc, BaseExceptionGroup): for inner in exc.exceptions: - _log_mcp_server_error(ord_id, inner) + _log_mcp_server_error(ord_id, inner, correlation_id) return + correlation_id_tag = f" [correlation-id={correlation_id}]" if isinstance(exc, httpx.HTTPStatusError): logger.error( - "Failed to load tools from %s (HTTP %d): %s", + "Failed to load tools from %s (HTTP %d)%s: %s", ord_id, exc.response.status_code, + correlation_id_tag, exc.response.text[:500], ) else: logger.exception( - "Failed to load tools from %s — skipping", ord_id, exc_info=exc + "Failed to load tools from %s%s — skipping", + ord_id, + correlation_id_tag, + exc_info=exc, ) @@ -730,12 +742,15 @@ async def get_mcp_tools_customer( dep.global_tenant_id, ) + correlation_id = str(uuid.uuid4()) try: - server_tools = await _list_server_tools(url, system_token, timeout) + server_tools = await _list_server_tools( + url, system_token, timeout, correlation_id + ) tools.extend(server_tools) logger.debug("Loaded %d tool(s) from %s", len(server_tools), dep.ord_id) except Exception as exc: - _log_mcp_server_error(dep.ord_id, exc) + _log_mcp_server_error(dep.ord_id, exc, correlation_id) logger.info( "Loaded %d MCP tool(s) from %d server(s)", len(tools), len(dependencies) @@ -765,10 +780,17 @@ async def call_mcp_tool_customer( """ logger.info("Calling tool '%s' on server '%s'", tool.name, tool.server_name) + correlation_id = str(uuid.uuid4()) + logger.debug( + "Calling tool '%s' [correlation-id=%s, path=%s]", + tool.name, + correlation_id, + tool.url, + ) async with httpx.AsyncClient( headers={ "Authorization": f"Bearer {auth_token}", - "x-correlation-id": str(uuid.uuid4()), + "x-correlation-id": correlation_id, }, timeout=timeout, ) as http_client: @@ -789,6 +811,12 @@ async def call_mcp_tool_customer( text = str(getattr(first, "text", "")) if result.isError: - logger.error("Tool '%s' returned an error: %s", tool.name, text) + logger.error( + "Tool '%s' returned an error [correlation-id=%s, path=%s]: %s", + tool.name, + correlation_id, + tool.url, + text, + ) return text diff --git a/src/sap_cloud_sdk/agentgateway/_lob.py b/src/sap_cloud_sdk/agentgateway/_lob.py index b1d63e55..9d0a41bd 100644 --- a/src/sap_cloud_sdk/agentgateway/_lob.py +++ b/src/sap_cloud_sdk/agentgateway/_lob.py @@ -290,28 +290,37 @@ def _fetch_user_auth_sync(): return token, gateway_url -def _log_mcp_server_error(fragment_name: str, exc: BaseException) -> None: +def _log_mcp_server_error( + fragment_name: str, exc: BaseException, correlation_id: str +) -> None: if isinstance(exc, BaseExceptionGroup): for inner in exc.exceptions: - _log_mcp_server_error(fragment_name, inner) + _log_mcp_server_error(fragment_name, inner, correlation_id) return + correlation_id_tag = f" [correlation-id={correlation_id}]" if isinstance(exc, httpx.HTTPStatusError): logger.error( - "Failed to load tools from fragment '%s' (HTTP %d): %s", + "Failed to load tools from fragment '%s' (HTTP %d)%s: %s", fragment_name, exc.response.status_code, + correlation_id_tag, exc.response.text[:500], ) else: logger.exception( - "Failed to load tools from fragment '%s' — skipping", + "Failed to load tools from fragment '%s'%s — skipping", fragment_name, + correlation_id_tag, exc_info=exc, ) async def list_server_tools( - dest_url: str, auth_token: str, fragment_name: str, timeout: float + dest_url: str, + auth_token: str, + fragment_name: str, + timeout: float, + correlation_id: str, ) -> list[MCPTool]: """List tools from a single MCP server. @@ -319,14 +328,21 @@ async def list_server_tools( dest_url: MCP endpoint URL. auth_token: Raw access token for the request. fragment_name: Fragment name for reference. + correlation_id: Outbound x-correlation-id for this request. Returns: List of MCPTool objects from this server. """ + logger.debug( + "Listing tools from fragment '%s' [correlation-id=%s, path=%s]", + fragment_name, + correlation_id, + dest_url, + ) async with httpx.AsyncClient( headers={ "Authorization": f"Bearer {auth_token}", - "x-correlation-id": str(uuid.uuid4()), + "x-correlation-id": correlation_id, }, timeout=timeout, ) as http_client: @@ -398,9 +414,10 @@ async def get_mcp_tools_lob( ) continue + correlation_id = str(uuid.uuid4()) try: server_tools = await list_server_tools( - mcp_url, system_token, fragment_name, timeout + mcp_url, system_token, fragment_name, timeout, correlation_id ) tools.extend(server_tools) logger.debug( @@ -409,7 +426,7 @@ async def get_mcp_tools_lob( fragment_name, ) except Exception as exc: - _log_mcp_server_error(fragment_name, exc) + _log_mcp_server_error(fragment_name, exc, correlation_id) logger.info("Loaded %d MCP tool(s) from %d fragment(s)", len(tools), len(fragments)) return tools @@ -434,10 +451,17 @@ async def call_mcp_tool_lob( Returns: Tool execution result as string. """ + correlation_id = str(uuid.uuid4()) + logger.debug( + "Calling tool '%s' [correlation-id=%s, path=%s]", + tool.name, + correlation_id, + tool.url, + ) async with httpx.AsyncClient( headers={ "Authorization": f"Bearer {user_auth_token}", - "x-correlation-id": str(uuid.uuid4()), + "x-correlation-id": correlation_id, }, timeout=timeout, ) as http_client: @@ -456,7 +480,13 @@ async def call_mcp_tool_lob( text = str(getattr(first, "text", "")) if result.isError: - logger.error("Tool '%s' returned an error: %s", tool.name, text) + logger.error( + "Tool '%s' returned an error [correlation-id=%s, path=%s]: %s", + tool.name, + correlation_id, + tool.url, + text, + ) return text @@ -482,12 +512,17 @@ async def _fetch_agent_card( AgentGatewaySDKError: If the request fails or returns a non-200 status. """ url = f"{fragment_url.rstrip('/')}/.well-known/agent-card.json" - logger.debug("Fetching agent card from '%s'", url) + correlation_id = str(uuid.uuid4()) + logger.debug( + "Fetching agent card [correlation-id=%s, path=%s]", + correlation_id, + url, + ) async with httpx.AsyncClient( headers={ "Authorization": f"Bearer {auth_token}", - "x-correlation-id": str(uuid.uuid4()), + "x-correlation-id": correlation_id, }, timeout=timeout, ) as client: @@ -495,20 +530,22 @@ async def _fetch_agent_card( response = await client.get(url) except httpx.RequestError as e: raise AgentGatewaySDKError( - f"Agent card request failed for '{fragment_url}': {e}" + f"Agent card request failed for '{fragment_url}' " + f"[correlation-id={correlation_id}]: {e}" ) from e if response.status_code != 200: raise AgentGatewaySDKError( f"Agent card request returned status {response.status_code} " - f"for '{fragment_url}': {response.text[:200]}" + f"for '{fragment_url}' [correlation-id={correlation_id}]: {response.text[:200]}" ) try: payload = response.json() except Exception as e: raise AgentGatewaySDKError( - f"Failed to parse agent card JSON for '{fragment_url}': {e}" + f"Failed to parse agent card JSON for '{fragment_url}' " + f"[correlation-id={correlation_id}]: {e}" ) from e return AgentCard(raw=payload) diff --git a/tests/agentgateway/unit/test_lob.py b/tests/agentgateway/unit/test_lob.py index e2e66723..a3b7054c 100644 --- a/tests/agentgateway/unit/test_lob.py +++ b/tests/agentgateway/unit/test_lob.py @@ -1,7 +1,7 @@ """Unit tests for LoB agent flow.""" import os -from unittest.mock import patch, MagicMock, AsyncMock +from unittest.mock import patch, MagicMock, AsyncMock, ANY import pytest @@ -29,7 +29,10 @@ from sap_cloud_sdk.agentgateway._token_cache import _GatewayUrlCache, _TokenCache from sap_cloud_sdk.agentgateway.config import ClientConfig from sap_cloud_sdk.destination import ConsumptionOptions, ConsumptionLevel -from sap_cloud_sdk.agentgateway.exceptions import AgentGatewaySDKError, MCPServerNotFoundError +from sap_cloud_sdk.agentgateway.exceptions import ( + AgentGatewaySDKError, + MCPServerNotFoundError, +) from sap_cloud_sdk.destination import ConsumptionLevel # Aliases for use in existing test assertions @@ -109,7 +112,9 @@ def test_strips_trailing_slashes_from_url(self): mock_dest.auth_tokens[0].http_header = {"value": header_value} mock_dest.url = "https://agw.example.com/v1/mcp///" - with patch("sap_cloud_sdk.agentgateway._lob.create_destination_client") as mock_client: + with patch( + "sap_cloud_sdk.agentgateway._lob.create_destination_client" + ) as mock_client: mock_client.return_value.get_destination.return_value = mock_dest result = _fetch_auth_token("dest-name", "tenant-sub") @@ -290,7 +295,9 @@ def test_returns_fragment_name(self): fragment = MagicMock() fragment.name = "sap-managed-runtime-agw-subscriber-ias-user-abc123" - with patch("sap_cloud_sdk.agentgateway._fragments.create_fragment_client") as mock_client: + with patch( + "sap_cloud_sdk.agentgateway._fragments.create_fragment_client" + ) as mock_client: mock_client.return_value.list_instance_fragments.return_value = [fragment] result = get_ias_user_fragment_name("tenant-sub") @@ -302,7 +309,9 @@ def test_uses_correct_filter_labels(self): fragment = MagicMock() fragment.name = "ias-user-fragment" - with patch("sap_cloud_sdk.agentgateway._fragments.create_fragment_client") as mock_client: + with patch( + "sap_cloud_sdk.agentgateway._fragments.create_fragment_client" + ) as mock_client: mock_client.return_value.list_instance_fragments.return_value = [fragment] get_ias_user_fragment_name("tenant-sub") @@ -316,10 +325,14 @@ def test_uses_correct_filter_labels(self): def test_raises_when_no_fragment_found(self): """Raise MCPServerNotFoundError when no IAS user fragment exists.""" - with patch("sap_cloud_sdk.agentgateway._fragments.create_fragment_client") as mock_client: + with patch( + "sap_cloud_sdk.agentgateway._fragments.create_fragment_client" + ) as mock_client: mock_client.return_value.list_instance_fragments.return_value = [] - with pytest.raises(MCPServerNotFoundError, match="No IAS user fragment found"): + with pytest.raises( + MCPServerNotFoundError, match="No IAS user fragment found" + ): get_ias_user_fragment_name("tenant-sub") @@ -399,7 +412,9 @@ async def test_reuses_cached_system_auth(self): async def test_raises_when_only_token_cache_provided(self): """Raise ValueError when token_cache given without gateway_url_cache.""" with pytest.raises(ValueError, match="both be provided or both be None"): - await fetch_system_auth("tenant-sub", token_cache=_TokenCache(ClientConfig())) + await fetch_system_auth( + "tenant-sub", token_cache=_TokenCache(ClientConfig()) + ) @pytest.mark.asyncio async def test_raises_when_only_gateway_url_cache_provided(self): @@ -424,10 +439,16 @@ async def test_fetches_user_auth_with_ias_user_fragment(self): with patch.dict(os.environ, {"APPFND_CONHOS_LANDSCAPE": "eu10"}): with ( - patch("sap_cloud_sdk.agentgateway._lob.get_ias_user_fragment_name") as mock_ias_user, - patch("sap_cloud_sdk.agentgateway._lob._fetch_auth_token") as mock_fetch, + patch( + "sap_cloud_sdk.agentgateway._lob.get_ias_user_fragment_name" + ) as mock_ias_user, + patch( + "sap_cloud_sdk.agentgateway._lob._fetch_auth_token" + ) as mock_fetch, ): - mock_ias_user.return_value = "sap-managed-runtime-agw-subscriber-ias-user-abc" + mock_ias_user.return_value = ( + "sap-managed-runtime-agw-subscriber-ias-user-abc" + ) mock_fetch.return_value = (raw_token, gateway_url) result = await fetch_user_auth("user-jwt", "tenant-sub") @@ -440,7 +461,10 @@ async def test_fetches_user_auth_with_ias_user_fragment(self): assert call_args[0][1] == "tenant-sub" options = call_args[0][2] assert options.user_token == "user-jwt" - assert options.fragment_name == "sap-managed-runtime-agw-subscriber-ias-user-abc" + assert ( + options.fragment_name + == "sap-managed-runtime-agw-subscriber-ias-user-abc" + ) assert options.fragment_level == ConsumptionLevel.INSTANCE @pytest.mark.asyncio @@ -481,13 +505,17 @@ async def test_reuses_cached_user_auth(self): async def test_raises_when_only_token_cache_provided(self): """Raise ValueError when token_cache given without gateway_url_cache.""" with pytest.raises(ValueError, match="both be provided or both be None"): - await fetch_user_auth("user-jwt", "tenant-sub", token_cache=_TokenCache(ClientConfig())) + await fetch_user_auth( + "user-jwt", "tenant-sub", token_cache=_TokenCache(ClientConfig()) + ) @pytest.mark.asyncio async def test_raises_when_only_gateway_url_cache_provided(self): """Raise ValueError when gateway_url_cache given without token_cache.""" with pytest.raises(ValueError, match="both be provided or both be None"): - await fetch_user_auth("user-jwt", "tenant-sub", gateway_url_cache=_GatewayUrlCache()) + await fetch_user_auth( + "user-jwt", "tenant-sub", gateway_url_cache=_GatewayUrlCache() + ) # ============================================================ @@ -552,7 +580,11 @@ async def test_uses_pre_fetched_system_token(self): # Verify list_server_tools called with the pre-fetched token mock_tools.assert_called_once_with( - "https://example.com/mcp", "pre-fetched-token", "mcp-server-a", 60.0 + "https://example.com/mcp", + "pre-fetched-token", + "mcp-server-a", + 60.0, + ANY, ) @pytest.mark.asyncio @@ -716,15 +748,16 @@ class TestOrdIdFromUrl: def test_extracts_ord_id_from_standard_url(self): """Return the second-to-last path segment as ord_id.""" - assert _ord_id_from_url( - "https://agw.example.com/v1/a2a/sap.s4:agent:v1/tenant-abc" - ) == "sap.s4:agent:v1" + assert ( + _ord_id_from_url( + "https://agw.example.com/v1/a2a/sap.s4:agent:v1/tenant-abc" + ) + == "sap.s4:agent:v1" + ) def test_strips_trailing_slash(self): """Handle trailing slash on URL.""" - assert _ord_id_from_url( - "https://agw.example.com/v1/a2a/ord-1/gt-1/" - ) == "ord-1" + assert _ord_id_from_url("https://agw.example.com/v1/a2a/ord-1/gt-1/") == "ord-1" def test_returns_empty_for_single_segment(self): """Return empty string when URL has only one path segment.""" @@ -746,7 +779,9 @@ def test_lists_fragments_with_a2a_label(self): with patch( "sap_cloud_sdk.agentgateway._fragments.create_fragment_client" ) as mock_client: - mock_client.return_value.list_instance_fragments.return_value = [mock_fragment] + mock_client.return_value.list_instance_fragments.return_value = [ + mock_fragment + ] result = list_a2a_fragments("tenant-sub") assert result == [mock_fragment] @@ -830,7 +865,9 @@ async def test_raises_on_non_200_status(self): mock_http.return_value.__aenter__.return_value = mock_http_instance with pytest.raises(AgentGatewaySDKError, match="404"): - await _fetch_agent_card("https://agw.example.com/base", "auth-token", 60.0) + await _fetch_agent_card( + "https://agw.example.com/base", "auth-token", 60.0 + ) @pytest.mark.asyncio async def test_raises_on_request_error(self): @@ -845,7 +882,9 @@ async def test_raises_on_request_error(self): mock_http.return_value.__aenter__.return_value = mock_http_instance with pytest.raises(AgentGatewaySDKError, match="Agent card request failed"): - await _fetch_agent_card("https://agw.example.com/base", "auth-token", 60.0) + await _fetch_agent_card( + "https://agw.example.com/base", "auth-token", 60.0 + ) # ============================================================ @@ -881,9 +920,7 @@ async def test_returns_agents_for_all_fragments(self): return_value=AgentCard(raw=card_payload), ), ): - result = await get_agent_cards_lob( - "tenant-sub", "system-token", 60.0 - ) + result = await get_agent_cards_lob("tenant-sub", "system-token", 60.0) assert len(result) == 1 assert isinstance(result[0], Agent) @@ -904,8 +941,12 @@ async def test_returns_empty_list_when_no_fragments(self): @pytest.mark.asyncio async def test_filters_by_agent_names(self): """Fetch all cards then keep only those whose agent card name matches.""" - frag_1 = self._make_fragment("frag-1", "https://agw.example.com/v1/a2a/ord-1/t1") - frag_2 = self._make_fragment("frag-2", "https://agw.example.com/v1/a2a/ord-2/t2") + frag_1 = self._make_fragment( + "frag-1", "https://agw.example.com/v1/a2a/ord-1/t1" + ) + frag_2 = self._make_fragment( + "frag-2", "https://agw.example.com/v1/a2a/ord-2/t2" + ) async def _cards_by_ord(fragment_url, token, timeout): if "ord-1" in fragment_url: @@ -933,8 +974,12 @@ async def _cards_by_ord(fragment_url, token, timeout): @pytest.mark.asyncio async def test_filters_by_ord_ids(self): """Only include fragments whose ordId (from URL) is in the ord_ids filter.""" - frag_1 = self._make_fragment("frag-1", "https://agw.example.com/v1/a2a/ord-1/t1") - frag_2 = self._make_fragment("frag-2", "https://agw.example.com/v1/a2a/ord-2/t2") + frag_1 = self._make_fragment( + "frag-1", "https://agw.example.com/v1/a2a/ord-1/t1" + ) + frag_2 = self._make_fragment( + "frag-2", "https://agw.example.com/v1/a2a/ord-2/t2" + ) with ( patch( @@ -1026,8 +1071,14 @@ def test_returns_client_id_from_destination_properties(self): mock_dest_client.get_destination.return_value = mock_dest with ( - patch("sap_cloud_sdk.agentgateway._lob._ias_dest_name", return_value="sap-managed-runtime-ias-eu10"), - patch("sap_cloud_sdk.agentgateway._lob.create_destination_client", return_value=mock_dest_client), + patch( + "sap_cloud_sdk.agentgateway._lob._ias_dest_name", + return_value="sap-managed-runtime-ias-eu10", + ), + patch( + "sap_cloud_sdk.agentgateway._lob.create_destination_client", + return_value=mock_dest_client, + ), ): result = get_ias_client_id_lob() @@ -1043,10 +1094,18 @@ def test_raises_when_destination_not_found(self): mock_dest_client.get_destination.return_value = None with ( - patch("sap_cloud_sdk.agentgateway._lob._ias_dest_name", return_value="sap-managed-runtime-ias-eu10"), - patch("sap_cloud_sdk.agentgateway._lob.create_destination_client", return_value=mock_dest_client), + patch( + "sap_cloud_sdk.agentgateway._lob._ias_dest_name", + return_value="sap-managed-runtime-ias-eu10", + ), + patch( + "sap_cloud_sdk.agentgateway._lob.create_destination_client", + return_value=mock_dest_client, + ), ): - with pytest.raises(AgentGatewaySDKError, match="sap-managed-runtime-ias-eu10"): + with pytest.raises( + AgentGatewaySDKError, match="sap-managed-runtime-ias-eu10" + ): get_ias_client_id_lob() def test_returns_empty_string_when_property_absent(self): @@ -1056,14 +1115,23 @@ def test_returns_empty_string_when_property_absent(self): mock_dest_client.get_destination.return_value = mock_dest with ( - patch("sap_cloud_sdk.agentgateway._lob._ias_dest_name", return_value="sap-managed-runtime-ias-eu10"), - patch("sap_cloud_sdk.agentgateway._lob.create_destination_client", return_value=mock_dest_client), + patch( + "sap_cloud_sdk.agentgateway._lob._ias_dest_name", + return_value="sap-managed-runtime-ias-eu10", + ), + patch( + "sap_cloud_sdk.agentgateway._lob.create_destination_client", + return_value=mock_dest_client, + ), ): result = get_ias_client_id_lob() assert result == "" def test_raises_when_landscape_env_not_set(self): - with patch("sap_cloud_sdk.agentgateway._lob._ias_dest_name", side_effect=EnvironmentError("APPFND_CONHOS_LANDSCAPE not set")): + with patch( + "sap_cloud_sdk.agentgateway._lob._ias_dest_name", + side_effect=EnvironmentError("APPFND_CONHOS_LANDSCAPE not set"), + ): with pytest.raises(EnvironmentError, match="APPFND_CONHOS_LANDSCAPE"): get_ias_client_id_lob()