Change tracking #4768

kravets-levko · 2020-04-01T18:10:23Z

What type of PR is this? (check all applicable)

Refactor
Feature

Description

Example of data stored in DB

Closes #3136.

…eir parents (query, dashboard)

kravets-levko · 2020-04-06T11:07:41Z

@arikfr (cc @rauchy) I still need to update tests, but the feature code is ready and I'd like to know your opinion about the result/what could be improved/etc. Also, I have two more questions:

when forking query, should it also log creation of all new visualizatons?
should it track API key updates (for dashboards and queries)?

redash/handlers/dashboards.py

arikfr · 2020-04-10T14:48:23Z

redash/models/changes.py

+def track_changes(attributes, parent_attr=None):
+    attributes = set(attributes) - {"id", "created_at", "updated_at", "version"}
+
+    # monkey-patch class because inheritance will break SQLAlchemy


I decided to create a decorator for change tracking because it looks nice when you use in when defining a model. Decorator needs to modify original model class: intercept __setattr__ and add a method to save changes. First I tried to inherit from original model class (class ChangeTracking(cls)) and it even worked. But in some cases SQLA was throwing errors - something like class is not mapped or like this (in particular that happened in tests). So eventually I checked how they implement own decorators in SQLA (e.g. sqlalchemy_utils.models.generic_repr) and realized that instead of using inheritance, they just monkey-patch the class (replace existing methods with own ones). I tried to do so - and it worked. OOP in Python is a bit weird; I guess when I inherited my class - it changed something in a mechanism SQLA uses to get model's metadata; when patching class I in fact keep the class itself untouched, and everything works

redash/models/changes.py

arikfr · 2020-04-10T14:54:51Z

redash/models/changes.py

