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

avoid n+1 queries on bulk GET /system endpoint #4120

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Sep 19, 2023

Closes #4028

Description Of Changes

Narrow down what we return on the bulk GET /system endpoint to not have relationship fields that can lead to n+1 query problems - in this case, it was specifically the connection_configs property/relationship. See issue for more context.

Code Changes

  • define a narrower BasicSystemResponse model without any relationship-backed fields, used for the bulk GET endpoint.

Steps to Confirm

  • ran locally with nox -s "fides_env(test)" with 4 systems, i added an additional integration onto one of those systems (i think two systems come seeded with integrations already).
    • without the change, i was seeing the GET /system endpoint take ~1s to respond
    • with the change, that was down to <200ms
  • confirmed that connection_configs was no longer being returned on the bulk GET /system endpoint response records
  • confirmed that all admin UI around this still seemed to be functional 👍

Pre-Merge Checklist

Copy link
Contributor

@galvana galvana left a comment

Choose a reason for hiding this comment

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

The system management page can still access the fields it needs, looks good to me!

@cypress
Copy link

cypress bot commented Sep 19, 2023

Passing run #4235 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 7a13124 into f7c0a75...
Project: fides Commit: d87fe4dfb8 ℹ️
Status: Passed Duration: 01:04 💡
Started: Sep 19, 2023 9:11 PM Ended: Sep 19, 2023 9:13 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (f7c0a75) 87.39% compared to head (7a13124) 87.39%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4120      +/-   ##
==========================================
- Coverage   87.39%   87.39%   -0.01%     
==========================================
  Files         320      320              
  Lines       19706    19707       +1     
  Branches     2531     2531              
==========================================
  Hits        17222    17222              
- Misses       2042     2043       +1     
  Partials      442      442              
Files Changed Coverage Δ
src/fides/api/api/v1/endpoints/system.py 90.00% <100.00%> (ø)
src/fides/api/schemas/system.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

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.

nice one

@adamsachs adamsachs marked this pull request as ready for review September 19, 2023 22:35
@adamsachs adamsachs merged commit b9efa45 into main Sep 19, 2023
@adamsachs adamsachs deleted the asachs/fides-4028-improve-system-get-performance branch September 19, 2023 22:37
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.

improve GET /system API performance with many integrations
3 participants