Instrument api with opentelemetry #22
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Warning Review failedThe pull request is closed. WalkthroughThe recent updates include significant refactoring and enhancement of repository classes ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MotorRepository
participant Database
Client->>MotorRepository: insert_motor(motor_data)
MotorRepository->>Database: Insert motor data
Database-->>MotorRepository: Confirmation
Client->>MotorRepository: find_motor(motor_id)
MotorRepository->>Database: Query motor by ID
Database-->>MotorRepository: Motor data
Client->>MotorRepository: delete_motor(motor_id)
MotorRepository->>Database: Delete motor by ID
Database-->>MotorRepository: Confirmation
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Outside diff range and nitpick comments (20)
Dockerfile (1)
Line range hint
9-9: Pin versions in apt-get install to ensure reproducible builds.- apt-get install -y --no-install-recommends git + apt-get install -y --no-install-recommends git=<version>lib/controllers/environment.py (7)
7-7: Ensure consistent import formatting.Consider grouping imports from the same module on a single line for clarity and consistency.
30-30: Clarify documentation.The comment "CRUD operations over models.Env on the database" could be expanded to specify which CRUD operations are supported (Create, Read, Update, Delete).
45-45: Improve method documentation.The method
get_rocketpy_envcould benefit from more detailed documentation explaining the parameters and the expected behavior.
Line range hint
89-111: Enhance error messages for better user experience.The error message "Failed to read environment" could be more descriptive. Consider including what operation failed and why, if possible.
Line range hint
165-196: Optimize logging statements.Consider reducing the verbosity of logging or implementing a logging level control to avoid cluttering the log files with routine operation messages.
Line range hint
200-223: Review exception handling strategy.The method
delete_env_by_iduses a broad exception handling strategy which might mask different types of exceptions. Narrow down the exceptions to handle specific cases.
[!TIP]
Codebase VerificationThe
simulate_envmethod in bothlib/routes/environment.pyandlib/controllers/environment.pydoes not include any input validation for theenv_idparameter. Additionally, the methodsget_env_by_idandget_rocketpy_envinlib/controllers/environment.pyalso do not perform any validation onenv_id. TheEnvControllerclass does not seem to have asimulate_envmethod defined within it, based on the search results.
- lib/routes/environment.py: The
simulate_envmethod directly callsEnvController.simulate_env(env_id)without validation.- lib/controllers/environment.py: The
simulate_envmethod does not validateenv_idbefore processing.To ensure robust input handling and prevent potential issues such as SQL injection or logical errors, it is crucial to add validation for the
env_idparameter before it is used in any database operations or further processing.Analysis chain
Line range hint
233-274: Validate input parameters rigorously.Ensure that all input parameters are validated before processing to prevent issues such as SQL injection or logical errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for input validation in the method simulate_env rg --type py "def simulate_env"Length of output: 169
Script:
#!/bin/bash # Check for input validation in the simulate_env method in lib/routes/environment.py rg --context 10 --type py "def simulate_env" lib/routes/environment.py # Check for input validation in the simulate_env method in lib/controllers/environment.py rg --context 10 --type py "def simulate_env" lib/controllers/environment.pyLength of output: 1226
Script:
#!/bin/bash # Search for the definition of EnvController.simulate_env rg --context 10 --type py "def simulate_env" | rg "EnvController" # Search for the definition of get_env_by_id method rg --context 10 --type py "def get_env_by_id" # Search for the definition of get_rocketpy_env method rg --context 10 --type py "def get_rocketpy_env"Length of output: 5282
Script:
#!/bin/bash # Search for the definition of EnvController.simulate_env rg --context 10 --type py "class EnvController" | rg "simulate_env"Length of output: 67
Script:
#!/bin/bash # Search for the definition of the EnvController class and list its methods rg --context 10 --type py "class EnvController"Length of output: 1067
Script:
#!/bin/bash # List all methods within the EnvController class rg --context 10 --type py "class EnvController" -A 100 | rg "def "Length of output: 541
lib/controllers/motor.py (4)
8-8: Standardize import statements.Group imports from the same module on a single line to improve readability.
32-36: Clarify constructor parameters.The constructor of
MotorControllercould use a docstring to explain the parametersmotorandmotor_kind.
Line range hint
49-102: Refactor complex method.The method
get_rocketpy_motoris complex and handles multiple motor types. Consider refactoring into smaller, more focused methods.- match motor.motor_kind: + if motor.motor_kind == MotorKinds.LIQUID: + return create_liquid_motor(motor) + elif motor.motor_kind == MotorKinds.HYBRID: + return create_hybrid_motor(motor) + else: + return create_solid_motor(motor)
137-170: Enhance logging for better traceability.Include more detailed logging, especially in error conditions, to help trace issues more effectively.
lib/controllers/rocket.py (6)
2-2: Organize imports effectively.Consider organizing imports alphabetically or by module to improve readability.
11-17: Document complex imports.Provide comments or documentation for complex imports, especially when importing multiple classes from a module.
45-48: Clarify class responsibilities.The class
RocketControllercould benefit from a clearer explanation of its responsibilities and capabilities in the class docstring.
Line range hint
78-146: Optimize complex method logic.The method
get_rocketpy_rocketis very complex and does a lot of work. Consider breaking it down into smaller, more manageable methods.
177-211: Enhance method documentation and error handling.The method
get_rocket_by_idcould use more detailed documentation, especially regarding its return values and error handling.
Line range hint
324-497: Optimize simulation logic for performance.The method
simulate_rocketperforms complex calculations and simulations. Consider optimizing these operations for better performance, possibly by caching results or using more efficient algorithms.lib/controllers/flight.py (2)
57-63: Consider adding type hints for the constructor parameters.Adding type hints for
rocket_optionandmotor_kindin the constructor would improve code readability and help with static type checking.
Line range hint
367-651: Convert to f-string for better readability and performance.The method
simulate_flightuses the olderformatmethod for string formatting. Converting these to f-strings would enhance readability and performance.- "Maximum Speed: {:.3f} m/s at {:.2f} s".format( - flight.max_speed, flight.max_speed_time - ) + f"Maximum Speed: {flight.max_speed:.3f} m/s at {flight.max_speed_time:.2f} s"
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
lib/data/engines/Cesaroni_7450M2505-P.csvis excluded by!**/*.csv
Files selected for processing (27)
- Dockerfile (1 hunks)
- lib/init.py (1 hunks)
- lib/api.py (3 hunks)
- lib/controllers/init.py (1 hunks)
- lib/controllers/environment.py (13 hunks)
- lib/controllers/flight.py (3 hunks)
- lib/controllers/motor.py (5 hunks)
- lib/controllers/rocket.py (6 hunks)
- lib/models/aerosurfaces.py (1 hunks)
- lib/models/environment.py (1 hunks)
- lib/models/flight.py (1 hunks)
- lib/models/motor.py (5 hunks)
- lib/models/rocket.py (2 hunks)
- lib/repositories/init.py (1 hunks)
- lib/repositories/environment.py (3 hunks)
- lib/repositories/flight.py (1 hunks)
- lib/repositories/motor.py (1 hunks)
- lib/repositories/repo.py (1 hunks)
- lib/repositories/rocket.py (1 hunks)
- lib/routes/environment.py (3 hunks)
- lib/routes/flight.py (5 hunks)
- lib/routes/motor.py (3 hunks)
- lib/routes/rocket.py (3 hunks)
- lib/secrets.py (1 hunks)
- lib/settings/gunicorn.py (1 hunks)
- pyproject.toml (3 hunks)
- requirements.txt (1 hunks)
Files not reviewed due to errors (2)
- lib/routes/motor.py (no review received)
- lib/routes/rocket.py (no review received)
Files skipped from review due to trivial changes (3)
- lib/controllers/init.py
- lib/models/aerosurfaces.py
- requirements.txt
Additional context used
Hadolint
Dockerfile
[warning] 9-9: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>(DL3008)
Ruff
lib/repositories/repo.py
18-18: Use
super()instead ofsuper(__class__, self)(UP008)Remove
__super__parameterslib/controllers/motor.py
305-305: Use f-string instead of
formatcall (UP032)Convert to f-string
308-310: Use f-string instead of
formatcall (UP032)Convert to f-string
313-313: Use f-string instead of
formatcall (UP032)Convert to f-string
321-321: Use f-string instead of
formatcall (UP032)Convert to f-string
lib/controllers/rocket.py
346-348: Use f-string instead of
formatcall (UP032)Convert to f-string
349-351: Use f-string instead of
formatcall (UP032)Convert to f-string
353-355: Use f-string instead of
formatcall (UP032)Convert to f-string
356-358: Use f-string instead of
formatcall (UP032)Convert to f-string
359-361: Use f-string instead of
formatcall (UP032)Convert to f-string
362-364: Use f-string instead of
formatcall (UP032)Convert to f-string
365-367: Use f-string instead of
formatcall (UP032)Convert to f-string
368-370: Use f-string instead of
formatcall (UP032)Convert to f-string
379-379: Use f-string instead of
formatcall (UP032)Convert to f-string
410-412: Use f-string instead of
formatcall (UP032)Convert to f-string
423-423: Use f-string instead of
formatcall (UP032)Convert to f-string
431-433: Use f-string instead of
formatcall (UP032)Convert to f-string
436-436: Use f-string instead of
formatcall (UP032)Convert to f-string
439-441: Use f-string instead of
formatcall (UP032)Convert to f-string
lib/controllers/flight.py
389-391: Use f-string instead of
formatcall (UP032)Convert to f-string
392-394: Use f-string instead of
formatcall (UP032)Convert to f-string
395-397: Use f-string instead of
formatcall (UP032)Convert to f-string
398-400: Use f-string instead of
formatcall (UP032)Convert to f-string
401-403: Use f-string instead of
formatcall (UP032)Convert to f-string
407-409: Use f-string instead of
formatcall (UP032)Convert to f-string
410-412: Use f-string instead of
formatcall (UP032)Convert to f-string
413-415: Use f-string instead of
formatcall (UP032)Convert to f-string
429-431: Use f-string instead of
formatcall (UP032)Convert to f-string
432-434: Use f-string instead of
formatcall (UP032)Convert to f-string
435-437: Use f-string instead of
formatcall (UP032)Convert to f-string
441-443: Use f-string instead of
formatcall (UP032)Convert to f-string
444-446: Use f-string instead of
formatcall (UP032)Convert to f-string
450-452: Use f-string instead of
formatcall (UP032)Convert to f-string
453-455: Use f-string instead of
formatcall (UP032)Convert to f-string
456-458: Use f-string instead of
formatcall (UP032)Convert to f-string
459-461: Use f-string instead of
formatcall (UP032)Convert to f-string
462-464: Use f-string instead of
formatcall (UP032)Convert to f-string
465-467: Use f-string instead of
formatcall (UP032)Convert to f-string
471-473: Use f-string instead of
formatcall (UP032)Convert to f-string
478-480: Use f-string instead of
formatcall (UP032)Convert to f-string
498-500: Use f-string instead of
formatcall (UP032)Convert to f-string
501-503: Use f-string instead of
formatcall (UP032)Convert to f-string
507-509: Use f-string instead of
formatcall (UP032)Convert to f-string
510-510: Use f-string instead of
formatcall (UP032)Convert to f-string
511-513: Use f-string instead of
formatcall (UP032)Convert to f-string
517-519: Use f-string instead of
formatcall (UP032)Convert to f-string
520-522: Use f-string instead of
formatcall (UP032)Convert to f-string
523-525: Use f-string instead of
formatcall (UP032)Convert to f-string
526-529: Use f-string instead of
formatcall (UP032)Convert to f-string
530-532: Use f-string instead of
formatcall (UP032)Convert to f-string
540-543: Use f-string instead of
formatcall (UP032)Convert to f-string
544-547: Use f-string instead of
formatcall (UP032)Convert to f-string
548-550: Use f-string instead of
formatcall (UP032)Convert to f-string
551-553: Use f-string instead of
formatcall (UP032)Convert to f-string
554-556: Use f-string instead of
formatcall (UP032)Convert to f-string
557-559: Use f-string instead of
formatcall (UP032)Convert to f-string
564-566: Use f-string instead of
formatcall (UP032)Convert to f-string
567-569: Use f-string instead of
formatcall (UP032)Convert to f-string
570-572: Use f-string instead of
formatcall (UP032)Convert to f-string
573-575: Use f-string instead of
formatcall (UP032)Convert to f-string
579-579: Use f-string instead of
formatcall (UP032)Convert to f-string
580-582: Use f-string instead of
formatcall (UP032)Convert to f-string
601-603: Use f-string instead of
formatcall (UP032)Convert to f-string
607-607: Use f-string instead of
formatcall (UP032)Convert to f-string
611-613: Use f-string instead of
formatcall (UP032)Convert to f-string
617-619: Use f-string instead of
formatcall (UP032)Convert to f-string
Additional comments not posted (29)
lib/models/flight.py (1)
13-15: LGTM! Usinghashforflight_idis a straightforward way to ensure uniqueness based on the object's state.lib/secrets.py (1)
16-20: The fallback mechanism inget_secretis well-implemented, ensuring robust secret retrieval.lib/models/environment.py (1)
18-20: LGTM! Consistent use ofhashforenv_idaligns well with the approach used in other models.Dockerfile (1)
19-19: The CMD command is correctly set up for running the application with gunicorn and uvicorn workers.lib/settings/gunicorn.py (1)
6-17: The OpenTelemetry configuration is correctly set up for production environment tracing.lib/__init__.py (1)
5-20: The logging configuration is correctly set up with both file and stdout handlers.pyproject.toml (1)
1-7: The build system configuration is correctly set up to dynamically include dependencies fromrequirements.txt.lib/models/rocket.py (1)
47-49: LGTM! Proper use of property for encapsulation.lib/routes/environment.py (6)
39-40: Proper implementation of OpenTelemetry tracing. Good for monitoring and debugging.
51-52: Consistent and correct use of OpenTelemetry tracing.
66-67: Consistent implementation of OpenTelemetry tracing across different routes.
78-79: Good consistency in the implementation of OpenTelemetry tracing.
90-91: Consistent and correct use of OpenTelemetry tracing.
102-103: Consistent implementation of OpenTelemetry tracing across different routes.lib/api.py (1)
38-38: Update API version to match project version.lib/repositories/environment.py (1)
17-21: Ensure proper initialization of repository with default values.lib/repositories/motor.py (1)
17-21: Ensure proper initialization of repository with default values.lib/repositories/repo.py (1)
56-83: Ensure robust connection handling and proper cleanup.lib/routes/flight.py (2)
31-32: Initialization of tracer is correct and follows best practices for OpenTelemetry.
44-47: The use oftracer.start_as_current_spanis consistent and correctly scopes each API operation within its own span.Also applies to: 58-59, 70-73, 87-90, 109-115, 134-137, 148-149, 160-161
lib/repositories/flight.py (3)
17-21: Initialization of the repository with the collection name "flights" is correct. The use of private attributes forflightandflight_idis a good encapsulation practice.
22-25: Thefetch_flightmethod and property setters/getters are implemented correctly, ensuring that the flight object and its ID are managed properly.Also applies to: 27-33, 35-41
83-105: The methods for retrieving and deleting flights by ID are implemented correctly. The logging statements are helpful for debugging and monitoring.Also applies to: 107-125
lib/repositories/rocket.py (2)
17-21: Initialization and attribute management inRocketRepositoryare correctly implemented, mirroring the structure and practices used inFlightRepository.
22-25: Thefetch_rocketmethod and associated properties are correctly implemented, ensuring proper management of rocket objects and their IDs.Also applies to: 27-41
lib/models/motor.py (2)
14-16: The addition of new enum values forMotorKinds,MotorEngines, andTankKindsis correct and enhances the flexibility of motor configurations.Also applies to: 20-21, 25-28
Line range hint
42-142: TheMotorTankandMotorclasses are well-defined with appropriate properties and methods. The use ofPrivateAttrfor_motor_kindinMotoris a good practice for encapsulation.lib/controllers/rocket.py (1)
254-320: Validate and sanitize inputs.Ensure that all input parameters are properly validated and sanitized to prevent SQL injection and other security issues.
lib/controllers/flight.py (1)
243-284: Clarify the purpose ofupdate_env_by_flight_id.The method
update_env_by_flight_idupdates the environment of a flight but also recreates and deletes the flight. Verify if this is the intended behavior or if there's a logical error.
| secrets = dotenv_values(".env") | ||
|
|
||
| @staticmethod | ||
| def get_os_secret(key): | ||
| return os.environ.get(key) | ||
|
|
||
| def get_secret(self, key): | ||
| dotenv_secret = self.secrets.get(key) | ||
| @classmethod | ||
| def get_secret(cls, key): | ||
| dotenv_secret = cls.secrets.get(key) | ||
| if not dotenv_secret: | ||
| return self.get_os_secret(key) | ||
| return cls.get_os_secret(key) |
There was a problem hiding this comment.
Consider making the secrets dictionary private to encapsulate its access within the Secrets class.
- secrets = dotenv_values(".env")
+ _secrets = dotenv_values(".env")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| secrets = dotenv_values(".env") | |
| @staticmethod | |
| def get_os_secret(key): | |
| return os.environ.get(key) | |
| def get_secret(self, key): | |
| dotenv_secret = self.secrets.get(key) | |
| @classmethod | |
| def get_secret(cls, key): | |
| dotenv_secret = cls.secrets.get(key) | |
| if not dotenv_secret: | |
| return self.get_os_secret(key) | |
| return cls.get_os_secret(key) | |
| _secrets = dotenv_values(".env") | |
| @staticmethod | |
| def get_os_secret(key): | |
| return os.environ.get(key) | |
| @classmethod | |
| def get_secret(cls, key): | |
| dotenv_secret = cls._secrets.get(key) | |
| if not dotenv_secret: | |
| return cls.get_os_secret(key) |
lib/api.py
Outdated
| exc_str = parse_error(exc) | ||
| logger.error(f"{request}: {exc_str}") | ||
| content = {"status_code": 10422, "message": exc_str, "data": None} | ||
| return JSONResponse( | ||
| content=content, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY | ||
| content=exc_str, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY |
There was a problem hiding this comment.
Refactor error handling to improve readability and maintainability.
- exc_str = parse_error(exc)
- logger.error(f"{request}: {exc_str}")
- return JSONResponse(
- content=exc_str, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY
+ error_message = parse_error(exc)
+ logger.error(f"{request}: {error_message}")
+ return JSONResponse(
+ content={"error": error_message}, status_code=status.HTTP_422_UNPROCESSABLE_ENTITYCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exc_str = parse_error(exc) | |
| logger.error(f"{request}: {exc_str}") | |
| content = {"status_code": 10422, "message": exc_str, "data": None} | |
| return JSONResponse( | |
| content=content, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY | |
| content=exc_str, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY | |
| error_message = parse_error(exc) | |
| logger.error(f"{request}: {error_message}") | |
| return JSONResponse( | |
| content={"error": error_message}, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY |
lib/controllers/flight.py
Outdated
| async def update_flight_by_id( | ||
| self, flight_id: str | ||
| ) -> Union[FlightUpdated, HTTPException]: | ||
| """ | ||
| Update a flight in the database. | ||
| Update a models.Flight in the database. | ||
|
|
||
| Args: | ||
| flight_id (int): Flight id. | ||
| flight_id: str | ||
|
|
||
| Returns: | ||
| FlightUpdated: Flight id and message. | ||
| views.FlightUpdated | ||
|
|
||
| Raises: | ||
| HTTP 404 Not Found: If the flight is not found in the database. | ||
| """ | ||
| successfully_read_flight = await FlightRepository( | ||
| flight_id=flight_id | ||
| ).get_flight() | ||
| if not successfully_read_flight: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Flight not found.", | ||
| ) | ||
|
|
||
| successfully_updated_flight = await FlightRepository( | ||
| flight=self.flight, flight_id=flight_id | ||
| ).update_flight( | ||
| rocket_option=self.rocket_option, motor_kind=self.motor_kind | ||
| ) | ||
| if not successfully_updated_flight: | ||
| try: | ||
| async with FlightRepository() as flight_repo: | ||
| flight_repo.fetch_flight(self.flight) | ||
| await flight_repo.create_flight( | ||
| motor_kind=self.motor_kind, | ||
| rocket_option=self.rocket_option, | ||
| ) | ||
| await flight_repo.delete_flight_by_id(flight_id) | ||
| except Exception as e: | ||
| exc_str = parse_error(e) | ||
| logger.error(f"controllers.flight.update_flight: {exc_str}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to update flight.", | ||
| detail=f"Failed to update flight: {exc_str}", | ||
| ) from e | ||
| else: | ||
| return FlightUpdated(new_flight_id=self.flight.flight_id) | ||
| finally: | ||
| logger.info( | ||
| f"Call to controllers.flight.update_flight completed for Flight {flight_id}" |
There was a problem hiding this comment.
Review logic in update_flight_by_id.
The method update_flight_by_id seems to recreate and then delete a flight, which might not be the intended behavior. Clarify the logic and ensure it aligns with the intended functionality.
lib/controllers/flight.py
Outdated
| @classmethod | ||
| async def update_rocket_by_flight_id( | ||
| cls, flight_id: str, *, rocket: Rocket, rocket_option, motor_kind | ||
| ) -> Union[FlightUpdated, HTTPException]: | ||
| """ | ||
| Update the rocket of a flight in the database. | ||
| Update a models.Flight.rocket in the database. | ||
|
|
||
| Args: | ||
| flight_id (int): Flight id. | ||
| rocket (models.Rocket): Rocket model object. | ||
| flight_id: str | ||
| rocket: models.Rocket | ||
|
|
||
| Returns: | ||
| FlightUpdated: Flight id and message. | ||
| views.FlightUpdated | ||
|
|
||
| Raises: | ||
| HTTP 404 Not Found: If the flight is not found in the database. | ||
| """ | ||
| successfully_read_flight = await FlightRepository( | ||
| flight_id=flight_id | ||
| ).get_flight() | ||
| if not successfully_read_flight: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Flight not found.", | ||
| ) | ||
|
|
||
| flight = successfully_read_flight.dict() | ||
| updated_rocket = rocket.dict() | ||
| updated_rocket["rocket_option"] = rocket_option | ||
| updated_rocket["motor"]["motor_kind"] = motor_kind | ||
| flight["rocket"] = Rocket(**updated_rocket) | ||
| flight = Flight(**flight) | ||
| successfully_updated_flight = await FlightRepository( | ||
| flight=flight, flight_id=flight_id | ||
| ).update_flight() | ||
| if not successfully_updated_flight: | ||
| try: | ||
| read_flight = await cls.get_flight_by_id(flight_id) | ||
| updated_rocket = rocket.dict() | ||
| updated_rocket["rocket_option"] = rocket_option | ||
| updated_rocket["motor"]["motor_kind"] = motor_kind | ||
| new_flight = read_flight.dict() | ||
| new_flight["rocket"] = updated_rocket | ||
| new_flight = Flight(**new_flight) | ||
| async with FlightRepository() as flight_repo: | ||
| flight_repo.fetch_flight(new_flight) | ||
| await flight_repo.create_flight( | ||
| motor_kind=motor_kind, rocket_option=rocket_option | ||
| ) | ||
| await flight_repo.delete_flight_by_id(flight_id) | ||
| except HTTPException as e: | ||
| raise e from e | ||
| except Exception as e: | ||
| exc_str = parse_error(e) | ||
| logger.error(f"controllers.flight.update_rocket: {exc_str}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to update flight.", | ||
| detail=f"Failed to update rocket: {exc_str}", | ||
| ) from e | ||
| else: | ||
| return FlightUpdated(new_flight_id=new_flight.flight_id) | ||
| finally: | ||
| logger.info( | ||
| f"Call to controllers.flight.update_rocket completed for Flight {flight_id}; Rocket {hash(rocket)}" |
There was a problem hiding this comment.
Check the logic in update_rocket_by_flight_id.
The method update_rocket_by_flight_id updates the rocket details but also recreates and deletes the flight. This could be a potential logical error. Review and confirm the intended functionality.
lib/routes/flight.py
Outdated
| @@ -3,6 +3,7 @@ | |||
| """ | |||
|
|
|||
| from fastapi import APIRouter | |||
| from opentelemetry import trace | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The opentelemetry-api and opentelemetry-sdk packages are present in requirements.txt, but opentelemetry-instrumentation-fastapi is missing. This package is likely necessary for instrumenting FastAPI applications with OpenTelemetry.
- Ensure
opentelemetry-instrumentation-fastapiis added torequirements.txt.
Analysis chain
Ensure that the OpenTelemetry package is correctly installed and configured.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the installation of OpenTelemetry packages.
# Test: Check if the required OpenTelemetry packages are in requirements.txt
grep -E 'opentelemetry-(api|sdk|instrumentation-fastapi)' requirements.txt
Length of output: 112
lib/controllers/motor.py
Outdated
| @classmethod | ||
| async def get_rocketpy_motor_as_jsonpickle( | ||
| cls, | ||
| motor_id: str, | ||
| ) -> Union[MotorPickle, HTTPException]: | ||
| """ | ||
| Get a rocketpy motor object encoded as jsonpickle string from the database. | ||
| Get a rocketpy.Motor object as a jsonpickle string. | ||
|
|
||
| Args: | ||
| motor_id (int): Motor id. | ||
| motor_id: str | ||
|
|
||
| Returns: | ||
| str: jsonpickle string of the rocketpy motor. | ||
| views.MotorPickle | ||
|
|
||
| Raises: | ||
| HTTP 404 Not Found: If the motor is not found in the database. | ||
| """ | ||
| successfully_read_motor = await MotorRepository( | ||
| motor_id=motor_id | ||
| ).get_motor() | ||
| if not successfully_read_motor: | ||
| try: | ||
| read_motor = await cls.get_motor_by_id(motor_id) | ||
| rocketpy_motor = cls.get_rocketpy_motor(read_motor) | ||
| except HTTPException as e: | ||
| raise e from e | ||
| except Exception as e: | ||
| exc_str = parse_error(e) | ||
| logger.error( | ||
| f"controllers.motor.get_rocketpy_motor_as_jsonpickle: {exc_str}" | ||
| ) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Motor not found.", | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Failed to read motor: {exc_str}", | ||
| ) from e | ||
| else: | ||
| return MotorPickle( | ||
| jsonpickle_rocketpy_motor=jsonpickle.encode(rocketpy_motor) | ||
| ) | ||
|
|
||
| successfully_read_rocketpy_motor = MotorController( | ||
| motor=successfully_read_motor, | ||
| motor_kind=MotorKinds(successfully_read_motor._motor_kind), | ||
| ).rocketpy_motor | ||
|
|
||
| return MotorPickle( | ||
| jsonpickle_rocketpy_motor=jsonpickle.encode( | ||
| successfully_read_rocketpy_motor | ||
| finally: | ||
| logger.info( |
There was a problem hiding this comment.
Review and optimize database interactions.
Ensure that database interactions are efficient and handle possible exceptions gracefully. Consider adding retries or more sophisticated error handling strategies.
lib/controllers/motor.py
Outdated
| """ | ||
| Update a motor in the database. | ||
|
|
||
| Args: | ||
| motor_id (int): Motor id. | ||
| motor_id: str | ||
|
|
||
| Returns: | ||
| MotorUpdated: motor id and message. | ||
| views.MotorUpdated | ||
|
|
||
| Raises: | ||
| HTTP 404 Not Found: If the motor is not found in the database. | ||
| """ | ||
| successfully_read_motor = await MotorRepository( | ||
| motor_id=motor_id | ||
| ).get_motor() | ||
| if not successfully_read_motor: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Motor not found.", | ||
| ) | ||
|
|
||
| successfully_updated_motor = await MotorRepository( | ||
| motor=self.motor, motor_id=motor_id | ||
| ).update_motor(motor_kind=self.motor_kind) | ||
| if not successfully_updated_motor: | ||
| try: | ||
| async with MotorRepository() as motor_repo: | ||
| motor_repo.fetch_motor(self.motor) | ||
| await motor_repo.create_motor(motor_kind=self.motor_kind) | ||
| await motor_repo.delete_motor_by_id(motor_id) | ||
| except Exception as e: | ||
| exc_str = parse_error(e) | ||
| logger.error(f"controllers.motor.update_motor: {exc_str}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to update motor.", | ||
| detail=f"Failed to update motor: {exc_str}", | ||
| ) from e | ||
| else: | ||
| return MotorUpdated(new_motor_id=self.motor.motor_id) | ||
| finally: | ||
| logger.info( | ||
| f"Call to controllers.motor.update_motor completed for Motor {motor_id}" | ||
| ) | ||
|
|
||
| return MotorUpdated(new_motor_id=str(successfully_updated_motor)) | ||
|
|
||
| @staticmethod | ||
| async def delete_motor( | ||
| motor_id: int, | ||
| ) -> "Union[MotorDeleted, HTTPException]": | ||
| async def delete_motor_by_id( | ||
| motor_id: str, | ||
| ) -> Union[MotorDeleted, HTTPException]: | ||
| """ | ||
| Delete a motor from the database. | ||
| Delete a models.Motor from the database. | ||
|
|
||
| Args: | ||
| motor_id (int): motor id. | ||
| motor_id: str | ||
|
|
||
| Returns: | ||
| MotorDeleted: motor id and message. | ||
| views.MotorDeleted | ||
|
|
||
| Raises: | ||
| HTTP 404 Not Found: If the motor is not found in the database. | ||
| """ | ||
| successfully_read_motor = await MotorRepository( | ||
| motor_id=motor_id | ||
| ).get_motor() | ||
| if not successfully_read_motor: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Motor not found.", | ||
| ) | ||
|
|
||
| successfully_deleted_motor = await MotorRepository( | ||
| motor_id=motor_id | ||
| ).delete_motor() | ||
| if not successfully_deleted_motor: | ||
| try: | ||
| async with MotorRepository() as motor_repo: | ||
| await motor_repo.delete_motor_by_id(motor_id) | ||
| except Exception as e: | ||
| exc_str = parse_error(e) | ||
| logger.error(f"controllers.motor.delete_motor: {exc_str}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to delete motor.", | ||
| detail=f"Failed to delete motor: {exc_str}", | ||
| ) from e | ||
| else: | ||
| return MotorDeleted(deleted_motor_id=motor_id) | ||
| finally: | ||
| logger.info( | ||
| f"Call to controllers.motor.delete_motor completed for Motor {motor_id}" |
There was a problem hiding this comment.
Ensure consistency in method outputs.
The method simulate_motor should consistently return structured data or raise an exception. Ensure that all paths adhere to this pattern.
lib/controllers/rocket.py
Outdated
| def __init__( | ||
| self, | ||
| *, | ||
| rocket: Rocket, | ||
| rocket_option: RocketOptions, | ||
| motor_kind: MotorKinds, | ||
| ): | ||
| self._rocket = rocket | ||
| self._rocket_option = rocket_option | ||
| self._motor_kind = motor_kind | ||
|
|
There was a problem hiding this comment.
Refactor constructor for clarity.
The constructor of RocketController is complex. Consider simplifying or refactoring to improve readability and maintainability.
lib/controllers/rocket.py
Outdated
| @staticmethod | ||
| def check_trigger(expression: str) -> bool: | ||
| """ | ||
| Check if the trigger expression is valid. | ||
|
|
||
| Args: | ||
| expression (str): Trigger expression. | ||
|
|
||
| Returns: | ||
| bool: True if the expression is valid, False otherwise. | ||
| """ | ||
|
|
||
| # Parsing the expression into an AST | ||
| try: | ||
| parsed_expression = ast.parse(expression, mode="eval") | ||
| except SyntaxError: | ||
| print("Invalid syntax.") | ||
| return False | ||
|
|
||
| # Constant case (supported after beta v1) | ||
| if isinstance(parsed_expression.body, ast.Constant): | ||
| return True | ||
| # Name case (supported after beta v1) | ||
| if ( | ||
| isinstance(parsed_expression.body, ast.Name) | ||
| and parsed_expression.body.id == "apogee" | ||
| ): | ||
| global apogee | ||
| apogee = "apogee" | ||
| return True | ||
|
|
||
| # Validating the expression structure | ||
| if not isinstance(parsed_expression.body, ast.Lambda): | ||
| print("Invalid expression structure (not a Lambda).") | ||
| return False | ||
|
|
||
| lambda_node = parsed_expression.body | ||
| if len(lambda_node.args.args) != 3: | ||
| print("Invalid expression structure (invalid arity).") | ||
| return False | ||
|
|
||
| if not isinstance(lambda_node.body, ast.Compare): | ||
| try: | ||
| for operand in lambda_node.body.values: | ||
| if not isinstance(operand, ast.Compare): | ||
| print( | ||
| "Invalid expression structure (not a Compare)." | ||
| ) | ||
| return False | ||
| except AttributeError: | ||
| print("Invalid expression structure (not a Compare).") | ||
| return False | ||
|
|
||
| # Restricting access to functions or attributes | ||
| for node in ast.walk(lambda_node): | ||
| if isinstance(node, ast.Call): | ||
| print( | ||
| "Calling functions is not allowed in the expression." | ||
| ) | ||
| return False | ||
| if isinstance(node, ast.Attribute): | ||
| print( | ||
| "Accessing attributes is not allowed in the expression." | ||
| ) | ||
| return False | ||
| return True | ||
|
|
||
| async def create_rocket(self) -> "Union[RocketCreated, HTTPException]": | ||
| """ | ||
| Create a rocket in the database. | ||
| Create a models.Rocket in the database. | ||
|
|
||
| Returns: | ||
| RocketCreated: Rocket id. | ||
| views.RocketCreated | ||
| """ | ||
| rocket = RocketRepository(rocket=self.rocket) | ||
| successfully_created_rocket = await rocket.create_rocket( | ||
| rocket_option=self.rocket_option | ||
| ) | ||
| if not successfully_created_rocket: | ||
| try: | ||
| async with RocketRepository() as rocket_repo: | ||
| rocket_repo.fetch_rocket(self.rocket) | ||
| await rocket_repo.create_rocket( | ||
| rocket_option=self.rocket_option, | ||
| motor_kind=self.motor_kind, | ||
| ) | ||
| except Exception as e: | ||
| exc_str = parse_error(e) | ||
| logger.error(f"controllers.rocket.create_rocket: {exc_str}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail="Failed to create rocket.", | ||
| detail=f"Failed to create rocket: {exc_str}", | ||
| ) from e | ||
| else: | ||
| return RocketCreated(rocket_id=self.rocket.rocket_id) | ||
| finally: | ||
| logger.info( |
There was a problem hiding this comment.
Improve error handling and logging.
The method create_rocket uses broad exception handling. Narrow down the exceptions and improve logging for better diagnostics.
lib/controllers/rocket.py
Outdated
| @classmethod | ||
| async def get_rocketpy_rocket_as_jsonpickle( | ||
| cls, rocket_id: str | ||
| ) -> Union[RocketPickle, HTTPException]: | ||
| """ | ||
| Get a rocketpy rocket object encoded as jsonpickle string from the database. | ||
| Get a rocketpy.Rocket object as jsonpickle string. | ||
|
|
||
| Args: | ||
| rocket_id (int): rocket id. | ||
| rocket_id: str | ||
|
|
||
| Returns: | ||
| str: jsonpickle string of the rocketpy rocket. | ||
| views.RocketPickle | ||
|
|
||
| Raises: | ||
| HTTP 404 Not Found: If the rocket is not found in the database. | ||
| """ | ||
| successfully_read_rocket = await RocketRepository( | ||
| rocket_id=rocket_id | ||
| ).get_rocket() | ||
| if not successfully_read_rocket: | ||
| try: | ||
| read_rocket = await cls.get_rocket_by_id(rocket_id) | ||
| rocketpy_rocket = cls.get_rocketpy_rocket(read_rocket) | ||
| except HTTPException as e: | ||
| raise e from e | ||
| except Exception as e: | ||
| exc_str = parse_error(e) | ||
| logger.error( | ||
| f"controllers.rocket.get_rocketpy_rocket_as_jsonpickle: {exc_str}" | ||
| ) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Rocket not found.", | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Failed to read rocket: {exc_str}", | ||
| ) from e | ||
| else: | ||
| return RocketPickle( | ||
| jsonpickle_rocketpy_rocket=jsonpickle.encode(rocketpy_rocket) | ||
| ) | ||
|
|
||
| successfully_read_rocketpy_rocket = RocketController( | ||
| rocket=successfully_read_rocket, | ||
| rocket_option=RocketOptions( | ||
| successfully_read_rocket._rocket_option | ||
| ), | ||
| motor_kind=MotorKinds(successfully_read_rocket.motor._motor_kind), | ||
| ).rocketpy_rocket | ||
|
|
||
| return RocketPickle( | ||
| jsonpickle_rocketpy_rocket=jsonpickle.encode( | ||
| successfully_read_rocketpy_rocket | ||
| finally: | ||
| logger.info( | ||
| f"Call to controllers.rocket.get_rocketpy_rocket_as_jsonpickle completed for Rocket {rocket_id}" |
There was a problem hiding this comment.
Review JSON handling and error propagation.
The method get_rocketpy_rocket_as_jsonpickle handles JSON operations and errors. Ensure that errors are properly logged and handled.
This PR intends to instrument infinity-API with opentelemetry hosting spans on Uptrace.
In terms of cost, Uptrace provides 50GB or 2500 time-series each month for free, which probably will suit our needs for years.
Summary by CodeRabbit
New Features
Chores
requirements.txt.Dependencies
opentelemetry.instrumentation.fastapi,opentelemetry-api,opentelemetry-sdk, anduptracetorequirements.txt.