Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging #363

pattisdr · 2022-04-05T20:20:41Z

Purpose

It sounds like most devs have to do some adjusting of their local environment during development to help with debugging. We intentionally suppress a lot of details that would help with troubleshooting.

In the shell, do export FIDESOPS__DEV_MODE="TRUE". Run make server and then run a traversal that has saas requests via postman.

This gives you:

No retries.
Errors in traversal are logged with traceback.
SaaS request details are logged, including path, headers, and request body.
Response is logged from saas endpoints
Side effect, this adds hot reloading too. Maybe hot reload should be a separate variable.

Changes

Up for discussion - this is something that is useful for me during debugging, but others may not want this behavior. We might add a follow-up ticket for logging more database details too, this is just because we've been focused on saas details lately.

Also effects FIDESOPS__LOG_PII, FIDESOPS__HOT_RELOAD, and FIDESOPS__CONFIG_PATH

Checklist

Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram)
Good unit test/integration test coverage
This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #

- No retries - Exceptions are not caught; entire traceback is logged. - Details of saas requests are logged - path, headers, body.

eastandwestwind

This will make iterating on SaaS connectors so much easier, very excited to get this one in!

docker-compose.yml

src/fidesops/service/connectors/saas_connector.py

eastandwestwind · 2022-04-05T21:28:30Z

Re: hot reloading, I'm all for it. I don't foresee a scenario where wouldn't want it, unless anyone sees differently?

pattisdr · 2022-04-05T21:34:19Z

well hot reloading can be a little annoying if you're running multiple privacy requests at a time and uploading the results to a local folder. The sar results themselves trigger a reloading, which can prevent the other privacy requests from executing. This came up when we were testing too many connections getting opened up in fidesops.

To be clear, I'm not introducing hot reloading, it's just that hot reloading already looks at the "DEV_MODE" variable. I'm proposing having hot reloading look at HOT_RELOAD and dev mode look at DEV_MODE.

src/fidesops/service/connectors/saas_connector.py

… to not get mixed up with other services.

…On Python 3 '{}'.format(b'abc') produces "b'abc'", not 'abc'; use '{!r}'.format(b'abc') if this is desired behavior

seanpreston · 2022-04-18T19:44:16Z

docs/fidesops/docs/guides/configuration_reference.md

-| `LOG_PII` | False | If this is set to "True", pii values will display unmasked in log output. This variable should always be set to "False" in production systems.|
-| `DEV_MODE` | False | If "True", the fidesops server will run with hot-reloading of files. This variable should always be set to "False" in production systems.|
+| `FIDESOPS_LOG_PII` | False | If this is set to "True", pii values will display unmasked in log output. This variable should always be set to "False" in production systems.
+| `FIDESOPS_HOT_RELOAD` | False | If "True", the fidesops server will run with hot-reloading of files. This variable should always be set to "False" in production systems.|


I'm pretty sure hot reloading is a well understood term, but it might be good to add a sentence explaining that just in case?

good suggestion

seanpreston · 2022-04-18T19:47:39Z

src/fidesops/core/config.py

