fix(insights): Remove insight reuse on dashboard and insights #21471

webjunkie · 2024-04-10T15:45:05Z

Problem

Another try for #20371 and #20508

How to reproduce:

create two dashboards with separate filters
- e.g. dashboard A filters for "browser = Firefox" an dashboard B for "browser = Hogbrowser"
add an insight, e.g. pageviews per day and add it to both dashboards
when switching between dashboards it will first show correct data depending on where you go first, but then start to show wrong data, for example no data when you previously already saw the Firefox filtered data
on top of that, when going to view or edit the insight, you will also still see wrong data and when editing, you will also the filters from the dashboard that are not really stored on the insight

Changes

remove any reuse of logics and cached insights accross dashboards and/or insight scene.
cleaned up when insightLogic and dataNodeLogic should load stuff
- it's important they don't load onMount as we already have insights from the main dashboard call
  - which is nice since we get all data in one call from cache

Does this work well for both Cloud and self-hosted?

it doesn't have an impact

How did you test this code?

tested creating insights, filters, dashboards
see extended Crypress tests... got them to pass 🤞🏻

posthog-bot · 2024-04-10T15:55:11Z

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

github-actions · 2024-04-10T16:04:08Z

Size Change: -259 B (0%)

Total Size: 1.04 MB

ℹ️ View Unchanged

Filename	Size	Change
`frontend/dist/toolbar.js`	1.04 MB	-259 B (0%)

_{compressed-size-action}

webjunkie · 2024-04-11T10:53:29Z

@thmsobrmlr Asking you since @Twixes is ooo.

I started to think about dashboard insight loading from scratch. But when accepting that we might just load things fresh from the backend, I found that I can quick fix these issues after all, by removing specific logic that reuses insights.

Do you think this works as a short term solution (while I still check if we might cache some insights)? If you don't want to dig into it, it can also wait for Michael.

thmsobrmlr · 2024-04-11T11:11:05Z

I started to think about dashboard insight loading from scratch. But when accepting that we might just load things fresh from the backend, I found that I can quick fix these issues after all, by removing specific logic that reuses insights.

I don't have a definitive answer here, but imagine that cached results are quite important for performance. I think something causes insights to load more often then they should when visiting a dashboard, but there are other instance where we still take the cached result.

posthog-bot · 2024-04-24T19:08:27Z

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

chromium: 0 added, 4 modified, 0 deleted (diff for shard 1, diff for shard 2)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Twixes

I fixed one more edge case that's been reported and added a test for that. Looking good, just pending a couple extra questions!

Twixes · 2024-04-25T11:55:31Z

frontend/src/scenes/insights/insightLogic.test.ts

@@ -629,7 +629,7 @@ describe('insightLogic', () => {
                    }),
                })

