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

Read over Python 3.12's Logging Guide and see if there's anything else we could be leveraging/configuring to improve how logging is structured throughout our applications #1272

Merged
merged 10 commits into from
Sep 11, 2024

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Aug 15, 2024

Description

This ticket consisted of reading the best practices and then changing our code to add 'excInfo=True' to most of our calls to current_app.logger.error(). By default, error() (unlike exception()) doesn't print a stack trace, but usually we want it to. Then we needed to remove some of the raise Exception as e statements where we passed e as a parameter to the logger (since we would be getting the complete stack trace).

Security Considerations

N/A

@terrazoon terrazoon marked this pull request as draft August 15, 2024 17:31
@terrazoon terrazoon changed the title initial Read over Python 3.12's Logging Guide and see if there's anything else we could be leveraging/configuring to improve how logging is structured throughout our applications Aug 16, 2024
@terrazoon terrazoon self-assigned this Aug 16, 2024
@terrazoon terrazoon requested review from A-Shumway42 and removed request for ccostino August 20, 2024 20:55
@terrazoon terrazoon marked this pull request as ready for review August 22, 2024 15:34
@ccostino
Copy link
Contributor

Looks like this needs another merge from main, @terrazoon; could you also please update the PR description and details? Thank you! 🙂

@ccostino
Copy link
Contributor

D'oh, style check fail; also PR still needs a proper description! 🙂

Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Awesome @terrazoon, thank you for this cleanup! This will help improve logging going forward, and nice catch with several of the older debug statements or other things that should not be logged as well!

Copy link
Contributor

@xlorepdarkhelm xlorepdarkhelm left a comment

Choose a reason for hiding this comment

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

See my notes.

app/__init__.py Outdated Show resolved Hide resolved
app/aws/s3.py Outdated Show resolved Hide resolved
tests/app/test_cronitor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@xlorepdarkhelm xlorepdarkhelm left a comment

Choose a reason for hiding this comment

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

LGTM

@terrazoon terrazoon merged commit a154d57 into main Sep 11, 2024
7 checks passed
@terrazoon terrazoon deleted the notify-api-1271 branch September 11, 2024 17:30
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