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

Update all logging references to use loguru #2031

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Dec 13, 2022

Update to use loguru instead of built-in logging

Closes #1842

Code Changes

  • Update all logging references to use loguru
  • Switch string formatting to use {}-style loguru formatting for lazy evaluation
  • Update Pii class to work well with loguru lazy formatting
  • Include a new, proper logging.log_pii config property

Steps to Confirm

  • run your server
  • notice (hopefully) all of our logs are back

Follow-up issues

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Most of this is pretty straightforward find-replace.

The only change that required some thought was the Pii updates, since before we'd been creating our own LogRecordFactory - there may have been a straightforward way of mimicking this functionality in loguru, but it wasn't immediately apparent to me, so i decided to switch up the implementation a bit. What I arrived at should be functionally equivalent, and I think it's quite a straightforward implementation.

I also decided to add in a proper logging.log_pii app config property, since we were just relying on a environment var beforehand outside of the general config framework. perhaps there was a reason for having it done that way? i couldn't see one, so i figured it made more sense to properly account for it as an app config property/setting.

A couple of notes:

  • I removed the get_task_logger reference in request_runner_service.py because I couldn't find a quick way to integrate that with loguru. this seems to be some functionality we may want, so i think it probably deserves some further attention. but i didn't want to hold up this PR on that, since we need to get overall logging back ASAP. (note that the logging in this file is not coming through at all on the current main, so this still represents an improvement). I can create a follow up ticket to handle this if we think it's necessary.
  • I did not update our logging references in our tests. I wasn't sure if this was in scope - I could see it going either way. regardless, it's not as high priority as the app code updates, and we want to get this out ASAP, so I decided to hold off. I can also create a follow up ticket for those updates if we want.

@adamsachs adamsachs force-pushed the 1842-update-all-logger-references-to-use-loguru-to-stop-logs-getting-suppressed branch from 6f8888c to b720512 Compare December 13, 2022 04:00
@adamsachs adamsachs marked this pull request as ready for review December 13, 2022 13:26
Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

I tried making some privacy requests with errors because I know this is an area we were not getting logs before, and I am seeing them now so this seems to have worked.

It would be nice if we could add a linting rule to prevent the use of the default logger so we don't accidentally get back in the same situation. I looked for a pylint plugin that might be able to do it, but didn't find anything. It was also not easy to search for because the results were assuming trying to solve an import error rather than preventing an import so I could just be missing one. It's a "nice to have" and not mandatory.

@pattisdr
Copy link
Contributor

Thanks @adamsachs will review shortly!

@pattisdr
Copy link
Contributor

@adamsachs just starting to review, but I'm still seeing a lot of instances of where the old built-in logging is still being used on this branch

@adamsachs
Copy link
Contributor Author

adamsachs commented Dec 13, 2022

@adamsachs just starting to review, but I'm still seeing a lot of instances of where the old built-in logging is still being used on this branch

whoops, yeah i think i missed a few. the uses in tests/ and scripts/ are expected, fwiw. but yeah, definitely some uses in src/ that somehow slipped by me! i'll address shortly, sorry bout that.

update -- just realized my vscode global search was only searching open files 🤦

src/fides/api/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

thanks for taking this on Adam -

Kelsey-Ethyca and others added 3 commits December 13, 2022 12:47
Switch to use {} loguru formatting
Update Pii class to work with loguru lazy formatting
Include a new, proper log_pii config var
@adamsachs adamsachs force-pushed the 1842-update-all-logger-references-to-use-loguru-to-stop-logs-getting-suppressed branch from e6a7cc4 to 925e8cc Compare December 13, 2022 17:50
@ThomasLaPiana
Copy link
Contributor

I think the big fideslib merge caused these issues, I'm going to take a look

@ThomasLaPiana
Copy link
Contributor

@adamsachs I'm overnighting a cape to you, thank you for tackling this!

@adamsachs
Copy link
Contributor Author

thanks proactively pushing this forward with the merge @ThomasLaPiana! i think we're just waiting on a re-review from @pattisdr before we can merge this.

@pattisdr
Copy link
Contributor

looking now!

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

thanks so much for taking this on @adamsachs! I added a last commit I was going through your changes, let me know if that looks ok -

src/fides/api/ops/util/logger.py Show resolved Hide resolved
src/fides/api/ops/util/logger.py Show resolved Hide resolved
src/fides/api/ctl/routes/health.py Show resolved Hide resolved
@adamsachs
Copy link
Contributor Author

adamsachs commented Dec 14, 2022

thanks so much for taking this on @adamsachs! I added a last commit I was going through your changes, let me know if that looks ok -

more than ok @pattisdr! thank you for that last bit of cleanup!

i think we're good to go now :)

@adamsachs adamsachs merged commit 29ce28b into main Dec 14, 2022
@adamsachs adamsachs deleted the 1842-update-all-logger-references-to-use-loguru-to-stop-logs-getting-suppressed branch December 14, 2022 16:09
RobertKeyser added a commit to ethyca/fides-helm that referenced this pull request Jan 24, 2023
this commit sets the correct name of the Fides log pii env var (as changed in ethyca/fides#2031) and changes the default logging level from debug --> info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update all logger. references to use loguru to stop logs getting suppressed
5 participants