Skip to content

Lj 462 Reduce Logging for Disabled Cache on SQLAlchemy#6089

Merged
Vagoasdf merged 7 commits intomainfrom
LJ-462-Reduce-PII-log-redshift
Apr 28, 2025
Merged

Lj 462 Reduce Logging for Disabled Cache on SQLAlchemy#6089
Vagoasdf merged 7 commits intomainfrom
LJ-462-Reduce-PII-log-redshift

Conversation

@Vagoasdf
Copy link
Copy Markdown
Contributor

Closes LJ#462

Description Of Changes

We were having trouble with unwanted SQLAlchemy log message related to caching issues with third-party dialects like Redshift.

Code Changes

  • Implemented a SQLAlchemyGeneratedFilter in sqlalchemy_filter.py to filter out specific unwanted log messages from the SQLAlchemy engine. (Thanks @galvana )
  • Added a new test suite TestSQLAlchemyLogger in test_logger.py to ensure that the SQLAlchemyGeneratedFilter correctly filters out unwanted log messages.

Steps to Confirm

  1. Run the test suite in test_logger.py to ensure that the SQLAlchemyGeneratedFilter is correctly filtering out the specified log messages.

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Apr 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2025 9:22pm
fides-privacy-center ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2025 9:22pm

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.93%. Comparing base (7a5def5) to head (6126e5a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/util/sqlalchemy_filter.py 91.66% 0 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (93.75%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6089   +/-   ##
=======================================
  Coverage   86.92%   86.93%           
=======================================
  Files         420      421    +1     
  Lines       26027    26043   +16     
  Branches     2833     2836    +3     
=======================================
+ Hits        22625    22640   +15     
  Misses       2787     2787           
- Partials      615      616    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +16 to +18
"cached since",
"caching disabled",
"does not support caching",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How certain are we that these are the only caching related substrings we need to catch? Are there any other substrings related to PII we might want to guard against (that may or may not be happening now?)

Copy link
Copy Markdown
Contributor Author

@Vagoasdf Vagoasdf Apr 25, 2025

Choose a reason for hiding this comment

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

The one we detected during this Issue was related to caching strategies for third party dialects. Docs over here: https://docs.sqlalchemy.org/en/14/core/connections.html#engine-thirdparty-caching point to the following structure for this error

For all third party dialects that don’t support this attribute, the logging for such a dialect will indicate dialect does not support caching.

cached since and caching disabled are failsafe nets for similar error that might contain PII, but haven't showed on the logs yet

Copy link
Copy Markdown
Contributor

@JadeCara JadeCara left a comment

Choose a reason for hiding this comment

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

The tests work for all test cases. I had one question about if we are filtering strongly enough, but this will definitely solve the immediate problem. Approving.

@Vagoasdf Vagoasdf merged commit 5b0b7de into main Apr 28, 2025
39 of 40 checks passed
@Vagoasdf Vagoasdf deleted the LJ-462-Reduce-PII-log-redshift branch April 28, 2025 14:04
@cypress
Copy link
Copy Markdown

cypress Bot commented Apr 28, 2025

fides    Run #12881

Run Properties:  status check passed Passed #12881  •  git commit 5b0b7de260: Lj 462 Reduce Logging for Disabled Cache on SQLAlchemy (#6089)
Project fides
Branch Review main
Run status status check passed Passed #12881
Run duration 00m 54s
Commit git commit 5b0b7de260: Lj 462 Reduce Logging for Disabled Cache on SQLAlchemy (#6089)
Committer Bruno Gutierrez Rios
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

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.

2 participants