+        f"Startup configuration: reloading = {hot_reloading}, dev_mode = {dev_mode}"
+    )
+    logger.warning(
+        f'Startup configuration: pii logging = {os.getenv("FIDESOPS_LOG_PII") == "True"}'


Should these only warn under certain circumstances? For instance if log pii, dev mode or hot reloading is true? In theory if those aren't true, there's nothing to warn about

I mean, up to you, I like seeing the "Startup configuration" regardless of whether it's true or false, we could change it to be a logger.info?

seanpreston · 2022-04-18T19:48:50Z

src/fidesops/service/connectors/saas_connector.py

-            raise ConnectionException(f"Operational Error connecting to '{self.key}'.")
+        except Exception as exc:  # pylint: disable=W0703
+            if config.dev_mode:  # pylint: disable=R1720
+                raise exc


We probably still want to log here which key Fidesops is failing to connect to, unless it appears in the stack trace?

good improvement, that probably won't be in the trace without adding it.

seanpreston · 2022-04-18T19:52:20Z

src/fidesops/core/config.py

@@ -178,14 +178,17 @@ class FidesopsConfig(FidesSettings):
    execution: ExecutionSettings

    is_test_mode: bool = os.getenv("TESTING") == "True"
-    hot_reloading: bool = os.getenv("DEV_MODE") == "True"
+    hot_reloading: bool = os.getenv("FIDESOPS_HOT_RELOAD") == "True"
+    dev_mode: bool = os.getenv("FIDESOPS_DEV_MODE") == "True"

    class Config:  # pylint: disable=C0115
        case_sensitive = True


I think we'll need to add env_prefix = FIDESOPS_ for this to work being able to reference these vars as FIDESOPS_VAR_NAME? We should use FIDESOPS__ as the prefix though, since that's consistent with the sub-sections above.

Added an extra underscore, I was following the FIDESOPS_CONFIG_PATH variable, so I've updated that to also have double underscores.

… adjusts existing FIDESOPS__CONFIG_PATH. - Explain hot reloading. - When logging original exception for saas requests, also log the key.

pattisdr · 2022-04-18T21:14:20Z

thanks for your comments @seanpreston

galvana

Looks like everyone's had a chance to comment on so we're ready to merge!

Adding DEV_MODE for easier debugging of SaaS requests

pattisdr added 2 commits April 5, 2022 15:15

Add DEV_MODE for easier debugging of saas requests.

911eaa1

- No retries - Exceptions are not caught; entire traceback is logged. - Details of saas requests are logged - path, headers, body.

Fix typo.

b991ff0

eastandwestwind reviewed Apr 5, 2022

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

src/fidesops/service/connectors/saas_connector.py Show resolved Hide resolved

src/fidesops/service/connectors/saas_connector.py Outdated Show resolved Hide resolved

pattisdr commented Apr 6, 2022

View reviewed changes

src/fidesops/service/connectors/saas_connector.py Outdated Show resolved Hide resolved

pattisdr marked this pull request as draft April 6, 2022 20:55

pattisdr added 4 commits April 18, 2022 10:18

Merge branch 'main' into fidesops_dev_mode

dc8949b

Update DEV_MODE/LOG_PII/HOT_RELOAD env vars to have FIDESOPS prefixes…

f79b716

… to not get mixed up with other services.

Running into mypy issues with attempting to use f-strings and bytes: …

9f06b59

…On Python 3 '{}'.format(b'abc') produces "b'abc'", not 'abc'; use '{!r}'.format(b'abc') if this is desired behavior

Log original exception and saas responses in dev mode.

3e7d317

pattisdr marked this pull request as ready for review April 18, 2022 19:37

pattisdr changed the title ~~[Discussion Needed] Add DEV_MODE for Easier SaaS Request Debugging~~ Add FIDESOPS_DEV_MODE for Easier SaaS Request Debugging Apr 18, 2022

seanpreston reviewed Apr 18, 2022

View reviewed changes

Add two underscores after FIDESOPS prefix for consistency - this also…

574bf60

… adjusts existing FIDESOPS__CONFIG_PATH. - Explain hot reloading. - When logging original exception for saas requests, also log the key.

pattisdr changed the title ~~Add FIDESOPS_DEV_MODE for Easier SaaS Request Debugging~~ Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging Apr 18, 2022

seanpreston assigned galvana Apr 25, 2022

galvana approved these changes May 2, 2022

View reviewed changes

galvana merged commit 12e25a1 into main May 2, 2022

galvana deleted the fidesops_dev_mode branch May 2, 2022 21:03

pattisdr mentioned this pull request May 3, 2022

mypy failing with "config not defined" #449

Closed

adamsachs pushed a commit to adamsachs/fidesops_forked_test that referenced this pull request May 17, 2022

Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging (ethyca#363)

d806e3c

Adding DEV_MODE for easier debugging of SaaS requests

sanders41 pushed a commit that referenced this pull request Sep 22, 2022

Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging (#363)

0ea593f

Adding DEV_MODE for easier debugging of SaaS requests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging #363

Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging #363

pattisdr commented Apr 5, 2022 •

edited

Loading

eastandwestwind left a comment

eastandwestwind commented Apr 5, 2022

pattisdr commented Apr 5, 2022

seanpreston Apr 18, 2022

pattisdr Apr 18, 2022

seanpreston Apr 18, 2022

pattisdr Apr 18, 2022 •

edited

Loading

seanpreston Apr 18, 2022 •

edited

Loading

pattisdr Apr 18, 2022

seanpreston Apr 18, 2022 •

edited

Loading

pattisdr Apr 18, 2022

pattisdr commented Apr 18, 2022

galvana left a comment

Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging #363

Add FIDESOPS__DEV_MODE for Easier SaaS Request Debugging #363

Conversation

pattisdr commented Apr 5, 2022 • edited Loading

Purpose

Changes

Checklist

Ticket

eastandwestwind left a comment

Choose a reason for hiding this comment

eastandwestwind commented Apr 5, 2022

pattisdr commented Apr 5, 2022

seanpreston Apr 18, 2022

Choose a reason for hiding this comment

pattisdr Apr 18, 2022

Choose a reason for hiding this comment

seanpreston Apr 18, 2022

Choose a reason for hiding this comment

pattisdr Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

seanpreston Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

pattisdr Apr 18, 2022

Choose a reason for hiding this comment

seanpreston Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

pattisdr Apr 18, 2022

Choose a reason for hiding this comment

pattisdr commented Apr 18, 2022

galvana left a comment

Choose a reason for hiding this comment

pattisdr commented Apr 5, 2022 •

edited

Loading

pattisdr Apr 18, 2022 •

edited

Loading

seanpreston Apr 18, 2022 •

edited

Loading

seanpreston Apr 18, 2022 •

edited

Loading