-
Notifications
You must be signed in to change notification settings - Fork 73
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
PROD-2644 Improve logging and error visibility for TraversalErrors #5263
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides Run #9930
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5263/merge
|
Run status |
Passed #9930
|
Run duration | 00m 36s |
Commit |
3a60219df3 ℹ️: Merge 343fe9dca2be49c04af424b64392c85c1769384c into aa37211d487f7ec3a5b38b58f923...
|
Committer | erosselli |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
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.
@erosselli good start, also could you add some tests please?
41c2917
to
c5c0c08
Compare
c5c0c08
to
76fe988
Compare
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.
great improvement, traversal errors have been something that has been buried for a long time - I think test coverage is good enough here. The "edges" case is not going to be hit anyway. Just a quick request to dry some of the code up -
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.
Looks good, approved with passing tests
fides Run #9937
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #9937
|
Run duration | 00m 36s |
Commit |
07e53f4047: PROD-2644 Improve logging and error visibility for TraversalErrors (#5263)
|
Committer | erosselli |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Closes PROD-2644
Description Of Changes
When a
TraversalError
is raised as part of running an access or erasure request the logged error does not include the privacy request id, making it hard to find, and the UI shows the request in an error state but provides no information on what the error actually was. This PR adds a log that includes the privacy request id, and also creates anExecutionLog
instance so the error can be surfaced in the UI.Code Changes
UnreachableNodeError
andUnreachableEdgeError
that inherit fromTraversalError
, and throw those instead when we have an unreachable node or edge, respectively.TraversalErrors
in bothrun_access_request
andrun_access_request_deprecated
, log the error including the id of the privacy request , create the necessaryExecutionLog
instances, and raise the error again.with_server_health_check
decorator from both theworker
andstatus
commands, they were mistakenly added in a recent PR.Steps to Confirm
Setup
nox -s dev -- postgres
and
prod-2644-example
) from theDataset
dropdown. You should have two integrations, each linked to a different datasetDSR 2.0
error
status, and there should be an Error log in theActivity Timeline
section (see screenshots below)You should also see the log in the fides container, e.g
2024-09-06 13:57:08 2024-09-06 16:57:08.400 | ERROR | fides.api.task.deprecated_graph_task:run_access_request_deprecated:116 - TraversalError encountered for privacy request pri_83ace5f2-e4f7-4c2b-9ce3-fb989cddb214. Error: Some nodes were not reachable: prod-2644-example:address
DSR 3.0
fides.toml
file, and replace the following two lines:[execution]
section, changeuse_dsr_3_0=false
touse_dsr_3_0=true
[celery]
section, changetask_always_eager = true
totask_always_eager = false
nox -s dev -- shell postgres worker
Other error cases
When the error is not for unreachable nodes, but rather a more generic
TraversalError
(e.g there's a cycle) then the error's shown a bit differently in the UIPre-Merge Checklist
CHANGELOG.md
main
downgrade()
migration is correct and works