feat(dashboards): allow one insight on many dashboards #9331

pauldambra · 2022-04-04T12:05:46Z

Opening a PR as a mini-RFC

Dashboards Model relations

Currently

At the model level

Insights can be saved by themselves or on a Dashboard

If they are saved to a dashboard and the dashboard is deleted the insight is deleted too

The relationship is different depending on which side you start at

erDiagram
    INSIGHT ||--|o DASHBOARD : ""
    DASHBOARD ||--o{ INSIGHT: "items"

A single Insight can be on zero or one Dashboard
A single Dashboard can have zero to many Insights

The foreign key is specified on Insight. The relationship specifies a "related_name" so Dashboard has a relationship back to Insight

At the API level

loading a dashboard

https://app.posthog.com/api/projects/:team_id/dashboards/:dashboard_id/?refresh=false

Viewing a dashboard makes an API call for that dashboard by ID

That call returns a structure

{
  "id": dashboard_id,
  "items": [
    // list of insights on dashboard
  ],
  "created_at": "2022-01-12T19:39:35.014190Z",
  "created_by": {
    // user
  },
  // .... other fields
}

loading an insight

https://app.posthog.com/api/projects/:team_id/insights/?short_id=the-short-id

Viewing an insight directly makes an API call by short id

This call returns a paginated structure (ideally (always?) with one result)

{
  "count": 1,
  "next": null,
  "previous": null,
  "results": [
    {
      "short_id": "the-short-id",
      "dashboard": dashboard_id,
        // .... other fields
    }
  ]
}

The API maps the same relationship:

Dashboards have many "items"
Insights have a single dashboard

Desired relationship

erDiagram
    INSIGHT }o--o{ DASHBOARD : ""

The desired relationship is that:

Dashboards have zero to many insights
Any Insight can be on many dashboards

Both insights and dashboards can exist without the other

Django many-to-many relationships implicitly create a join table

erDiagram
    INSIGHT ||--o{ DASHBOARD_INSIGHT: ""
    DASHBOARD_INSIGHT }o--|| DASHBOARD: ""

Manually creating that join model would allow future changes which could store data unique to that dashboard/item relation

E.g. a different date range applied to the same insight on different dashboards

Complications

The insight model is stored in a posthog_dashboarditem table

Dashboard Item might not be the correct name for the relation. Dashboards may in future hold more than only insights. E.g. Markdown blocks

Leaving Insights on "dashboard item" constrains either future naming, or means a markdown block would be modelled as an insight in code.

To change this we would need a migration to rename the table or to copy the data to a new table

That migration would need to work for cloud and self-host

the insight model already has a one way relationship direct to dashboard

Any migration to a join table would need to migrate the items relationship off Insight and keep any existing relationship

Proposed Approach

Ultimately, the approach is to add a new relation onto Dashboard

insights = models.ManyToManyField(Insight)

and deprecate the existing dashboard field on Insight

This allows querying in both directions. And can have a manual "through" table specified in case future changes need to store data associated with a particular insight/dashboard combo. E.g. different date filters on the "last month" dashboard and "this week" dashboard

(optional) Rename the posthog_dashboarditems table

PostgreSQL documentation on alter table for rename says:

"There is no effect on the stored data."

It would need testing, but this suggests we can have a single migration and code change that renames the insight table and changes the table name in the model

TODO This could lead to a DB that has had the table renamed and running instances that are expecting the old table name. That would need to be addressed, adding complication to this work

It is quicker to leave Insights on this table until PostHog 2.0 allows breaking changes and add a TODO to that effect on the model

change the Insight API to present an array of dashboard ids

Currently, the API presents a single dashboard ID. The UI uses this to provide a button to jump to the dashboard. This would need to change to a dropdown button showing the one to many dashboards you could visit from a given insight

The following UI interactions would need to change:

adding to a dashboard becomes multiselect
showing what dashboard an insight is on can't be a single button

There are 17 places that the UI code knows that dashboard ID is number | null and changing to number | number[]| null or adding a new number[] property would need to be accounted for

Put a new relation on dashboards to a new DashboardItem model

insights = models.ManyToManyField(Insight)

Initially empty, the dashboard API would read from both insights and items relations and union and deduplicate the results of this relation and the existing Items relation

3. Migrate existing dashboard-insight relation onto the new model

In a perfect world this could be as simple as looping through Insight models, prefetching their dashboard, and storing the relation onto the new insights relation on that instance of Dashboard

Based on the number of Insights stored on cloud. This would take more than long enough to cause deployments to fail or data to appear to be inconsistent.

~~The join migration would need to be manually written as a fast set based SQL operation~~ see comment

Or the Insight model would union and deduplicate its old and new relation and present that to the outside world. While an async migration paged through the DB updates needed

pauldambra · 2022-04-04T15:30:08Z

@clarkus the only UI impact I can see of this is the flow of showing what dashboards an insight is on, navigating to them from the insight, and adding it to more

Twixes · 2022-04-05T10:01:36Z

Yeah, we'll definitely need a reworked modal to go from a boolean approach (either is on a dashboard or isn't) to a number (anywhere between 0 and n dashboards).
One tricky thing we definitely need to keep in mind in this project is dashboard permissions: currently when an insight belongs to a dashboard the user can't edit, that user also can't edit that insight. But what if only one out of a few dashboards the insight is on is restricted (e.g. user can edit "Funnels usage" but can't edit "Board meeting metrics")? 🤔

… joins

pauldambra · 2022-04-05T10:28:53Z

Added many insights using

insert into posthog_dashboarditem (short_id, team_id, filters, favorited, is_sample, deleted, saved, updated_at,
                                   layouts, last_modified_at, refreshing, dashboard_id)
select left(md5((i::int+3000000)::text), 12),
       1,
       to_jsonb('{}'::text),
       False,
       False,
       False,
       False,
       now(),
       to_jsonb('{}'::text),
       now(),
       False,
       1
from generate_series(1, 1000000) s(i);

and inserted them in one step as a synchronous migration

insert into posthog_dashboardinsight (insight_id, dashboard_id)
select id as insight_id, dashboard_id from posthog_dashboarditem
where dashboard_id is not null
ON CONFLICT ON CONSTRAINT posthog_dashboardinsight_dashboard_id_insight_id_675423e4_uniq
DO NOTHING;

This took between 18 and 25 seconds over three attempts

A million is a lot of insights but this demonstrates that a single insert migration could timeout and cause issues with upgrades

Twixes · 2022-04-05T10:53:50Z

That's my main worry with splitting posthog_insight out of posthog_dashboarditem… It shouldn't be an issue for self-hosted setups, but Cloud is tricky. Chunking the migration into more doable portions may help, but if that still takes more than 10 seconds, I worry about the impact of (and on) users editing insights at that time. @guidoiaquinti may have some more/better tips here.

guidoiaquinti · 2022-04-05T12:31:09Z

That's my main worry with splitting posthog_insight out of posthog_dashboarditem… It shouldn't be an issue for self-hosted setups, but Cloud is tricky. Chunking the migration into more doable portions may help, but if that still takes more than 10 seconds, I worry about the impact of (and on) users editing insights at that time. @guidoiaquinti may have some more/better tips here.

That's a valid concern. As we don't know what's our customer p100 sort by row count on self-hosted, we should make this migration idempotent and split it into chunks in a loop (e.g. 100 rows/query) that will be retried in case of failures.

There's also a (probably marginal) race condition:

t0 - app is running
t1 - user starts to upgrade the app -> migration starts, web is still running the old version
t2 - user adds an insight (as the app is the old version we sill write the record to the "old" table)
t3 - migration completes without migrating the insight
t4 - app migration completes, web is now running the new version
t5 - there's an orphan record

…es to new dashboard insight relation

…ytomany relation

…is_ n+1

clarkus · 2022-04-05T23:23:45Z

I made some progress from the insights side of things. This allows for adding an insight to one or more dashboards, or creating a new dashboard and then adding. I am working through the dashboard side of things next.

pauldambra · 2022-04-06T16:45:13Z

…after connecting dashboards and insights

neilkakkar · 2022-04-12T12:06:51Z

Q: Is there a link to somewhere I can see where we decided doing this makes sense / solves a good use case / etc. etc. ? This mini-RFC feels more implementation focused, and I'm missing context 😅

mariusandra

I think this is probably the least disruptive way to bring about this database change 👍.

This is still a draft, so I'm not sure what is in the TODO list and what is not, nevertheless here and inline are some comments/questions. Happy to address them myself as well.

@pauldambra when you did your test here,... did the table you were inserting to have an index or not? Those 18-25 seconds for copying 1M rows from one table to another seem like a lot. Could it be that this was the case, and this insert could be made much faster by cleverly dropping and creating indexes? This feels way too long.
We already need an explicit table for the relation, as the fields layouts and color should go there, and be removed from insight. They can be unique for each dashboards.

mariusandra · 2022-04-12T13:07:30Z

ee/clickhouse/views/insights.py

+        # check permissions on any dashboard on legacy dashboard relation or the many-to-many dashboards relation
+        dashboards: List[Dashboard] = [insight.dashboard] + list(insight.dashboards.all())
+        edit_permissions = [d.can_user_edit(cast(User, request.user).id) for d in dashboards if d is not None]
+        return all(edit_permissions)


I'd argue you should be able to edit an insight if you can edit any dashboard the insight is on, not only if you can edit all dashboards the insight is on.

As an example, when someone from the company's board adds an insight onto their private dashboard, you wouldn't expect everyone else to automatically lose edit rights? I'd expect the opposite: the private board dashboards should also show, that this specific insight has been shared more broadly.

mariusandra · 2022-04-12T14:46:48Z

posthog/test/test_dashboard_insight_model.py

+from posthog.test.base import BaseTest
+
+
+class TestDashboardInsightModel(BaseTest):


I don't see the DashboardInsight itself. Should this be the table between dashboards and insights? If so, is this the right name for such a table, given it'll contain 1) what to show 2) coordinates of the item on the dashboard, 3) color of the item. The "item" is currently just an insight, but it can also be a block of text, the events list table, or whatever. Should we make a new many-to-many model for every new type of dashboard element?

