Skip to content

Feature/otel #76

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

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

Feature/otel #76

wants to merge 13 commits into from

Conversation

pvishwa3
Copy link

@pvishwa3 pvishwa3 commented Mar 3, 2025

PR Type

Enhancement, Bug fix


Description

  • Migrated from BaseHTTPRequestHandler to FastAPI framework.

  • Added new endpoints /sync and /finalize for handling requests.

  • Updated Dockerfile to use Uvicorn for serving FastAPI app.

  • Updated dependencies in requirements.txt to include FastAPI and Uvicorn.


Changes walkthrough 📝

Relevant files
Enhancement
sync.py
Migrated to FastAPI and added new endpoints                           

sync.py

  • Replaced BaseHTTPRequestHandler with FastAPI framework.
  • Added /sync and /finalize endpoints for handling requests.
  • Improved error handling and response structure.
  • Refactored request processing logic for better readability.
  • +52/-57 
    Configuration changes
    Dockerfile
    Updated Dockerfile to use Uvicorn for FastAPI                       

    Dockerfile

  • Updated to install FastAPI and Uvicorn dependencies.
  • Changed command to use Uvicorn for serving FastAPI app.
  • Added --reload flag for development purposes.
  • +5/-2     
    Dependencies
    requirements.txt
    Updated dependencies for FastAPI and Uvicorn                         

    requirements.txt

  • Added FastAPI and Uvicorn dependencies.
  • Ensured compatibility with existing dependencies.
  • +2/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    sonarqubecloud bot commented Mar 3, 2025

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The error handling in the /sync endpoint is incomplete - the function continues execution after catching exceptions without proper return values, which could lead to undefined behavior

    except Exception:
            status = 'Degraded'
            logger.exception(f'failed to parse request: {request_data}')
    try:        
    Dead Code

    Commented out code and debug print statement should be removed before merging

    #request_data = json.loads(request_body)
    print(request_data)
    Development Settings

    The --reload flag should not be used in production as it restarts the server on code changes and impacts performance

    CMD ["uvicorn", "sync:app", "--host", "0.0.0.0", "--port", "80", "--reload"]

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Improve error handling with responses

    The error handling in the /sync endpoint silently swallows exceptions and
    continues execution. Implement proper error handling by returning appropriate
    HTTP error responses when exceptions occur.

    sync.py [74-78]

     try:
             rule_group_namespace = request_data['parent']['spec']['name']
    -except Exception:
    -        status = 'Degraded'
    +except Exception as e:
             logger.exception(f'failed to parse request: {request_data}')
    +        return {"error": "Failed to parse request", "status": "Degraded"}, 400
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The current error handling silently continues execution after exceptions, which could lead to unexpected behavior. Adding proper HTTP error responses is crucial for API reliability and debugging.

    High
    General
    Remove development configuration in production

    Remove the commented out CMD instruction and the --reload flag which should not
    be used in production as it's meant for development only.

    Dockerfile [15-16]

    -CMD ["uvicorn", "sync:app", "--host", "0.0.0.0", "--port", "80", "--reload"]
    -#CMD ["uvicorn", "run", "app/sync.py", "--port", "80"]
    +CMD ["uvicorn", "sync:app", "--host", "0.0.0.0", "--port", "80"]
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Using --reload flag in production is a security risk and can impact performance. Removing it along with unused CMD instruction is important for production deployment safety.

    Medium
    Remove unused debug code

    Remove the commented out code that's not being used. This includes both the
    commented JSON loads line and the print statement, as they add unnecessary
    clutter and could cause confusion.

    sync.py [67-72]

     @app.post("/sync")
     async def post(request: Request):
         request_data = await request.json()  # Accepts any dynamic JSON payload
    -    #request_data = json.loads(request_body)
    -    print(request_data)
         parent = request_data['parent']
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: Removing commented-out code and debug print statements improves code cleanliness and maintainability. While valid, this is a minor improvement that doesn't affect functionality.

    Low
    • More

    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.

    1 participant