Skip to content

Conversation

@erosselli
Copy link
Contributor

@erosselli erosselli commented Nov 14, 2025

No ticket

Description Of Changes

Adds a new endpoint POST api/v1/admin/heap-dump (for owner roles only) to allow manually executing the exising _capture_heap_dump function.

Code Changes

Steps to Confirm

  1. Set FIDES__EXECUTION__MEMORY_WATCHDOG_ENABLED=true
  2. Make a request to POST api/v1/admin/heap-dump
  3. Check the server logs, you should see memory usage info

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
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • 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

vercel bot commented Nov 14, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Nov 14, 2025 3:33pm
fides-privacy-center Ignored Ignored Nov 14, 2025 3:33pm

@erosselli erosselli marked this pull request as ready for review November 14, 2025 15:37
@erosselli erosselli requested a review from a team as a code owner November 14, 2025 15:37
@erosselli erosselli requested review from adamsachs and galvana and removed request for a team November 14, 2025 15:37
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 14, 2025

Greptile Overview

Greptile Summary

This PR adds a new admin endpoint POST /admin/heap-dump that allows owner-role users to manually trigger memory diagnostics. The implementation properly restricts access to owners only through the new HEAP_DUMP_EXEC scope, and includes comprehensive test coverage for both enabled/disabled states and logging verification.

Key changes:

  • New endpoint at /admin/heap-dump with proper OAuth scope-based authorization
  • New HEAP_DUMP_EXEC scope restricted to owner roles (not available to contributors)
  • Reusable _capture_heap_dump() function updated to work for both automatic threshold triggers and manual API calls
  • Complete test coverage including configuration state and log output verification
  • URL pattern corrected from underscore to dash notation (heap-dump vs heap_dump)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is straightforward, well-tested, and properly secured with owner-only access controls. The endpoint delegates to existing, well-tested diagnostic functionality. All code changes are additive with no breaking changes.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/fides/api/api/v1/endpoints/admin.py 5/5 Added new POST /admin/heap-dump endpoint with proper scope-based authorization for owner roles
src/fides/api/util/memory_watchdog.py 5/5 Changed heap dump header from "THRESHOLD EXCEEDED" to generic "MEMORY DUMP" for manual triggers
tests/ctl/api/test_admin.py 5/5 Added comprehensive tests for heap dump endpoint covering enabled, disabled, and logging scenarios

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

6 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

the addition of an API invocation here seems reasonable to help make this more accessible 👍

report_lines = [
"", # Leading newline for visual separation
"=" * 80,
"MEMORY DUMP - THRESHOLD EXCEEDED",
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch/tweak :)

@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.33%. Comparing base (515cf0d) to head (c12c3fb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6973   +/-   ##
=======================================
  Coverage   87.32%   87.33%           
=======================================
  Files         525      525           
  Lines       34445    34456   +11     
  Branches     3965     3966    +1     
=======================================
+ Hits        30080    30091   +11     
  Misses       3501     3501           
  Partials      864      864           

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

@erosselli erosselli added this pull request to the merge queue Nov 14, 2025
Merged via the queue into main with commit c14d295 Nov 14, 2025
69 checks passed
@erosselli erosselli deleted the erosselli/api-for-heap-dump branch November 14, 2025 16:46
adamsachs pushed a commit that referenced this pull request Nov 14, 2025
@greptile-apps greptile-apps bot mentioned this pull request Nov 14, 2025
18 tasks
Kelsey-Ethyca pushed a commit that referenced this pull request Nov 18, 2025
jjdaurora pushed a commit that referenced this pull request Dec 5, 2025
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.

3 participants