posthog-bot · 2022-04-20T07:33:00Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

pauldambra · 2022-04-20T13:46:57Z

superceded by #9416

pauldambra · 2022-04-20T13:48:34Z

@neilkakkar I missed your comment, sorry

There's not much more there but the RFC for the insights work is here https://github.com/PostHog/product-internal/blob/5c4efc9f46416dabd239a5ef931613e035596a18/requests-for-comments/2022-03-14%20nail%20insights.md

add a TODO on insight

e0bf95c

pauldambra mentioned this pull request Apr 4, 2022

fix(insight): remove unused action and reducer #9336

Merged

pauldambra added 3 commits April 5, 2022 10:18

add manytomany dashboard to insight relation

af7a0ce

merge from master and update migration

5d9a120

remove unnecessary change to IDE config file

72b31d5

pauldambra added 2 commits April 5, 2022 11:15

update migration to add unique constraint on posthog_dashboardinsight…

9b90c68

… joins

Merge branch 'master' into many-to-many-insights

aa88da9

pauldambra added 6 commits April 5, 2022 14:29

Add migration that pages through old dashboard item relation and copi…

c4b5d8f

…es to new dashboard insight relation

correct reverse relationship name

64230de

hide the insight/item split from insight patch calls

3108ad6

creating an insight with a dashboard id adds the dashboard to the man…