+
+        changes = Change(
+            object=self,
+            object_version=getattr(self, "version", None),


I'm not sure we have any use for version -- maybe we can skip it?

Done.

Upd.: if needed - we can always log it as a regular field (in changes array)

arikfr · 2020-04-10T14:55:25Z

redash/models/changes.py

+            user=changed_by,
+            change={
+                "object_type": self.__table__.name,
+                "object_id": self.id,


Isn't object_type/object_id redundant? Or you have it here for cases when changes.object is the parent?

Or you have it here for cases when changes.object is the parent?

Exactly. When we modify, say, visualization - we log it as a change to corresponding query; but then there is no other way to know that changes were made to visualization. I tried few different data structures, but this one looked the most straightforward

I added comment in the code that describes a structure of changes record and why we need object_type/object_id in changes field

redash/models/changes.py

arikfr · 2020-04-10T15:00:50Z

redash/models/changes.py

@@ -1,12 +1,20 @@
 from sqlalchemy.inspection import inspect
 from sqlalchemy_utils.models import generic_repr
+from sqlalchemy.orm import object_session
+
+from enum import Enum

 from .base import GFKBase, db, Column
 from .types import PseudoJSON


 @generic_repr("id", "object_type", "object_id", "created_at")
 class Change(GFKBase, db.Model):


We need to add index(es) for this table to make it efficient to run a query like: give me last N changes for object type X and id Y.

Currently I added only index for object_type, object_id pair. When I'll start working on API - I'll check if we need more indexes or not.

arikfr · 2020-04-10T15:03:09Z

when forking query, should it also log creation of all new visualizatons?

Yes.

should it track API key updates (for dashboards and queries)?

Yes.

…(stop loading it from DB)

kravets-levko · 2020-04-13T10:53:40Z

Recent changes (since last code review):

when creating/forking query it now logs its visualizations as well;
stopped logging object_version, removed this column from the changes table; updated migration script;
added index for object_type + object_id fields;
when creating new instance - log all initial fields (not just modified);
log changes to API keys;
optimized a way we access "parent" object - now it will not load instance from DB (this also simplified API keys tracking).

redash/handlers/dashboards.py

migrations/versions/4952e040e9dd_clear_changes_table.py

jhult · 2021-01-31T01:42:25Z

This looks like a cool feature. Any news on it?

guidopetri · 2023-07-21T19:46:34Z

@kravets-levko , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested (or @jhult ) in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

Change tracking

6d4bc4c

kravets-levko added the Backend label Apr 1, 2020

kravets-levko self-assigned this Apr 1, 2020

kravets-levko added 3 commits April 2, 2020 21:20

Change tracking

dcf2a55

Change tracking: query and dashboard attributes

1d440fa

Change tracking: visualization and widget attributes

49feef7

kravets-levko force-pushed the change-tracking branch from a8dfd75 to 0e8e4d8 Compare April 6, 2020 10:48

Log changes to nested objects (visualizations, widgets) related to th…

b2adadc

…eir parents (query, dashboard)

kravets-levko force-pushed the change-tracking branch from 0e8e4d8 to b2adadc Compare April 6, 2020 10:49

kravets-levko requested review from arikfr and rauchy April 6, 2020 10:55

kravets-levko added 3 commits April 6, 2020 16:25

Migraton to clean changes table

3b4bc15

Replace inheritance with patching to avoid SQLAlchemy issues

15d3af3

Temp

527b73a

arikfr reviewed Apr 10, 2020

View reviewed changes

redash/handlers/dashboards.py Show resolved Hide resolved

arikfr reviewed Apr 10, 2020

View reviewed changes

kravets-levko added 4 commits April 13, 2020 12:37

Update migration: add index, drop unused column, add downgrade script

69ee572

Ensure that all fields are logged when creating a new object

1a8b46e

Log changes to API keys; optimize a way we access to a parent object …

baac67a

…(stop loading it from DB)

When creating/forking query - log changes to its visualzations

26b6bff

arikfr reviewed Apr 13, 2020

View reviewed changes

redash/handlers/dashboards.py Show resolved Hide resolved

migrations/versions/4952e040e9dd_clear_changes_table.py Outdated Show resolved Hide resolved

kravets-levko added 6 commits April 13, 2020 15:36

Add org_id column to changes table

505e90c

Add tests for all models that use change tracking

a36fbe4

Add some comments to tests

8e61afe

API endpoints to access changes (basic implementation)

587e755

Track changes to Alerts and Alert Destinations

b74beb0

Log changes to Alerts and Alert Destinations

3592ca5

kravets-levko added 4 commits April 17, 2020 12:36

Add API endpoint for Alert Destination changes; refine API code

3f94434

Changes API: order of items

995fa33

Add some tests for API endpoints

49ff502

Merge branch 'master' into change-tracking

f49826c

kravets-levko marked this pull request as ready for review April 20, 2020 16:27

kravets-levko requested a review from arikfr April 20, 2020 16:58

weekly-digest bot mentioned this pull request Apr 27, 2020

Weekly Digest (20 April, 2020 - 27 April, 2020) #4839

Closed

guidopetri added the 1-week-review label Jul 21, 2023

guidopetri closed this Jul 29, 2023

guidopetri deleted the change-tracking branch July 29, 2023 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change tracking #4768

Change tracking #4768

kravets-levko commented Apr 1, 2020 •

edited

Loading

kravets-levko commented Apr 6, 2020

arikfr Apr 10, 2020

kravets-levko Apr 13, 2020

arikfr Apr 10, 2020

kravets-levko Apr 13, 2020 •

edited

Loading

arikfr Apr 10, 2020

kravets-levko Apr 10, 2020

kravets-levko Apr 13, 2020

arikfr Apr 10, 2020

kravets-levko Apr 13, 2020

arikfr commented Apr 10, 2020

kravets-levko commented Apr 13, 2020

jhult commented Jan 31, 2021

guidopetri commented Jul 21, 2023

Change tracking #4768

Change tracking #4768

Conversation

kravets-levko commented Apr 1, 2020 • edited Loading

What type of PR is this? (check all applicable)

Description

Example of data stored in DB

kravets-levko commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kravets-levko Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr commented Apr 10, 2020

kravets-levko commented Apr 13, 2020

jhult commented Jan 31, 2021

guidopetri commented Jul 21, 2023

kravets-levko commented Apr 1, 2020 •

edited

Loading

kravets-levko Apr 13, 2020 •

edited

Loading