Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check if response has valid json before logging it. #1087

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gkmngrgn
Copy link

@gkmngrgn gkmngrgn commented Jan 27, 2025

That's a suggestion for -> langfuse/langfuse#4464

Honestly, I'm not sure about if we can consider it as a bug. Because the response comes from outside and there's no guarantee that res.text is always json serializable.

There were many options to make the outputs more human-readable but I preferred this way:

  • Added a sub function to serialize and log the response at the same time, in each if statement.
  • If the status code is 200 or 201, we may not need to the json output, so it logs response text manually, for the specific case.

Important

Improves JSON parsing and error handling in _process_response by adding parse_json_response function in langfuse/request.py.

  • Behavior:
    • Adds parse_json_response function in _process_response to handle JSON parsing and logging.
    • Raises APIError for invalid JSON responses in _process_response.
    • Logs response text for status codes 200 and 201 if return_json is False.
  • Error Handling:
    • Improves error handling for non-JSON responses by raising APIError with status code and message.
  • Logging:
    • Logs response text in _process_response for successful status codes and when JSON parsing is attempted.

This description was created by Ellipsis for e2e0e88. It will automatically update as commits are pushed.

@CLAassistant
Copy link

CLAassistant commented Jan 27, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: Experimental PR review

PR Summary

This PR adds improved JSON response handling and validation in the Langfuse Python client.

  • Added parse_json_response helper function in langfuse/request.py to safely validate and parse JSON responses before logging
  • Improved error handling by catching invalid JSON responses that could previously cause unhandled exceptions
  • Optimized logging by only attempting JSON parsing when needed (status codes 200/201)
  • Consolidated duplicate JSON parsing logic into a single reusable function for better maintainability

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

raise APIError(response.status_code, "Invalid JSON response received")

log.debug("received response: %s", response.text)
return json_response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkmngrgn thanks for raising this PR! If the parsing fails, we would fall back to None. I would prefer to return response.text to have the error reason propagated to the ApiError construction further below

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hassiebp hey, I did a commit, can you confirm if it's what you meant? 36cf937

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hassiebp hi again, any update that I can help you in this PR?

Copy link
Contributor

@hassiebp hassiebp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for raising this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants