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

Added support for logging in JSON format as well. #1985

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

Conversation

Vidit-Ostwal
Copy link
Contributor

@Vidit-Ostwal Vidit-Ostwal commented Jan 27, 2025

Added support to save logs as a JSON file.

Now, when initializing the crew, you can simply set output_log_file to True or provide a file name (e.g., "log.json") and enable save_as_json by setting it to True. Here's an example:

crew = Crew(
    agents=[List of agents],
    tasks=[List of tasks],
    verbose=True,
    memory=False,
    max_iterations=1,
    output_log_file="log.json",
    save_as_json=True
)

The generated .json file will contain an array of JSON events, making it easy to parse and work with.

Issue #1984,#1970, #1793

@joaomdmoura
Copy link
Collaborator

Disclaimer: This review was made by a crew of AI Agents.

Code Review Comment for PR #1985: JSON Format Logging Implementation

Overview

This pull request introduces the capability for logging in JSON format alongside the existing text-based logging system, primarily modifying the crew.py and file_handler.py files. The enhancements made are beneficial for structured logging; however, there are several areas for improvement that should be addressed to ensure robustness and maintainability.

Changes in crew.py

Positive Aspects

  • The addition of the save_as_json parameter is a well-considered enhancement that adds flexibility to the logging format.
  • The integration into the existing code structure is clean and minimizes disruption to current functionality.

Suggested Improvements

  1. Parameter Spacing Issue:

    • Current Code:
      self._file_handler = FileHandler(self.output_log_file,self.save_as_json)
    • Improvement:
      self._file_handler = FileHandler(self.output_log_file, self.save_as_json)
  2. Missing Type Hints:

    • Suggest adding type hints for better code clarity and to enable static type checking:
      save_as_json: Optional[bool] = Field(
          default=False,
          description="If true saves the logs in JSON format",
      ) -> None

Changes in file_handler.py

Positive Aspects

  • The implementation of JSON logging is straightforward and effectively handles file extensions while including timestamps in logs.

Suggested Improvements

  1. Improved Error Handling:

    • It's essential to enhance error handling within the log method to ensure failures are managed gracefully.
      try:
          # JSON log handling
      except Exception as e:
          raise ValueError(f"Failed to log message: {str(e)}")
  2. Type Hints and Documentation:

    • Ensure all methods and classes include type hints and proper docstrings for better maintainability. For example:
      class FileHandler:
          """Handler for file operations supporting both JSON and text-based logging.
          
          Args:
              file_path (Union[bool, str]): Path to the log file or boolean flag
              save_as_json (bool): If True, saves logs in JSON format
          """
  3. Refined File Path Handling:

    • Isolate path logic into a dedicated method to enhance readability.
      def _initialize_path(self, file_path: Union[bool, str]) -> str:
          # Your refactored code here

General Recommendations

  • Add Unit Tests: Comprehensive testing should be established to cover both JSON and text logging scenarios to ensure reliability and correctness.

  • Documentation: Including example usage in the documentation can guide future developers:

    """
    Example usage:
        handler = FileHandler("logs/output", save_as_json=True)
        handler.log(message="Task completed", status="success")
    """
  • Performance Consideration: Consider implementing a buffering mechanism to improve performance for large logging operations.

  • Log Rotation: To handle file sizes better, implementing a log rotation strategy could prevent the handler from crashing with excessively large files.

Conclusion

The modifications introduced in this pull request enhance the logging framework significantly by introducing JSON support. However, implementing the suggested improvements would sharpen the reliability, maintainability, and overall quality of the code. Enhancing error handling, applying type hints, and broadening the documentation will create a solid foundation for future development and changes in this logging capability.

Related Pull Requests

While specific related PRs couldn't be analyzed due to access constraints, it is recommended to review any recent PRs that have modified logging mechanisms or related functionalities for additional insights that may influence the current review.

Let's keep up the great work and continue improving our codebase!

@Vidit-Ostwal
Copy link
Contributor Author

Vidit-Ostwal commented Jan 28, 2025

@joaomdmoura, @lorenzejay Please review this.

@Vidit-Ostwal
Copy link
Contributor Author

@bhancockio , could you review this code and share your thoughts? Also, do you think this functionality improves the user experience?

@Vidit-Ostwal Vidit-Ostwal changed the title Added functionality to have json format as well for the logs Added support for logging in JSON format as well. Feb 4, 2025
Copy link
Collaborator

@bhancockio bhancockio left a comment

Choose a reason for hiding this comment

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

Hey @Vidit-Ostwal! Thank you so much for working on this issue.

Instead of adding the save_as_json property to the crew, could you please just use the file_path.endswith(".json") check to see if we should save things in a JSON format or not.

We want to keep things as magical and easy to use as possible in crew.

Once you make this change, could you please shoot me a message at [email protected] so I know to come back and review this.

In that message, could you please shoot over some screenshots or a quick loom recording so I can easily verify if this is working.

Looking forward to getting this merged in soon. Thank you again for your help 😁

@Vidit-Ostwal
Copy link
Contributor Author

Hi @joaomdmoura, attaching the final files here, for ease confirmation.

logs.json
logs.txt

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.

4 participants