Add logging #40
Add logging #40
Conversation
AbooMinister25
requested review from
shenanigansd,
jonathan-d-zhang and
Robin5605
There was a problem hiding this comment.
@AbooMinister25 mind getting this to use https://github.com/vipyrsec/dragonfly-logging-config? Thanks!
| @@ -39,16 +42,17 @@ def fetch_packages(*, pypi_client: PyPIServices) -> list[tuple[str, str]]: | |||
| def load_packages(packages: list[tuple[str, str]], *, http_client: Client, access_token: str) -> None: | |||
| """Load all of the given packages into the Dragonfly API, using the given HTTP session and access token.""" | |||
| payload = [{"name": name, "version": version} for name, version in packages] | |||
| logger.info("Loading packages: %s", payload) | |||
There was a problem hiding this comment.
Considering this will spit out 100 name/value pairs this should probably be debug
| logger.info("Loading packages: %s", payload) | |
| logger.debug("Loading packages: %s", payload) |
|
|
||
|
|
||
| def main(*, http_client: Client, pypi_client: PyPIServices) -> None: | ||
| """Run the loader.""" | ||
| access_token = get_access_token(http_client=http_client) | ||
|
|
||
| packages = fetch_packages(pypi_client=pypi_client) | ||
| logger.info("Fetched packages: %s", packages) |
There was a problem hiding this comment.
| logger.info("Fetched packages: %s", packages) |
I don't think we should log this because we log the same thing later on when we're loading the packages. We end up with 2 x 100 packages logged which is not ideal. We could also just do something like, fetched 100 packages maybe. doesn't really matter, up to you
| logger.debug("%s %s - Status %s", request.method, request.url, response.status_code) | ||
|
|
||
| try: | ||
| response.raise_for_status() | ||
| except HTTPError: | ||
| logger.exception("Response had a non 2xx status code") |
There was a problem hiding this comment.
I like the idea of the log level being based on the status code all in one log message. But I don't know if I like setting the level = logger.debug stuff that much, though. perhaps the logging library has a way to call a generic log method and pass in the level as a parameter? That would be much cleaner.
I just want to avoid the case where:
<blah blah>
GET /SOME URL - Status 500
<bunch of random logs>
EXCEPTION: Response had a non 2xx status code
<more blah blah>
wherein you need to go looking for which response had a non 2xx status code.
| logger.debug("%s %s - Status %s", request.method, request.url, response.status_code) | |
| try: | |
| response.raise_for_status() | |
| except HTTPError: | |
| logger.exception("Response had a non 2xx status code") | |
| try: | |
| response.raise_for_status() | |
| level = logger.debug | |
| except HTTPError: | |
| level = logger.exception | |
| finally: | |
| level("%s %s - Status %s", request.method, request.url, response.status_code) |
No description provided.