-        it('loads from the dashboardLogic when in dashboard context', async () => {
+        it.skip('loads from the dashboardLogic when in dashboard context', async () => {


If these tests are no longer relevant, we should just remove them. It's improbable that we're ever going to re-enable them

Twixes · 2024-04-25T11:56:09Z

frontend/src/scenes/dashboard/dashboardLogic.tsx

@@ -885,7 +861,9 @@ export const dashboardLogic = kea<dashboardLogicType>([
            let refreshesFinished = 0
            let totalResponseBytes = 0

-            const hardRefreshWithoutCache = ['refresh', 'load_missing'].includes(action)
+            const hardRefreshWithoutCache = ['refresh', 'load_missing', 'refresh_insights_on_filters_updated'].includes(


Can we walk me through why refresh_insights_on_filters_updated is added?

That was because upon changing filters, a request to /insight is made with refresh=false which might return null as results. Maybe that's different with your other changes now? But yeah let's check tomorrow.

Twixes · 2024-04-25T11:59:21Z

frontend/__snapshots__/components-cards-insight-card--insight-card--light.png

Are all the changes in results here expected?

"Pageview count" table is no longer time series

The retention tile no longer shows the date range

The stickiness tile now shows "No matching events"

posthog-bot · 2024-04-25T15:04:25Z

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2024-04-25T15:15:23Z

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

webjunkie · 2024-04-26T10:48:35Z

Any idea why one Cypress test now fails?

We rather load fresh to be save

Fixes remaining unwanted propagation of dashboard filter changes to other dashboards.

I first made this test on master, making sure it fails. Passes on this branch!

Twixes · 2024-04-26T13:07:46Z

Found why the UI snapshots were different: mocks reused the same shortId, so resulted in accidental reuse of the same insightLogic instances. Fixed.
As for the failing error, not sure, but should be pretty evident when you run bin/e2e-test-runner. Have you tried that?

webjunkie · 2024-04-26T13:28:05Z

It was all green earlier.. and locally it fails on a different thing for me, which is not helpful 🤔

Twixes · 2024-04-26T15:01:20Z

614068b should fix it.

webjunkie · 2024-05-02T07:43:55Z

This PR fixes (or tries to) #19788 by the way.

webjunkie marked this pull request as ready for review April 11, 2024 10:50

webjunkie requested a review from thmsobrmlr April 11, 2024 10:50

Twixes self-requested a review April 17, 2024 09:12

Twixes reviewed Apr 25, 2024

View reviewed changes

webjunkie and others added 17 commits April 26, 2024 15:06

Remove find insight from mounted logic

d115a7e

We rather load fresh to be save

Do not set insights anymore

8ecebbb

Bring some tests

3a6aca5

Adjust Cypress tests

36e1a3a

Skip unit test

e2dbdb8

Skip unit test

4d0b40c

Try less loading

30c82b3

Adjust dataNodeLogic

90826a0

Make insightLogic not load things on mount when cached

af8028a

Make sure refresh happens on filter change

e3be07e

Make dataNodeLogic never load for dashboard with cached results

389e26d

Fix usage of updateTileOnDashboards

c95623e

Fixes remaining unwanted propagation of dashboard filter changes to other dashboards.

Fix "Remove from dashboard" buttons being out of date

e017744

Add one more E2E test against a frustrating edge case

dba304d

I first made this test on master, making sure it fails. Passes on this branch!

De-only-fy

6851d4e

Adjust and remove test

9a751c8

Fix conflicting insight IDs in Storybook mocks

16e9855

Twixes force-pushed the fix/dashboard-logic-insights branch from 31526a6 to 16e9855 Compare April 26, 2024 13:06

Twixes added 2 commits April 26, 2024 16:26

Merge branch 'master' into fix/dashboard-logic-insights

4427967

Remove fromDashboard from addToDashboardModalLogic

614068b

webjunkie merged commit ebd336f into master Apr 29, 2024
103 checks passed

webjunkie deleted the fix/dashboard-logic-insights branch April 29, 2024 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(insights): Remove insight reuse on dashboard and insights #21471

fix(insights): Remove insight reuse on dashboard and insights #21471

webjunkie commented Apr 10, 2024 •

edited

Loading

posthog-bot commented Apr 10, 2024

github-actions bot commented Apr 10, 2024 •

edited

Loading

webjunkie commented Apr 11, 2024

thmsobrmlr commented Apr 11, 2024

posthog-bot commented Apr 24, 2024

Twixes left a comment

Twixes Apr 25, 2024

Twixes Apr 25, 2024

webjunkie Apr 25, 2024

Twixes Apr 25, 2024

posthog-bot commented Apr 25, 2024

posthog-bot commented Apr 25, 2024

webjunkie commented Apr 26, 2024

Twixes commented Apr 26, 2024 •

edited

Loading

webjunkie commented Apr 26, 2024

Twixes commented Apr 26, 2024

webjunkie commented May 2, 2024

fix(insights): Remove insight reuse on dashboard and insights #21471

fix(insights): Remove insight reuse on dashboard and insights #21471

Conversation

webjunkie commented Apr 10, 2024 • edited Loading

Problem

Changes

Does this work well for both Cloud and self-hosted?

How did you test this code?

posthog-bot commented Apr 10, 2024

📸 UI snapshots have been updated

github-actions bot commented Apr 10, 2024 • edited Loading

webjunkie commented Apr 11, 2024

thmsobrmlr commented Apr 11, 2024

posthog-bot commented Apr 24, 2024

📸 UI snapshots have been updated

Twixes left a comment

Choose a reason for hiding this comment

Twixes Apr 25, 2024

Choose a reason for hiding this comment

Twixes Apr 25, 2024

Choose a reason for hiding this comment

webjunkie Apr 25, 2024

Choose a reason for hiding this comment

Twixes Apr 25, 2024

Choose a reason for hiding this comment

posthog-bot commented Apr 25, 2024

📸 UI snapshots have been updated

posthog-bot commented Apr 25, 2024

📸 UI snapshots have been updated

webjunkie commented Apr 26, 2024

Twixes commented Apr 26, 2024 • edited Loading

webjunkie commented Apr 26, 2024

Twixes commented Apr 26, 2024

webjunkie commented May 2, 2024

webjunkie commented Apr 10, 2024 •

edited

Loading

github-actions bot commented Apr 10, 2024 •

edited

Loading

Twixes commented Apr 26, 2024 •

edited

Loading