Skip to content

ensure we show all frames in our profiler output#6153

Merged
adamsachs merged 3 commits intomainfrom
asachs/profiler-show-all-output
May 16, 2025
Merged

ensure we show all frames in our profiler output#6153
adamsachs merged 3 commits intomainfrom
asachs/profiler-show-all-output

Conversation

@adamsachs
Copy link
Copy Markdown
Contributor

@adamsachs adamsachs commented May 16, 2025

Description Of Changes

We're not showing all frames in our endpoint profiler output, which in practice seems to make that output pretty useless. This switch has the output show all frames - which can definitely be a bit verbose. But what's the point of profiling if it's not comprehensive? :)

Note: this was used to help find and confirm a potentially important performance improvement to our GET /privacy experience endpoint

Code Changes

  • set show_all=True in our profiler output

Steps to Confirm

  1. send a profile-request: true header in any API calls against a fides webserver
  2. locally with this change, i get an output that starts like this:

  _     ._   __/__   _ _  _  _ _/_   Recorded: 10:38:10  Samples:  67
 /_//_/// /_\ / //_// / //_'/ //     Duration: 0.921     CPU time: 0.703
/   _/                      v4.5.1

Program: /usr/local/bin/uvicorn --host 0.0.0.0 --port 8080 --reload --reload-dir src --reload-dir data --reload-dir /fidesplus/ethyca-fides/src --reload-include=*.yml fidesplus.main:fidesplus_app

0.920 run  asyncio/runners.py:8
|- 0.067 coro  starlette/middleware/base.py:146
|  |- 0.009 GZipMiddleware.__call__  starlette/middleware/gzip.py:17
|  |  `- 0.009 dict.__call__  starlette/middleware/base.py:101
|  |     `- 0.009 SlowAPIMiddleware.dispatch  slowapi/middleware.py:117
|  |        |- 0.007 _find_route_handler  slowapi/middleware.py:18
|  |        |  |- 0.002 APIRoute.matches  fastapi/routing.py:538
|  |        |  |  `- 0.002 APIRoute.matches  starlette/routing.py:255
|  |        |  |     `- 0.002 get_route_path  starlette/_utils.py:96
|  |        |  |        |- 0.001 [self]  starlette/_utils.py
|  |        |  |        `- 0.001 sub  re.py:202
|  |        |  |- 0.002 [self]  slowapi/middleware.py
|  |        |  `- 0.003 APIRoute.matches  fastapi/routing.py:538
...

and includes useful information that shows our own function calls:

`- 0.828 wrapper  fidesplus/api/endpoint_cache.py:119
   |                                   |- 0.826 privacy_experience_list  fidesplus/api/routes/privacy_experiences.py:393
   |                                   |  |- 0.023 PrivacyExperience.component  fides/api/models/privacy_experience.py:603
   |                                   |  |  `- 0.023 InstrumentedAttribute.__get__  sqlalchemy/orm/attributes.py:466
   |                                   |  |     `- 0.023 ScalarObjectAttributeImpl.get  sqlalchemy/orm/attributes.py:908
   |                                   |  |        `- 0.023 ScalarObjectAttributeImpl._fire_loader_callables  sqlalchemy/orm/attributes.py:951
  1. on plus-rc, which doesn't have this change, the same profile output is basically useless:
  _     ._   __/__   _ _  _  _ _/_   Recorded: 10:42:00  Samples:  84
 /_//_/// /_\ / //_// / //_'/ //     Duration: 3.286     CPU time: 1.587
/   _/                      v4.5.1

Program: /usr/local/bin/fidesplus

3.286 run  asyncio/runners.py:8
|- 0.258 privacy_experience_list  fidesplus/api/routes/privacy_experiences.py:393
|     [21 frames hidden]  fidesplus, starlette, anyio, fides, s...
|- 0.154 privacy_experience_list  fidesplus/api/routes/privacy_experiences.py:393
|     [5 frames hidden]  fidesplus, starlette, anyio
`- 2.848 privacy_experience_list  fidesplus/api/routes/privacy_experiences.py:393
      [10 frames hidden]  fidesplus, starlette, anyio, fastapi
         2.750 run_sync_in_worker_thread  anyio/_backends/_asyncio.py:834
         `- 2.750 [await]  anyio/_backends/_asyncio.py

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 May 16, 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 May 16, 2025 2:58pm
fides-privacy-center ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2025 2:58pm

@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2025

Codecov Report

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

Project coverage is 86.92%. Comparing base (bb1f05e) to head (892d1c6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/main.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6153   +/-   ##
=======================================
  Coverage   86.92%   86.92%           
=======================================
  Files         423      423           
  Lines       26150    26150           
  Branches     2840     2840           
=======================================
  Hits        22730    22730           
  Misses       2800     2800           
  Partials      620      620           

☔ 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.

Copy link
Copy Markdown
Contributor

@erosselli erosselli 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 this! I think we can add a Changelog entry under the Developer Experience section :)

@adamsachs adamsachs merged commit a57a57c into main May 16, 2025
17 checks passed
@adamsachs adamsachs deleted the asachs/profiler-show-all-output branch May 16, 2025 14:59
@cypress
Copy link
Copy Markdown

cypress Bot commented May 16, 2025

fides    Run #12930

Run Properties:  status check passed Passed #12930  •  git commit a57a57c757: ensure we show all frames in our profiler output (#6153)
Project fides
Branch Review main
Run status status check passed Passed #12930
Run duration 00m 50s
Commit git commit a57a57c757: ensure we show all frames in our profiler output (#6153)
Committer Adam Sachs
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
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