2270c07

…ytomany relation

make the new dashboard tests less rubbish

73939e3

remove second migration

0d5d09e

pauldambra mentioned this pull request Apr 5, 2022

feat: migrate insight.items to dashboard.insights #9351

Closed

pauldambra added 7 commits April 5, 2022 16:37

don't hardcode ids in tests

d55524c

don't hardcode ids in tests

6df5af8

can safely add dashboard links on create and update

29c9613

write a test to show that loading a dashboard is nplus1

5538fd8

make test pass and add a comment that adding insights to dashboards _…

9e3a592

…is_ n+1

dashboard does not show soft deleted insights

ff45451

skip test instead of having incorrect assertion

424cf90

start refactoring insight tests

6229e92

pauldambra mentioned this pull request Apr 6, 2022

fix: uncomment and correct an assertion #9355

Closed

Merge branch 'master' into many-to-many-insights

e8484cd

pauldambra added 2 commits April 6, 2022 19:28

deal with caching WIP

eb49426

a pass at dashboard+insight specific filter hashes

8cc0bb5

pauldambra mentioned this pull request Apr 7, 2022

fix: n-plus-1 on dashboard get #9365

Merged

pauldambra added 17 commits April 7, 2022 17:19

wip

e55eb9d

restrict but don't flatten the number of queries to load a dashboard …

