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

Fix some server implementations not properly logging context #933

Merged
merged 2 commits into from
Jun 2, 2024

Conversation

MarkRx
Copy link
Contributor

@MarkRx MarkRx commented May 31, 2024

User description

The contextualize method needs to be called on the same thread.

Also added some missing setup_logger invocations.


PR Type

Bug fix, Enhancement


Description

  • Added setup_logger invocation with JSON format and DEBUG level in Azure DevOps and Bitbucket server webhooks.
  • Refactored handle_request method in Azure DevOps, Bitbucket, and GitLab server webhooks to use an inner async function for logging context.

Changes walkthrough 📝

Relevant files
Bug fix
azuredevops_server_webhook.py
Fix logging context and add logger setup in Azure DevOps server
webhook

pr_agent/servers/azuredevops_server_webhook.py

  • Added setup_logger invocation with JSON format and DEBUG level.
  • Refactored handle_request to use an inner async function for logging
    context.
  • +8/-3     
    bitbucket_server_webhook.py
    Fix logging context and add logger setup in Bitbucket server webhook

    pr_agent/servers/bitbucket_server_webhook.py

  • Added setup_logger invocation with JSON format and DEBUG level.
  • Refactored handle_request to use an inner async function for logging
    context.
  • +8/-3     
    gitlab_webhook.py
    Fix logging context in GitLab server webhook                         

    pr_agent/servers/gitlab_webhook.py

  • Refactored handle_request to use an inner async function for logging
    context.
  • +6/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are localized to specific methods in multiple files, involving logging setup and refactoring to use async functions. The logic is straightforward and the modifications are not extensive.

    🏅 Score

    85

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The use of await inside the inner function assumes that PRAgent().handle_request is an asynchronous function. If it's not, this will raise a runtime error.

    Performance Concern: The addition of setup_logger at the module level in each webhook file might lead to redundant logger setups if these modules are imported multiple times in different parts of the application.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add exception handling to the inner function to ensure errors are logged

    The inner function should handle exceptions to ensure that any errors during the request
    handling are properly logged and do not cause the background task to fail silently.

    pr_agent/servers/azuredevops_server_webhook.py [45-47]

     async def inner():
    -    with get_logger().contextualize(**log_context):
    -        await PRAgent().handle_request(url, body)
    +    try:
    +        with get_logger().contextualize(**log_context):
    +            await PRAgent().handle_request(url, body)
    +    except Exception as e:
    +        get_logger().error(f"Error handling request: {e}")
     
    Suggestion importance[1-10]: 8

    Why: Adding exception handling to the inner function is crucial for robust error management and to prevent the background task from failing silently. This is a significant improvement in terms of reliability and maintainability.

    8
    Best practice
    Ensure the logger is configured only once to prevent reconfiguration issues

    The setup_logger function should be called only once during the application startup to
    avoid reconfiguring the logger multiple times, which can lead to unexpected behavior.

    pr_agent/servers/bitbucket_server_webhook.py [19]

    -setup_logger(fmt=LoggingFormat.JSON, level="DEBUG")
    +if not get_logger().hasHandlers():
    +    setup_logger(fmt=LoggingFormat.JSON, level="DEBUG")
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to check if the logger already has handlers before setting it up again is valid and prevents potential issues with multiple configurations. This is a good practice to ensure consistent logging behavior.

    7

    @mrT23
    Copy link
    Collaborator

    mrT23 commented May 31, 2024

    @okotek please review this (especially for 'pr_agent/servers/gitlab_webhook.py')

    @mrT23 mrT23 requested a review from okotek May 31, 2024 08:15
    @mrT23 mrT23 merged commit 83f3cc5 into Codium-ai:main Jun 2, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants