feat: log routes when the router is populated #609
Conversation
👷 Deploy request for robyn pending review.Visit the deploys page to approve it
|
CodSpeed Performance ReportMerging #609 will not alter performanceComparing Summary
|
robyn/__init__.py
Outdated
| @@ -173,6 +173,16 @@ def start(self, url: str = "127.0.0.1", port: int = 8080): | |||
| logger.info(f"Robyn version: {__version__}") | |||
| logger.info(f"Starting server at {url}:{port}") | |||
|
|
|||
| endpoint = {} | |||
|
|
|||
| routes = self.router.get_routes() | |||
There was a problem hiding this comment.
I think we should write the logs in the add_route method, instead of doing the get_routes here - it's minor, but it slows down the startup time
robyn/__init__.py
Outdated
| endpoint["method"] = route[0] | ||
| endpoint["route"] = route[1] | ||
|
|
||
| logger.info("Logging endpoint: {}".format(endpoint)) |
There was a problem hiding this comment.
I think the wording here is not very clear, maybe change the word Logging to Adding.
Also, I think using f-strings is nicer and more consistent with the rest of the code base
|
Hey @carlosm27 👋 Great work! Added a few comments, let me know if something is unclear! |
robyn/__init__.py
Outdated
|
|
||
| routes = self.router.get_routes() | ||
|
|
||
| for route in routes: |
There was a problem hiding this comment.
Hey @carlosm27 👋
Thank you for the PR 😄
We should be using something like this
| for route in routes: | |
| for method, route, _ in routes: | |
| logger.info("Logging endpoint: {method: %s, route: %s}", method, route) |
As the logging library is optimized for deferred string evaluation https://docs.python.org/3/howto/logging.html#optimization
There was a problem hiding this comment.
Thank you for the reviews @IdoKendo 😄
The logging routes is in the `add_route` method instead of in the `start` method. Also using f-string to format the logging
|
I tried this approach: But I receive the following error message:
I used f-string instead for the logging: I will continue researching to solve this. |
robyn/__init__.py
Outdated
| @@ -99,6 +99,11 @@ def add_route( | |||
| } | |||
| route_type = http_methods[route_type] | |||
|
|
|||
| routes = self.router.get_routes() | |||
|
|
|||
| for route in routes: | |||
There was a problem hiding this comment.
You don't need to do the for loop here, this function runs for every route separately and has the endpoint and method as input parameters.
There was a problem hiding this comment.
Sorry, I misunderstand when you said to write the logs in add_route method. I tried this without using a for loop, but it shows one route when running the base_routes.py file. What I'm trying to do is that it shows all the routes in the list of routes.
if len(routes) > 2:
logger.info(f"Logging endpoint: method={routes[0][0]}, route={routes[0][1]}")
There was a problem hiding this comment.
My bad. Sorry, yes I misunderstand.
logger.info(f"Logging endpoint: method={route_type}, route={endpoint}")
Remove `for loop` from `add_route` method to log routes
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@carlosm27 maybe try this |
Hey @sansyrox I tried this:
|
|
@carlosm27 , the issue is not with your code but adding that is breaking something else. I can have a look tomorrow 😄 |
|
@carlosm27 , I understand what you mean. Our abstraction is not the best right now. We can merge your PR and address that later. Thank you for the great work! 😄 |
sansyrox
changed the title
Hey @sansyrox thank you for your review and feedback. |
Description
This PR is addressing the feature request of adding logs to the endpoints when the app starts.
This PR fixes #599