chore(kagent-adk/session_service): add logging for Kagent API errors #1029
Conversation
Signed-off-by: Huang Huang <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR adds consistent error logging for HTTP API failures in the Kagent session service to improve debugging and monitoring capabilities. The changes wrap existing HTTP status error handling with try-catch blocks that log the response text before re-raising the exceptions.
- Adds error logging for all HTTP API failures in session operations
- Maintains existing error handling behavior while adding visibility into failure details
- Uses consistent logging format across all session service methods
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: | ||
| logger.error(f"Failed to create session: {response.text}") |
There was a problem hiding this comment.
Logging the full response text could potentially expose sensitive information. Consider logging only the status code and reason phrase, or sanitize the response content before logging.
| logger.error(f"Failed to create session: {response.text}") | |
| logger.error( | |
| f"Failed to create session: status_code={response.status_code}, reason={response.reason_phrase}" | |
| ) |
| except httpx.HTTPStatusError as e: | ||
| if e.response.status_code == 404: | ||
| return None | ||
| logger.error(f"Failed to get session: {response.text}") |
There was a problem hiding this comment.
Logging the full response text could potentially expose sensitive information. Consider logging only the status code and reason phrase, or sanitize the response content before logging.
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: | ||
| logger.error(f"Failed to list sessions: {response.text}") |
There was a problem hiding this comment.
Logging the full response text could potentially expose sensitive information. Consider logging only the status code and reason phrase, or sanitize the response content before logging.
| logger.error(f"Failed to list sessions: {response.text}") | |
| logger.error(f"Failed to list sessions: status_code={response.status_code}, reason={getattr(response, 'reason_phrase', '')}") |
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: | ||
| logger.error(f"Failed to delete session: {response.text}") |
There was a problem hiding this comment.
Logging the full response text could potentially expose sensitive information. Consider logging only the status code and reason phrase, or sanitize the response content before logging.
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: | ||
| logger.error(f"Failed to append event: {response.text}") |
There was a problem hiding this comment.
Logging the full response text could potentially expose sensitive information. Consider logging only the status code and reason phrase, or sanitize the response content before logging.
| response.raise_for_status() | ||
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: |
There was a problem hiding this comment.
The error handling pattern is duplicated across multiple methods. Consider extracting this into a helper method or decorator to reduce code duplication and improve maintainability.
| response.raise_for_status() | ||
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: |
There was a problem hiding this comment.
The error handling pattern is duplicated across multiple methods. Consider extracting this into a helper method or decorator to reduce code duplication and improve maintainability.
| response.raise_for_status() | ||
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: |
There was a problem hiding this comment.
The error handling pattern is duplicated across multiple methods. Consider extracting this into a helper method or decorator to reduce code duplication and improve maintainability.
| response.raise_for_status() | ||
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: |
There was a problem hiding this comment.
The error handling pattern is duplicated across multiple methods. Consider extracting this into a helper method or decorator to reduce code duplication and improve maintainability.
|
thanks for the PR @mozillazg . can you address the copilot comments? |
mozillazg
changed the title
2 participants
No description provided.