Skip to content
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

Change logging exception to logging error, at least for HTTPErrors #835

Conversation

sfisher
Copy link
Contributor

@sfisher sfisher commented Feb 14, 2025

Hi Dave,

Thanks for your feedback. You can see I've changed IncompleteRead, HTTPError and the exception branch to use log.error() instead of log.exception() and some use the simple_error() method which just logs the exception name, line number and optionally a status code if it's an HTTPError.

Does this seem reasonable to you?

I tested out the simple_error() method manually and it all seems to work ok.

Let me know what you think and I'll maybe try it on dev.

Copy link
Contributor

@datadavev datadavev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Consider using the first trace entry as per comment. Also consider if this even needs to be logged? Not sure how much extra processing grabbing the trace is.

@sfisher sfisher marked this pull request as draft February 14, 2025 20:23
…es-for-common-httperrors-400-ish-and-500-ish-status-codes-and-bad-certificates
…he way down the stack) and adding test_logging.py management command.
@sfisher sfisher marked this pull request as ready for review February 22, 2025 00:13
@sfisher sfisher changed the base branch from main to develop February 22, 2025 00:14
@sfisher
Copy link
Contributor Author

sfisher commented Feb 22, 2025

Hi @datadavev

I'd talked with Adam about changes for now and he said we should proceed with the first step of reducing exception to error in our code. We'll talk with Ashley in our meeting about the best way to see the difference on stage since I think most daemons don't run on stage and he'd like to see before/after snapshots before we proceed. I suppose we can just start some like the link checker manually.

Let me know if you see problems with the changes here since I just incorporated them here. I can move to a different PR if you'd like me to, instead.

@sfisher
Copy link
Contributor Author

sfisher commented Mar 3, 2025

I tested the link checker on dev and the logging is significantly reduced. (See the issue for a few notes.) I think I'll deploy to stage and I think we should go ahead with this to production later this week unless there is some real reason not to.

@jsjiang
Copy link
Contributor

jsjiang commented Mar 5, 2025

@sfisher Hi Scott, Changes look good. Please go ahead merge and deploy on prod.
Thank you

Jing

@sfisher sfisher merged commit e49fb7a into develop Mar 5, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants