fix(insights): HogQL calculation of saved legacy insights v2 #21595

Twixes · 2024-04-17T07:49:23Z

Changes

Revert of #21590, with ~~one change~~ two changes added:

insight serialization will now only use cached results, to match the behavior of the legacy system, preventing ClickHouse overload (5c295c8)
dashboard filters are now taken into account, with proper validation

posthog/hogql_queries/query_runner.py

posthog/api/insight.py

Twixes · 2024-04-18T14:58:58Z

package.json

@@ -34,7 +34,8 @@
        "build:esbuild": "node frontend/build.mjs",
        "schema:build": "pnpm run schema:build:json && pnpm run schema:build:python",
        "schema:build:json": "ts-node bin/build-schema.mjs && prettier --write frontend/src/queries/schema.json",
-        "schema:build:python": "datamodel-codegen --class-name='SchemaRoot' --collapse-root-models --target-python-version 3.10 --disable-timestamp --use-one-literal-as-default --use-default --use-default-kwarg --use-subclass-enum --input frontend/src/queries/schema.json --input-file-type jsonschema --output posthog/schema.py --output-model-type pydantic_v2.BaseModel && ruff format posthog/schema.py",
+        "schema:build:python": "datamodel-codegen --class-name='SchemaRoot' --collapse-root-models --target-python-version 3.10 --disable-timestamp --use-one-literal-as-default --use-default --use-default-kwarg --use-subclass-enum --input frontend/src/queries/schema.json --input-file-type jsonschema --output posthog/schema.py --output-model-type pydantic_v2.BaseModel && ruff format posthog/schema.py && pnpm schema:build:python:fix-up-enum",
+        "schema:build:python:fix-up-enum": "sed -i '' -e 's/Optional\\[PropertyOperator\\] = \\(\"[A-Za-z_]*\"\\)/Optional[PropertyOperator] = PropertyOperator(\\1)/g' posthog/schema.py # THIS SED INVOCATION IS A MASSIVE HACK - remove when datamodel-codegen properly supports enums",


Context: We need EventPropertyFilter.operator to have a default value, to support the historic assumption that "exact" is the default operator. But that default value needs to be an enum member (PropertyOperator.exact), which datamodel-codegen doesn't support

Should we move that into a shell file? (See build:json above where I also moved to a file cause of more logic)

Yeah, that's definitely a good idea at this point! Done

github-actions · 2024-04-18T15:03:39Z

Size Change: 0 B

Total Size: 1.01 MB

ℹ️ View Unchanged

Filename	Size
`frontend/dist/toolbar.js`	1.01 MB

_{compressed-size-action}

This reverts commit a6314c6.

Wow, this was a pain to figure out, only was an issue in CI, because the trigger was `TestCohort::test_creating_update_and_calculating_with_new_cohort_query` running prior to `TestInsight:: test_insight_refreshing_query` – had to use trial and error.

Matters in CI.

webjunkie

Seems alright in general. Some notes though.

Another thing I'm wondering for later:
If I go to an insight to view it, and it has been updated "1 day ago". I can click refresh there and it'll make just the request to /query and then tell me "Computed a few seconds ago". But since we don't store this in the insight, upon refresh it'll be a day old again.
Not sure how to tie this in together... we probably cannot be sure the query is exactly the umodified insight and such 😩

webjunkie · 2024-04-19T09:55:32Z

package.json

@@ -34,7 +34,8 @@
        "build:esbuild": "node frontend/build.mjs",
        "schema:build": "pnpm run schema:build:json && pnpm run schema:build:python",
        "schema:build:json": "ts-node bin/build-schema.mjs && prettier --write frontend/src/queries/schema.json",
-        "schema:build:python": "datamodel-codegen --class-name='SchemaRoot' --collapse-root-models --target-python-version 3.10 --disable-timestamp --use-one-literal-as-default --use-default --use-default-kwarg --use-subclass-enum --input frontend/src/queries/schema.json --input-file-type jsonschema --output posthog/schema.py --output-model-type pydantic_v2.BaseModel && ruff format posthog/schema.py",
+        "schema:build:python": "datamodel-codegen --class-name='SchemaRoot' --collapse-root-models --target-python-version 3.10 --disable-timestamp --use-one-literal-as-default --use-default --use-default-kwarg --use-subclass-enum --input frontend/src/queries/schema.json --input-file-type jsonschema --output posthog/schema.py --output-model-type pydantic_v2.BaseModel && ruff format posthog/schema.py && pnpm schema:build:python:fix-up-enum",
+        "schema:build:python:fix-up-enum": "sed -i '' -e 's/Optional\\[PropertyOperator\\] = \\(\"[A-Za-z_]*\"\\)/Optional[PropertyOperator] = PropertyOperator(\\1)/g' posthog/schema.py # THIS SED INVOCATION IS A MASSIVE HACK - remove when datamodel-codegen properly supports enums",


