#283: Improve health endpoint with missing required conditions #291
Conversation
…ing-required-conditions
|
Hi @RobyBen can you check failing pipelines? I think you don't have your development environment configured with linting and pre-commit too. For more info check global set up docshttps://antoniomrtz.github.io/SpotifyElectron/developer/SETUP/ and set tup backend docs |
|
|
||
| class HealthCheckResponse(BaseModel): |
There was a problem hiding this comment.
Move model class to schema file instead of having it into the controller + class docstring
|
|
||
| class HealthCheckResponse(BaseModel): | ||
| status: str | ||
| details: Dict[str, Dict[str, str]] |
There was a problem hiding this comment.
Use dict instead of Dict. See docs. This applies to list, dict, tuple type hints
| """Validates if the app has launched correctly | ||
| async def check_database_connection() -> Dict[str, str]: | ||
| try: | ||
| db_client = DatabaseConnectionManager.get_database_client() |
There was a problem hiding this comment.
Create a method inside db connection client that handles the ping and use DatabasePingFailed exception already declared at database_schema
| @router.get("/", summary="Health Check Endpoint") | ||
| def get_health() -> Response: | ||
| """Validates if the app has launched correctly | ||
| async def check_database_connection() -> Dict[str, str]: |
There was a problem hiding this comment.
This logic belongs to a service layer more than a controller one. Move functions that handle logic to health_service
| def check_auth_config() -> Dict[str, str]: | ||
| try: | ||
| if not all([ | ||
| hasattr(AuthConfig, 'SIGNING_SECRET_KEY') and AuthConfig.SIGNING_SECRET_KEY, |
There was a problem hiding this comment.
PropertiesManager has all the names of environment attributes in env_variables. Try not hardcode keys like that because it's better to only have one source of truth instead of multiple around the code
| ) | ||
| async def get_health() -> JSONResponse: | ||
| """Validates if the app has launched correctly and all critical components are healthy | ||
|
|
||
| Returns |
There was a problem hiding this comment.
Invalid docs format, check stream_controller. Docs have to follow the following format:
"""Streams song audio
Args:
name (str): song name
request (Request): incoming request
"""
| service_health = check_song_service() | ||
| auth_health = check_auth_config() | ||
|
|
||
| health_details = { |
| status_code = HTTP_200_OK if is_healthy else HTTP_503_SERVICE_UNAVAILABLE | ||
|
|
||
| if not is_healthy: | ||
| logger.warning(f"Health check failed: {health_details}") |
There was a problem hiding this comment.
I dont think this is needed, we're going to return 503 if healthcheck failed
| await db_client.admin.command('ping') | ||
| return {"status": "healthy", "message": "Database connection is active"} | ||
| except Exception as e: | ||
| logger.error(f"Database health check failed: {str(e)}") |
There was a problem hiding this comment.
Use loggin exception instead of error. More info here
| from app.database.DatabaseConnectionManager import DatabaseConnectionManager | ||
| from app.spotify_electron.song.providers.song_service_provider import SongServiceProvider | ||
| from app.logging.logging_schema import SpotifyElectronLogger | ||
| from app.logging.logging_constants import LOGGING_HEALTH |
There was a problem hiding this comment.
Is this tested? This imports appear to be non existing
AntonioMrtz
changed the title
Description
Improve health endpoint with missing required conditions
Commit type
feat: indicates the addition of a new feature or functionality to the project.Issue
#283
Solution
Add health end points
Proposed Changes
Potential Impact
info about problems in project
Screenshots
None
Additional Tasks
Tests.
Assigned
@AntonioMrtz