Reduce Idle Connections from Health Checks [#1102]

[pattisdr]

Purpose

The health check endpoints are opening up too many connections against the fidesops application database. Shoutout to @RobertKeyser and @sanders41 for spotting.

Changes

• Stop creating a new engine as part of the process of running a health check.
• Create an engine to be used across the application, and pass this into get_db_session when we go to get a single connection, instead of creating a new engine. (Note we are currently using the default pool_size and max_overflow parameters. This is something that may need to be configurable in the future). This is used for all our API endpoints.
  • Note that our celery processes intentionally create their own separate engines for managing connections for running privacy requests.

Background

The typical usage of create_engine() is once per particular database URL, held globally for the lifetime of a single application process. A single Engine manages many individual DBAPI connections on behalf of the process and is intended to be called upon in a concurrent fashion. The Engine is not synonymous to the DBAPI connect function, which represents just one connection resource - the Engine is most efficient when created just once at the module level of an application, not per-object or per-function call.

Checklist

• Update CHANGELOG.md file
  • Merge in main so the most recent CHANGELOG.md file is being appended to
  • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
  • Add a link to this PR at the end of the description with the PR number as the text. example: #1
• Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
• If docs updated (select one):
  • 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)
• Good unit test/integration test coverage
• This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
• The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #1102

[pattisdr]

thanks for merging @sanders41 ⭐

[seanpreston]

Good solution. Is there a limit to how many connections one engine can make?

[pattisdr]

yes, I noted this in the description, we're currently using the defaults, so I believe the pool_size is 5, and there's a default max_overflow of 10, so you take the sum - 15. This is something that we'll probably want to make configurable at some point.