-
Notifications
You must be signed in to change notification settings - Fork 84
Misc troubleshooting improvements #6916
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
Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/two-factor-authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR adds troubleshooting improvements for stuck privacy requests and memory management:
Key Changes:
- Added new
/privacy-request/administrate/cancelendpoint to allow bulk cancellation of privacy requests in non-terminal states - Relaxed resubmission restrictions to allow
pendingstatus requests to be resubmitted (previously only non-complete/non-pending could be resubmitted) - Enhanced memory watchdog with heap dump capture functionality using
objgraphto help diagnose memory leaks when thresholds are exceeded - Added support for arbitrary Celery configuration via
FIDES__CELERY__*environment variables beyond the explicitly defined fields
How it works:
The cancel endpoint follows the same pattern as approve/deny endpoints, using bulk operations with proper validation. The heap dump feature captures object type counts, GC stats, and uncollectable objects when memory limits are exceeded, logging them as a single comprehensive report. The Celery env var merging uses JSON parsing to handle type conversion for booleans, integers, and other JSON-compatible values.
Confidence Score: 4/5
- This PR is safe to merge with minor attention to the Celery config parsing logic
- The PR adds valuable troubleshooting features with comprehensive test coverage. One logical issue was found in
celery_settings.py:56where lowercasing values before JSON parsing could break case-sensitive string values (e.g., URLs with uppercase). The exception handling catches this, but it's not the intended behavior. All other changes are well-tested and follow existing patterns. - Pay close attention to
src/fides/config/celery_settings.py- the JSON parsing logic should not lowercase values before parsing
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/fides/api/api/v1/endpoints/privacy_request_endpoints.py | 5/5 | Added cancel endpoint and relaxed resubmit restrictions for pending requests |
| src/fides/api/util/memory_watchdog.py | 5/5 | Added comprehensive heap dump functionality for memory leak diagnosis |
| src/fides/config/celery_settings.py | 3/5 | Added merge function for arbitrary Celery env vars, but has case-sensitivity issue with JSON parsing |
| src/fides/service/privacy_request/privacy_request_service.py | 5/5 | Added cancel_privacy_requests method and relaxed resubmit restrictions |
12 files reviewed, 1 comment
src/fides/config/celery_settings.py
Outdated
| # Use JSON parsing to handle type conversion properly | ||
| # This handles booleans (true/false), integers, floats, etc. | ||
| try: | ||
| celery_dict[stripped_key] = json.loads(value.lower()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: lowercasing value before JSON parsing breaks case-sensitive string values
For example: FIDES__CELERY__BROKER_URL=Redis://myhost would become redis://myhost after lowercasing, which might cause unexpected behavior. Consider only lowercasing for known boolean strings:
| celery_dict[stripped_key] = json.loads(value.lower()) | |
| # Try parsing as-is first for case-sensitive values | |
| celery_dict[stripped_key] = json.loads(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how correct this is, @galvana can you double-check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this is valid, I fixed it
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (81.48%) 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 #6916 +/- ##
==========================================
- Coverage 87.29% 87.27% -0.02%
==========================================
Files 523 523
Lines 34172 34251 +79
Branches 3932 3943 +11
==========================================
+ Hits 29829 29892 +63
- Misses 3487 3499 +12
- Partials 856 860 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
erosselli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a changelog entry. Approving with some questions/comments
src/fides/config/celery_settings.py
Outdated
| # Use JSON parsing to handle type conversion properly | ||
| # This handles booleans (true/false), integers, floats, etc. | ||
| try: | ||
| celery_dict[stripped_key] = json.loads(value.lower()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how correct this is, @galvana can you double-check?
Ticket ENG-1860
Description Of Changes
Misc improvements to help troubleshoot and deal with stuck privacy requests.
Code Changes
PATCH /privacy-request/administrate/cancelFIDES__CELERY__prefixPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works