Eng 319 verify update all logging for dsr activity logging#6191
Eng 319 verify update all logging for dsr activity logging#6191
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (72.22%) 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 #6191 +/- ##
==========================================
- Coverage 87.08% 87.07% -0.01%
==========================================
Files 423 423
Lines 26315 26321 +6
Branches 2869 2870 +1
==========================================
+ Hits 22916 22919 +3
- Misses 2775 2776 +1
- Partials 624 626 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
erosselli
left a comment
erosselli
left a comment
There was a problem hiding this comment.
Approving with some questions. Don't forget the changelog entry
| attachment_uploaded = "attachment_uploaded" | ||
| attachment_deleted = "attachment_deleted" | ||
| attachment_retrieved = "attachment_retrieved" |
There was a problem hiding this comment.
are these not used anymore?
There was a problem hiding this comment.
I added them and then learned more about the audit log so they were never really used.
| logger.bind( | ||
| privacy_request_id=privacy_request.id, | ||
| time_taken=time.time() - start_time, |
There was a problem hiding this comment.
I think there might be a decorator you can use to bind the privacy request id all throughout the function?
There was a problem hiding this comment.
Updated with log_context decorator! Thanks :)
| connection_key=None, | ||
| dataset_name="Access Package Upload", | ||
| collection_name=None, | ||
| message=f"Access Package Upload successful for privacy request: {privacy_request.id}", |
There was a problem hiding this comment.
do we need the id for the privacy request in the message, if the execution log is already linked to the privacy request? thinking about the UX of reading these execution logs in the UI
There was a problem hiding this comment.
Updated - much cleaner now :)
| # Only mark as complete if not in error state | ||
| if privacy_request.status != PrivacyRequestStatus.error: |
There was a problem hiding this comment.
I'm surprised we didn't have this check before ?
There was a problem hiding this comment.
I was too! I purposefully made the upload error for the test steps above it errored and then completed.
JadeCara
deleted the
ENG-319-verify-update-all-logging-for-dsr-activity-logging
branch
fides
|
||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 01m 00s |
| Commit |
|
| Committer | JadeWibbels |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
5
|
| View all changes introduced in this branch ↗︎ | |
2 participants
Issue 319
Description Of Changes
Adds Execution Logs for uploading the access packages as well as some bind logs.
Additional changes for this ticket in fidesplus PR 2211
Code Changes
StorageUploadErrorand include the specific error messagesSteps to Confirm
a. with an error - Upload error appears in the UI and in logging. To create this error I went and changed the last two digits of the storage config default using the
/api/v1/storage/defaultPUT methodb. with success - Upload appears in the UI and in logging
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works