Should we move that into a shell file? (See build:json above where I also moved to a file cause of more logic)

webjunkie · 2024-04-19T10:00:02Z

posthog/api/test/test_insight.py

+                ],
+            )
+            self.assertEqual(response["last_refresh"], "2012-01-16T05:01:34Z")
+            self.assertEqual(response["last_modified_at"], "2012-01-15T04:01:34Z")  # did not change


Wait, does that mean it didn't change from the non-dashboard one? Shouldn't it be different? Or it didn't change since creation? In the latter case we might adjust the case to make it clear we get a different not-change for dashboards 🙈

I think this is pretty valid, because the underlying definition of the insight hasn't changed. The dashboard config is just overlaid on top and kind of ephemeral. Unless I misunderstood?

webjunkie · 2024-04-19T10:21:00Z

posthog/hogql_queries/query_runner.py

@@ -313,8 +335,12 @@ def run(self, refresh_requested: Optional[bool] = None) -> CachedQueryResponse:
                    return cached_response
                else:
                    QUERY_CACHE_HIT_COUNTER.labels(team_id=self.team.pk, cache_hit="stale").inc()
+                    if execution_mode == ExecutionMode.CACHE_ONLY:
+                        return cached_response


Do we need to set is_cached = True here as well? 🤔

Hmm, yes, reworked and hopefully clarified this code overall

Clarifies the `cached_response.is_cached = True` situation.

posthog-bot · 2024-04-19T13:27:55Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Twixes · 2024-04-19T18:14:11Z

But since we don't store this in the insight, upon refresh it'll be a day old again.

This is exactly what should be addressed here. Just added a test to make sure the two endpoints indeed use the same cache, unlike previously.

sentry-io · 2024-04-22T08:49:39Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ CHQueryErrorUnknownIdentifier: Missing columns: '--events__override.person_id' '--events__override.distinct_id' while processi... /api/projects/{parent_lookup_team_id}/insights/... View Issue
‼️ ValidationError: 2 validation errors for PropertyGroupFilter /api/projects/{parent_lookup_team_id}/dashboard... View Issue
‼️ ValidationError: 2 validation errors for PropertyGroupFilter /api/projects/{parent_lookup_team_id}/insights/... View Issue
‼️ ValidationError: 2 validation errors for PropertyGroupFilter /api/projects/{parent_lookup_team_id}/dashboard... View Issue
‼️ NotImplementedError: EventsQuery does not support dashboard filters out of the box /api/projects/{parent_lookup_team_id}/insights/... View Issue

_{Did you find this useful? React with a 👍 or 👎}

…21595)" This reverts commit 3e2d28f.

Revert "fix(insights): HogQL calculation of saved legacy insights v2 (#21595)" This reverts commit 3e2d28f.

Twixes requested review from timgl and a team April 17, 2024 07:49

webjunkie reviewed Apr 17, 2024

View reviewed changes

posthog/hogql_queries/query_runner.py Outdated Show resolved Hide resolved

Twixes force-pushed the hogql-calculation-bis branch from bb87101 to 7a8db16 Compare April 17, 2024 13:29

Twixes requested a review from webjunkie April 17, 2024 17:27

Twixes force-pushed the hogql-calculation-bis branch from aa0de3e to f4db229 Compare April 17, 2024 20:34

PostHog deleted a comment from posthog-bot Apr 17, 2024

PostHog deleted a comment from github-actions bot Apr 17, 2024

github-advanced-security bot found potential problems Apr 17, 2024

View reviewed changes

posthog/api/insight.py Dismissed Show dismissed Hide dismissed

Twixes force-pushed the hogql-calculation-bis branch from d404e98 to 92d44d7 Compare April 17, 2024 22:14

Twixes mentioned this pull request Apr 18, 2024

refactor(querying): Consolidate PersonOnEventsMode and PersonsOnEventsMode #21628

Merged

Twixes force-pushed the hogql-calculation-bis branch from 3054a3e to 230975e Compare April 18, 2024 14:23

Twixes commented Apr 18, 2024

View reviewed changes

PostHog deleted a comment from posthog-bot Apr 18, 2024

PostHog deleted a comment from github-actions bot Apr 18, 2024

Twixes force-pushed the hogql-calculation-bis branch from ce321cd to 79a61d3 Compare April 18, 2024 16:50

Twixes added 11 commits April 19, 2024 00:01

fix(insights): HogQL calculation of saved legacy insights v2

e49394b

This reverts commit a6314c6.

Only use cached results in process_query for insight serializer

72f47eb

Fix type of results

41c2387

Rename RecalculationMode to ExecutionMode

8f657d6

Fix typing more

476f4f8

Properly support dashboard filters

f70aa7f

Hacky fix for schema.py

1f3d863

Don't test legacy generate_insight_cache_key with query

bc0e01c

Fix importing & typing

d9c7779

Fix typo

4d250a8

