353- initial infra for fideslog integration #541

eastandwestwind · 2022-05-19T18:25:40Z

NOTICE: Upon merging, internal devs should set a new env var: `FIDESOPS__ROOT_USER__ANALYTICS_ID=internal`

Purpose

Adds fideslog integration to fidesops.

Changes

Adds new Analytics class and associated schema, so we have ability to send event data
Adds new env var in config for global opt in/out with default as opt out = True (we can adjust as needed in future as we implement more granular user controls)
Updates docs for config

Testing Steps

make server with no env vars should do 2 things: 1) set a ANALYTICS_ID in fidesops.toml, and 2) send a server_start event, viewable in logs: fidesops | INFO:fidesops.analytics:Analytics event sent: server_start with client id: 5905867115b98e73c35a60a35983fbef
make server with your ANALYTICS_ID in fidesops.toml. The server_start event should still be sent, and the original ANALYTICS_IDshould exist infidesops.toml`
make server with FIDESOPS__ROOT_USER__ANALYTICS_ID env var set to internal should send event with client id of internal, regardless of ANALYTICS_ID config value.
Running tests via make commands should not send events
Running CI should not send events

Checklist

Update CHANGELOG.md file in appropriate sections
Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
If docs updated (select one):
- documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
- documentation issue created (tag docs-team to complete issue separately)
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 #353

eastandwestwind · 2022-05-19T20:35:58Z

Got some feedback from @PSalant726 on this via screenshare code review. Will work on this Monday

PSalant726

Notes from today's screen share so I don't forget 😅

docs/fidesops/docs/guides/configuration_reference.md

fidesops.toml

requirements.txt

src/fidesops/analytics.py

src/fidesops/core/config.py

src/fidesops/schemas/analytics.py

PSalant726

I see several of my previous comments have 👍 's or have been resolved, but I'm not seeing any code changes since those comments were left. Maybe you just haven't pushed them up yet - I just wanted to point that out.

requirements.txt

src/fidesops/analytics.py

src/fidesops/schemas/analytics.py

…tart event

eastandwestwind · 2022-06-01T15:23:49Z

@PSalant726 this is ready for another pass!

Makefile

run_infrastructure.py

src/fidesops/analytics.py

src/fidesops/core/config.py

src/fidesops/main.py

src/fidesops/schemas/analytics.py

src/fidesops/analytics.py

PSalant726

Only one of these comments is blocking this right now - the rest are just nits. This is really close 🙏

Makefile

data/config/fidesops.toml

fidesops.toml

run_infrastructure.py

src/fidesops/analytics.py

…of analytics by default

eastandwestwind · 2022-06-07T13:41:05Z

Thanks again for the review @PSalant726 ! Back over to you, with some need for clarification

PSalant726

All of my comments at this point are just nice-to-haves, nothing blocking. Thanks for sticking with this - it's looking great now 🙌

Makefile

data/config/fidesops.toml

run_infrastructure.py

fidesops.toml

eastandwestwind · 2022-06-07T20:58:47Z

Ok we're ready again @PSalant726 !

* 353- initial infra for fideslog integration * adds changelog * cr and adds storage of analytics id in config * implement internal mode, exclude tests/CI, implement sending server start event * remove validationerr * format * sort * lint * cr changes * lint fixes * Adds root_user to test toml config * Add analytics opt out arg to parser of run_infrastructure * implicit true when passing env var to docker-compose * small code style changes, and updating test fidesops.toml to opt out of analytics by default * missing comma * spacing issue * remove fidesctl dep

eastandwestwind added 2 commits May 19, 2022 13:23

353- initial infra for fideslog integration

751c690

adds changelog

6aca837

eastandwestwind added the DON'T MERGE label May 19, 2022

PSalant726 reviewed May 19, 2022

View reviewed changes

PSalant726 suggested changes May 24, 2022

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

src/fidesops/analytics.py Outdated Show resolved Hide resolved

src/fidesops/schemas/analytics.py Outdated Show resolved Hide resolved

eastandwestwind added 2 commits May 25, 2022 08:19

cr and adds storage of analytics id in config

f0d25aa

implement internal mode, exclude tests/CI, implement sending server s…

8047877

…tart event

seanpreston assigned sanders41 and PSalant726 and unassigned sanders41 Jun 1, 2022

eastandwestwind added 2 commits June 1, 2022 10:14

remove validationerr

a122531

merge in latest, resolve conflicts

529eb85

eastandwestwind added 3 commits June 1, 2022 11:03

format

e42abc3

sort

f76629b

lint

206aea4

PSalant726 suggested changes Jun 2, 2022

View reviewed changes

PSalant726 assigned eastandwestwind and unassigned PSalant726 Jun 2, 2022

PSalant726 added documentation Improvements or additions to documentation enhancement New feature or request dependencies Pull requests that update a dependency file and removed DON'T MERGE labels Jun 2, 2022

eastandwestwind added 4 commits June 2, 2022 20:04

cr changes

aff2f94

merge in latest main, fix conflicts

4db73de

lint fixes

61f153e

Adds root_user to test toml config

e86c5b0

eastandwestwind added 2 commits June 6, 2022 14:55

Add analytics opt out arg to parser of run_infrastructure

832489d

implicit true when passing env var to docker-compose

2dda131

PSalant726 suggested changes Jun 6, 2022

View reviewed changes

small code style changes, and updating test fidesops.toml to opt out …

a1d1b07

…of analytics by default

PSalant726 approved these changes Jun 7, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

data/config/fidesops.toml Show resolved Hide resolved

run_infrastructure.py Outdated Show resolved Hide resolved

fidesops.toml Show resolved Hide resolved

fidesops.toml Show resolved Hide resolved

eastandwestwind added 4 commits June 7, 2022 14:20

missing comma

327fbf8

spacing issue

894ffdc

merge in latest, fix conflicts

151ccc8

remove fidesctl dep

55573c9

PSalant726 approved these changes Jun 7, 2022

View reviewed changes

PSalant726 merged commit 0296ba0 into main Jun 7, 2022

PSalant726 deleted the 353-fideslog-integration branch June 7, 2022 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

353- initial infra for fideslog integration #541

353- initial infra for fideslog integration #541

eastandwestwind commented May 19, 2022 •

edited

Loading

eastandwestwind commented May 19, 2022

PSalant726 left a comment

PSalant726 left a comment

eastandwestwind commented Jun 1, 2022

PSalant726 left a comment •

edited

Loading

eastandwestwind commented Jun 7, 2022

PSalant726 left a comment

eastandwestwind commented Jun 7, 2022

353- initial infra for fideslog integration #541

353- initial infra for fideslog integration #541

Conversation

eastandwestwind commented May 19, 2022 • edited Loading

NOTICE: Upon merging, internal devs should set a new env var: FIDESOPS__ROOT_USER__ANALYTICS_ID=internal

Purpose

Changes

Testing Steps

Checklist

Ticket

eastandwestwind commented May 19, 2022

PSalant726 left a comment

Choose a reason for hiding this comment

PSalant726 left a comment

Choose a reason for hiding this comment

eastandwestwind commented Jun 1, 2022

PSalant726 left a comment • edited Loading

Choose a reason for hiding this comment

eastandwestwind commented Jun 7, 2022

PSalant726 left a comment

Choose a reason for hiding this comment

eastandwestwind commented Jun 7, 2022

eastandwestwind commented May 19, 2022 •

edited

Loading

NOTICE: Upon merging, internal devs should set a new env var: `FIDESOPS__ROOT_USER__ANALYTICS_ID=internal`

PSalant726 left a comment •

edited

Loading