From 18e47b17a0ea139bb50a25005692e65a6b883cf5 Mon Sep 17 00:00:00 2001 From: "John M. Kuchta" Date: Tue, 23 Jun 2026 17:06:00 -0700 Subject: [PATCH] feat(session): log Meraki X-Request-Id on 5xx responses Forward-port of #420 to the httpx branch. The session refactor moved 5xx handling into SessionBase._handle_server_error (sync) and the inline async dispatch, so the warning now includes the X-Request-Id and a new _log_server_error_exhausted helper logs it at error level once retries are exhausted. Async unifies on APIError (no AsyncAPIError) and tests target the new meraki/session/ paths and _client.request mocks. When the header is absent, "none" is logged in its place. Co-Authored-By: Claude Opus 4.8 (1M context) --- changelog.d/+log-x-request-id-on-5xx.added.md | 1 + meraki/session/async_.py | 10 +++- meraki/session/base.py | 20 ++++++- tests/unit/conftest.py | 5 ++ tests/unit/test_aio_rest_session.py | 53 ++++++++++++++++++ tests/unit/test_rest_session.py | 55 +++++++++++++++++++ 6 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 changelog.d/+log-x-request-id-on-5xx.added.md diff --git a/changelog.d/+log-x-request-id-on-5xx.added.md b/changelog.d/+log-x-request-id-on-5xx.added.md new file mode 100644 index 0000000..a242f18 --- /dev/null +++ b/changelog.d/+log-x-request-id-on-5xx.added.md @@ -0,0 +1 @@ +On 5xx responses, the SDK now logs the Meraki `X-Request-Id` response header so it can be shared with Meraki to look up the request in server-side logs. If the header is absent, `none` is logged in its place. After retries are exhausted, the request ID is also logged at error level. diff --git a/meraki/session/async_.py b/meraki/session/async_.py index 7d4c1bf..90829fc 100644 --- a/meraki/session/async_.py +++ b/meraki/session/async_.py @@ -256,11 +256,19 @@ async def request(self, metadata: Dict[str, Any], method: str, url: str, **kwarg if retries == 0: raise APIError(metadata, response) elif status >= 500: + request_id = response.headers.get("X-Request-Id") or "none" if self._logger: - self._logger.warning(f"{tag}, {operation} - {status} {reason}, retrying in 1 second") + self._logger.warning( + f"{tag}, {operation} - {status} {reason} (X-Request-Id: {request_id}), retrying in 1 second" + ) await self._sleep(1) retries -= 1 if retries == 0: + if self._logger: + self._logger.error( + f"{tag}, {operation} - {status} {reason} failed after retries. " + f"Provide this X-Request-Id to Meraki for log lookup: {request_id}" + ) raise APIError(metadata, response) elif 400 <= status < 500: retries = await self._handle_client_error_async(response, metadata, retries) diff --git a/meraki/session/base.py b/meraki/session/base.py index 68f6531..bf5757b 100644 --- a/meraki/session/base.py +++ b/meraki/session/base.py @@ -294,6 +294,7 @@ def request(self, metadata: Dict[str, Any], method: str, url: str, **kwargs: Any self._sleep(1) retries -= 1 if retries == 0: + self._log_server_error_exhausted(response, metadata) raise APIError(metadata, response) elif 400 <= status < 500: retries = self._handle_client_error(response, metadata, retries) @@ -371,14 +372,29 @@ def _handle_rate_limit( return wait def _handle_server_error(self, response: "httpx.Response", metadata: Dict[str, Any]) -> None: - """Handle 5xx server errors. Logs warning before retry.""" + """Handle 5xx server errors. Logs warning (with Meraki X-Request-Id) before retry.""" tag = metadata["tags"][0] operation = metadata["operation"] reason = response.reason_phrase if hasattr(response, "reason_phrase") else "" status = response.status_code + request_id = response.headers.get("X-Request-Id") or "none" if self._logger: - self._logger.warning(f"{tag}, {operation} - {status} {reason}, retrying in 1 second") + self._logger.warning(f"{tag}, {operation} - {status} {reason} (X-Request-Id: {request_id}), retrying in 1 second") + + def _log_server_error_exhausted(self, response: "httpx.Response", metadata: Dict[str, Any]) -> None: + """Log at error level once 5xx retries are exhausted, surfacing the X-Request-Id for Meraki log lookup.""" + tag = metadata["tags"][0] + operation = metadata["operation"] + reason = response.reason_phrase if hasattr(response, "reason_phrase") else "" + status = response.status_code + request_id = response.headers.get("X-Request-Id") or "none" + + if self._logger: + self._logger.error( + f"{tag}, {operation} - {status} {reason} failed after retries. " + f"Provide this X-Request-Id to Meraki for log lookup: {request_id}" + ) def _handle_client_error( self, diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index d8c6111..316aafc 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -117,6 +117,11 @@ def session(): return make_sync_session() +@pytest.fixture +def session_with_logger(): + return make_sync_session(logger=MagicMock()) + + @pytest.fixture def async_session(): return make_async_session() diff --git a/tests/unit/test_aio_rest_session.py b/tests/unit/test_aio_rest_session.py index 7c158de..2db7750 100644 --- a/tests/unit/test_aio_rest_session.py +++ b/tests/unit/test_aio_rest_session.py @@ -185,6 +185,59 @@ async def test_5xx_raises_after_max_retries(self, async_session): with pytest.raises(APIError): await async_session.request(_metadata(), "GET", "/organizations") + @pytest.mark.asyncio + async def test_request_id_logged_in_warning(self, async_session_with_logger): + session = async_session_with_logger + resp_500 = _mock_aio_response( + status_code=500, + reason_phrase="Internal Server Error", + headers={"X-Request-Id": "abc123def456"}, + ) + resp_200 = _mock_aio_response(status_code=200) + session._client.request = AsyncMock(side_effect=[resp_500, resp_200]) + + with patch(SLEEP_PATCH, side_effect=_noop_sleep): + result = await session.request(_metadata(), "GET", "/organizations") + + assert result.status_code == 200 + warning_messages = [c.args[0] for c in session._logger.warning.call_args_list] + assert any("X-Request-Id: abc123def456" in m for m in warning_messages) + + @pytest.mark.asyncio + async def test_request_id_logged_as_error_after_exhausting_retries(self, async_session_with_logger): + session = async_session_with_logger + session._maximum_retries = 2 + resp_500 = _mock_aio_response( + status_code=500, + reason_phrase="Internal Server Error", + headers={"X-Request-Id": "deadbeef00112233"}, + ) + session._client.request = AsyncMock(return_value=resp_500) + + with patch(SLEEP_PATCH, side_effect=_noop_sleep): + with pytest.raises(APIError): + await session.request(_metadata(), "GET", "/organizations") + + error_messages = [c.args[0] for c in session._logger.error.call_args_list] + assert any("deadbeef00112233" in m for m in error_messages) + assert any("Provide this X-Request-Id to Meraki" in m for m in error_messages) + + @pytest.mark.asyncio + async def test_no_request_id_logs_none(self, async_session_with_logger): + session = async_session_with_logger + session._maximum_retries = 2 + resp_500 = _mock_aio_response(status_code=500, reason_phrase="Internal Server Error", headers={}) + session._client.request = AsyncMock(return_value=resp_500) + + with patch(SLEEP_PATCH, side_effect=_noop_sleep): + with pytest.raises(APIError): + await session.request(_metadata(), "GET", "/organizations") + + warning_messages = [c.args[0] for c in session._logger.warning.call_args_list] + assert any("X-Request-Id: none" in m for m in warning_messages) + error_messages = [c.args[0] for c in session._logger.error.call_args_list] + assert any("log lookup: none" in m for m in error_messages) + # --- Connection errors --- diff --git a/tests/unit/test_rest_session.py b/tests/unit/test_rest_session.py index 142c2e0..1c593c5 100644 --- a/tests/unit/test_rest_session.py +++ b/tests/unit/test_rest_session.py @@ -134,6 +134,61 @@ def test_connection_error_retries_exactly_maximum_retries(self, mock_sleep, sess assert mock_sleep.call_count == 3 +# --- X-Request-Id logging on 5xx --- + + +class TestRequestIdLoggingOn5xx: + @patch("time.sleep", return_value=None) + def test_request_id_logged_in_warning(self, mock_sleep, session_with_logger): + session = session_with_logger + resp_500 = _mock_response( + 500, + reason_phrase="Internal Server Error", + headers={"X-Request-Id": "abc123def456"}, + ) + resp_200 = _mock_response(200) + session._client.request = MagicMock(side_effect=[resp_500, resp_200]) + + result = session.request(_metadata(), "GET", "/organizations") + + assert result.status_code == 200 + warning_messages = [c.args[0] for c in session._logger.warning.call_args_list] + assert any("X-Request-Id: abc123def456" in m for m in warning_messages) + + @patch("time.sleep", return_value=None) + def test_request_id_logged_as_error_after_exhausting_retries(self, mock_sleep, session_with_logger): + session = session_with_logger + session._maximum_retries = 2 + resp_500 = _mock_response( + 500, + reason_phrase="Internal Server Error", + headers={"X-Request-Id": "deadbeef00112233"}, + ) + session._client.request = MagicMock(return_value=resp_500) + + with pytest.raises(APIError): + session.request(_metadata(), "GET", "/organizations") + + error_messages = [c.args[0] for c in session._logger.error.call_args_list] + assert any("deadbeef00112233" in m for m in error_messages) + assert any("Provide this X-Request-Id to Meraki" in m for m in error_messages) + + @patch("time.sleep", return_value=None) + def test_no_request_id_logs_none(self, mock_sleep, session_with_logger): + session = session_with_logger + session._maximum_retries = 2 + resp_500 = _mock_response(500, reason_phrase="Internal Server Error", headers={}) + session._client.request = MagicMock(return_value=resp_500) + + with pytest.raises(APIError): + session.request(_metadata(), "GET", "/organizations") + + warning_messages = [c.args[0] for c in session._logger.warning.call_args_list] + assert any("X-Request-Id: none" in m for m in warning_messages) + error_messages = [c.args[0] for c in session._logger.error.call_args_list] + assert any("log lookup: none" in m for m in error_messages) + + # --- Pagination tests ---