f1b1c74

…after connecting dashboards and insights

correctly dedupe and update insights

a71cc83

fix cache test

1602e40

raise validation error on illegal dashboard link

3aecf21

make insight to representation dashboard aware filtering work

f00b479

team id in test needs to be consistent

ce2567e

fix insight patching behaviour

5a37d01

better linking between dashboard and insight

e1b2100

convert to many-to-many without manual through table - WIP

2d1ba56

works but never writes to the cache

4d3b57c

resolve nplus1 test

8dc7086

cache key WIP

ba7af95

WIP

eddb23b

improvements to keeping dashboard and dashboards in sync

9baa3de

add comment with todo to update_cache

1879647

remove unnecessary todo from dashboard logic

9228e3e

mariusandra reviewed Apr 12, 2022

View reviewed changes

mariusandra mentioned this pull request Apr 13, 2022

feature(insights): many-to-many dashboards #9408

Closed

posthog-bot added the stale label Apr 20, 2022

pauldambra mentioned this pull request Apr 20, 2022

feat(insights): dashboards insights many-to-many another attempt #9416

Merged

pauldambra closed this Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(dashboards): allow one insight on many dashboards #9331

feat(dashboards): allow one insight on many dashboards #9331

Uh oh!

pauldambra commented Apr 4, 2022 •

edited

Loading

Uh oh!

pauldambra commented Apr 4, 2022

Uh oh!

Twixes commented Apr 5, 2022

Uh oh!

pauldambra commented Apr 5, 2022

Uh oh!

Twixes commented Apr 5, 2022

Uh oh!

guidoiaquinti commented Apr 5, 2022

Uh oh!

clarkus commented Apr 5, 2022

Uh oh!

pauldambra commented Apr 6, 2022 •

edited

Loading

Uh oh!

neilkakkar commented Apr 12, 2022

Uh oh!

mariusandra left a comment

Uh oh!

mariusandra Apr 12, 2022

Uh oh!

mariusandra Apr 12, 2022

Uh oh!

posthog-bot commented Apr 20, 2022

Uh oh!

pauldambra commented Apr 20, 2022

Uh oh!

pauldambra commented Apr 20, 2022

Uh oh!

Uh oh!

		from posthog.test.base import BaseTest


		class TestDashboardInsightModel(BaseTest):

feat(dashboards): allow one insight on many dashboards #9331

feat(dashboards): allow one insight on many dashboards #9331

Uh oh!

Conversation

pauldambra commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dashboards Model relations

Currently

At the model level

At the API level

loading a dashboard

loading an insight

Desired relationship

Complications

The insight model is stored in a posthog_dashboarditem table

the insight model already has a one way relationship direct to dashboard

Proposed Approach

(optional) Rename the posthog_dashboarditems table

change the Insight API to present an array of dashboard ids

Put a new relation on dashboards to a new DashboardItem model

3. Migrate existing dashboard-insight relation onto the new model

Uh oh!

pauldambra commented Apr 4, 2022

Uh oh!

Twixes commented Apr 5, 2022

Uh oh!

pauldambra commented Apr 5, 2022

Uh oh!

Twixes commented Apr 5, 2022

Uh oh!

guidoiaquinti commented Apr 5, 2022

Uh oh!

clarkus commented Apr 5, 2022

Uh oh!

pauldambra commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neilkakkar commented Apr 12, 2022

Uh oh!

mariusandra left a comment

Choose a reason for hiding this comment

Uh oh!

mariusandra Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

mariusandra Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

posthog-bot commented Apr 20, 2022

Uh oh!

pauldambra commented Apr 20, 2022

Uh oh!

pauldambra commented Apr 20, 2022

Uh oh!

Uh oh!

pauldambra commented Apr 4, 2022 •

edited

Loading

pauldambra commented Apr 6, 2022 •

edited

Loading