Update test_query_runner.py

0f44d39

Twixes added 4 commits April 19, 2024 00:01

Do return stale result in CACHE_ONLY case

d431127

Fix execute_hogql_query espionage

932e10a

Wow, this was a pain to figure out, only was an issue in CI, because the trigger was `TestCohort::test_creating_update_and_calculating_with_new_cohort_query` running prior to `TestInsight:: test_insight_refreshing_query` – had to use trial and error.

Fix typing even more

f67a764

Don't require pnpm for schema:build:python

4770169

Matters in CI.

Twixes force-pushed the hogql-calculation-bis branch from 281d302 to 4770169 Compare April 18, 2024 22:01

PostHog deleted a comment from posthog-bot Apr 18, 2024

Twixes added 3 commits April 19, 2024 10:09

Merge branch 'master' into hogql-calculation-bis

3abf5ea

Fix schema:build:python

ea3582c

Fix sed usage

6e579f5

webjunkie approved these changes Apr 19, 2024

View reviewed changes

Move schema:build:python to a bash script

1917ed0

Twixes force-pushed the hogql-calculation-bis branch from 0fdf1e2 to af5670b Compare April 19, 2024 12:44

Validate cache properly

e457fd4

Clarifies the `cached_response.is_cached = True` situation.

Twixes force-pushed the hogql-calculation-bis branch from af5670b to e457fd4 Compare April 19, 2024 13:15

Twixes and others added 3 commits April 19, 2024 15:17

Merge branch 'master' into hogql-calculation-bis

89f0e8a

Fix Python formatting

18e7cdb

Update UI snapshots for webkit (2)

fe1003b

Twixes added 2 commits April 19, 2024 20:12

Add test to ensure /query/ and /inisghts/ use the same cache

77c8edb

Update mypy-baseline.txt

0d86acb

Twixes force-pushed the hogql-calculation-bis branch from 239b23b to 0d86acb Compare April 19, 2024 18:13

Twixes merged commit 3e2d28f into master Apr 22, 2024
106 checks passed

Twixes deleted the hogql-calculation-bis branch April 22, 2024 08:30

Twixes added a commit that referenced this pull request Apr 22, 2024

Revert "fix(insights): HogQL calculation of saved legacy insights v2 (#…

84bd309

…21595)" This reverts commit 3e2d28f.

Twixes mentioned this pull request Apr 22, 2024

revert(insights): HogQL calculation of saved legacy insights v2 #21718

Merged

Twixes added a commit that referenced this pull request Apr 22, 2024

revert(insights): HogQL calculation of saved legacy insights v2 (#21718)

2f019a3

Revert "fix(insights): HogQL calculation of saved legacy insights v2 (#21595)" This reverts commit 3e2d28f.

Twixes mentioned this pull request Apr 22, 2024

test(e2e): Treat HogQL insights as fully rolled out #21722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(insights): HogQL calculation of saved legacy insights v2 #21595

fix(insights): HogQL calculation of saved legacy insights v2 #21595

Twixes commented Apr 17, 2024 •

edited

Loading

Twixes Apr 18, 2024

webjunkie Apr 19, 2024

Twixes Apr 19, 2024

github-actions bot commented Apr 18, 2024 •

edited

Loading

webjunkie left a comment •

edited

Loading

webjunkie Apr 19, 2024

webjunkie Apr 19, 2024

Twixes Apr 19, 2024 •

edited

Loading

webjunkie Apr 19, 2024

Twixes Apr 19, 2024

posthog-bot commented Apr 19, 2024

Twixes commented Apr 19, 2024

sentry-io bot commented Apr 22, 2024 •

edited

Loading

fix(insights): HogQL calculation of saved legacy insights v2 #21595

fix(insights): HogQL calculation of saved legacy insights v2 #21595

Conversation

Twixes commented Apr 17, 2024 • edited Loading

Changes

Twixes Apr 18, 2024

Choose a reason for hiding this comment

webjunkie Apr 19, 2024

Choose a reason for hiding this comment

Twixes Apr 19, 2024

Choose a reason for hiding this comment

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

webjunkie left a comment • edited Loading

Choose a reason for hiding this comment

webjunkie Apr 19, 2024

Choose a reason for hiding this comment

webjunkie Apr 19, 2024

Choose a reason for hiding this comment

Twixes Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

webjunkie Apr 19, 2024

Choose a reason for hiding this comment

Twixes Apr 19, 2024

Choose a reason for hiding this comment

posthog-bot commented Apr 19, 2024

📸 UI snapshots have been updated

Twixes commented Apr 19, 2024

sentry-io bot commented Apr 22, 2024 • edited Loading

Suspect Issues

Twixes commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 18, 2024 •

edited

Loading

webjunkie left a comment •

edited

Loading

Twixes Apr 19, 2024 •

edited

Loading

sentry-io bot commented Apr 22, 2024 •

edited

Loading