[Clickhouse] Event list #1787

timgl · 2020-10-01T10:32:43Z

Changes

Please describe.
If this affects the front-end, include screenshots.

Checklist

All querysets/queries filter by Team (if this PR affects any querysets/queries)
Backend tests (if this PR affects the backend)
Cypress E2E tests (if this PR affects the front and/or backend)

…query if opening an url with a filter

…(was: show sign if exactly LIMIT fetched)

…6-2-event-list * origin/clickhouse-sessions-qa: (24 commits) remove reverse add default time to fix test, fix some any(*) filter issues fix test mypy fix pagination, timestamp calculation and ordering on pages 2 and beyond show seconds with two digits add person properties to sessions query response add elements to query results events in clickhouse sessions default limit to 50 based offset is always 0 load LIMIT+1 sessions in postgres, pop last and show load more sign. (was: show sign if exactly LIMIT fetched) support limit + offset indent sql initial limit/offset block rename offset to nextOffset date from filters throw error if failed instead of returning an empty list prevent multiple queries load sessions based on the URL, not after mount --> avoids duplicate query if opening an url with a filter ...

yakkomajuri · 2020-10-01T11:57:36Z

ee/docker-compose.ch.yml

@@ -32,7 +32,7 @@ services:
            CLICKHOUSE_VERIFY: 'False'
            KAFKA_URL: 'kafka://kafka'
            REDIS_URL: 'redis://redis:6379/'
-            SECRET_KEY: '<randomly generated secret key>'
+            SECRET_KEY: 'alsdfjiosdajfklalsdjkf'


Indeed, having errors thrown because of <randomly generated secret key> is annoying, but the point is that users then know they should update for their own safety. Perhaps moving towards @Twixes approach from #1704 is best, while keeping <randomly generated secret key> here for safety.

For what it's worth only we will be using this docker-compose file. This is not for general consumption.

Hmm yeah that's true... I guess it's not a problem then

Ah yep I didn't mean to commit this but it was annoying. If James G is happy I can leave this in

yeah doesn't bother me any

fuziontech

Looks awesome. Send it! (with comments lol)

fuziontech · 2020-10-01T12:22:18Z

bin/docker-dev-web

@@ -3,4 +3,4 @@
 set -e
 python manage.py migrate

-python manage.py runserver 0.0.0.0:8000 & ./bin/start-frontend 0.0.0.0
+python manage.py runserver 0.0.0.0:8000 & ./bin/start-frontend 127.0.0.1


Nothing hits what yarn run start-http runs correct? If so this is great

fuziontech · 2020-10-01T12:22:59Z

dev.Dockerfile

+# install dev dependencies 
+RUN pip install -r requirements/dev.txt --compile
+


fuziontech · 2020-10-01T12:25:12Z

ee/clickhouse/models/property.py

@@ -13,7 +13,7 @@ def parse_filter(filters: List[Property]) -> Tuple[str, Dict]:
    result = ""
    params = {}
    for idx, prop in enumerate(filters):
-        result += "{cond}(ep.key = %(k{idx})s) AND (ep.value = %(v{idx})s)".format(
+        result += "{cond}(ep.key = %(k{idx})s) AND (trim(BOTH '\"' FROM ep.value) = %(v{idx})s)".format(


I'd leave this out for now. It's better to add this to the materialized view on clickhouse so that we don't have to clean anything up in post.

Hm tests fail/you won't be able to filter without this. Is it easy to add it to materliazed view?

Let's keep this in for now... but I'd like to get that solved at the source as quickly as possible. When we change serialization from JSON to Avro or Proto this will be cleaned up upstream

fuziontech · 2020-10-01T12:27:22Z

ee/clickhouse/sql/events.py

+    events_with_array_props_view ewap
+INNER JOIN person_distinct_id as pid ON ewap.distinct_id = pid.distinct_id
+INNER JOIN person ON pid.person_id = person.id
+where ewap.team_id = %(team_id)s


nit: inConSISTent CapS

fuziontech · 2020-10-01T12:30:07Z

ee/clickhouse/views/events.py

 from posthog.utils import convert_property_value


 class ClickhouseEvents(EventViewSet):
-    def list(self, request):
+    def _get_elements(self, query_result: List[Dict], team: Team) -> Dict[str, Any]:
+        element_hashes = [event[6] for event in query_result]


I really don't like that we are pulling out hashes by index 6 here. Has a bit of a smell. Not a huge deal though. If we did this in a few spots it would make it really hard to refactor the query and reorder columns.

Would be better if we had event.element_hash on the row. Something we can work towards.

Yeah agree, need to solve this. surprised the driver doesn't give you a dict or object

fuziontech · 2020-10-01T12:38:01Z

ee/clickhouse/sql/events.py

+INNER JOIN person_distinct_id as pid ON ewap.distinct_id = pid.distinct_id
+INNER JOIN person ON pid.person_id = person.id
+where ewap.team_id = %(team_id)s
+AND ewap.uuid IN (select uuid from events WHERE team_id = %(team_id)s {conditions})


nit: inConSISTent CapS

fuziontech · 2020-10-01T12:38:07Z

ee/clickhouse/sql/events.py

-) {conditions} {limit}
+    WHERE team_id = %(team_id)s AND {filters}
+)
+AND ewap.uuid IN (select uuid from events WHERE team_id = %(team_id)s {conditions})


nit: inConSISTent CapS

fuziontech · 2020-10-01T12:39:24Z

posthog/api/test/test_event.py

-        )
-
-        with self.assertNumQueries(7):
+def test_event_api_factory(event_factory, person_factory, action_factory):


👀 very nice

* origin: Always limit events (#1794) Fix ambiguous timestamp ordering (#1792) Fix dev docker build (#1791) Create CODE_OF_CONDUCT.md (#1790) [Clickhouse] Event list (#1787) made shared_dashboards endpoint exempt from x-frame-options header (#1789)

mariusandra and others added 30 commits September 29, 2020 16:39

convert sessions table logic to TS

bccca29

convert rest of sessions to TS

4209eba

sessions table logic refactor, store date in the url

ae6d713

add back/forward buttons

da2b6ce

load sessions based on the URL, not after mount --> avoids duplicate …

3fe5f83

…query if opening an url with a filter

prevent multiple queries

7215fa8

throw error if failed instead of returning an empty list

49d2d57

date from filters

5cfbb2b

rename offset to nextOffset

6852c8f

initial limit/offset block

741bd1a

indent sql

85b0ff9

support limit + offset

d5aa0bf

load LIMIT+1 sessions in postgres, pop last and show load more sign. …

1720426

…(was: show sign if exactly LIMIT fetched)

based offset is always 0

5421f09

default limit to 50

d20ed85

events in clickhouse sessions

97e6803

add elements to query results

cd9d242

add person properties to sessions query response

a0286f3

show seconds with two digits

52ad201

fix pagination, timestamp calculation and ordering on pages 2 and beyond

a84d229

mypy

f35a0ab

fix test

246ea80

add default time to fix test, fix some any(*) filter issues

d5db609

remove reverse

1a03af0

WIP event list

03830df

merge master

d6374cc

Events progress

5b57e84

Finish off event listing, skip live actions for now

b715c5c

merge master

e3a0842

timgl changed the title ~~1736 2 event list~~ [Clickhouse] Event list Oct 1, 2020

timgl had a problem deploying to posthog-1736-2-event-li-keekxx October 1, 2020 10:33 Failure

Fix mypy

cafd633

timgl temporarily deployed to posthog-1736-2-event-li-keekxx October 1, 2020 10:37 Inactive

Fix mypy again

f7df74d

timgl temporarily deployed to posthog-1736-2-event-li-9swjbp October 1, 2020 10:41 Inactive

Try fixing mypy

b57d0d9

timgl temporarily deployed to posthog-1736-2-event-li-8die7n October 1, 2020 10:48 Inactive

EDsCODE added the clickhouse label Oct 1, 2020

yakkomajuri reviewed Oct 1, 2020

View reviewed changes

Fix assertnumqueries

11dbb6b

timgl temporarily deployed to posthog-1736-2-event-li-12haks October 1, 2020 12:38 Inactive

fuziontech approved these changes Oct 1, 2020

View reviewed changes

Fix tests

afd417e

timgl temporarily deployed to posthog-1736-2-event-li-y5cgkf October 1, 2020 12:59 Inactive

Fix tests

e955a74

timgl had a problem deploying to posthog-1736-2-event-li-nihomc October 1, 2020 13:20 Failure

fix test

e4e996c

timgl temporarily deployed to posthog-1736-2-event-li-nihomc October 1, 2020 13:25 Inactive

Fix tests

c81fd7d

timgl temporarily deployed to posthog-1736-2-event-li-dnsz7t October 1, 2020 13:31 Inactive

Fix tests

2a80de4

timgl had a problem deploying to posthog-1736-2-event-li-wf3nf9 October 1, 2020 13:41 Failure

Fix tests again

328d5b2

timgl temporarily deployed to posthog-1736-2-event-li-wf3nf9 October 1, 2020 13:42 Inactive

timgl merged commit 92e8bbd into master Oct 1, 2020

timgl deleted the 1736-2-event-list branch October 1, 2020 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clickhouse] Event list #1787

[Clickhouse] Event list #1787

timgl commented Oct 1, 2020

yakkomajuri Oct 1, 2020

fuziontech Oct 1, 2020

yakkomajuri Oct 1, 2020

timgl Oct 1, 2020

timgl Oct 1, 2020

fuziontech Oct 1, 2020

fuziontech left a comment

fuziontech Oct 1, 2020

fuziontech Oct 1, 2020

fuziontech Oct 1, 2020

timgl Oct 1, 2020 •

edited

Loading

fuziontech Oct 1, 2020

fuziontech Oct 1, 2020

fuziontech Oct 1, 2020

timgl Oct 1, 2020

fuziontech Oct 1, 2020

fuziontech Oct 1, 2020

fuziontech Oct 1, 2020

		# install dev dependencies
		RUN pip install -r requirements/dev.txt --compile

[Clickhouse] Event list #1787

[Clickhouse] Event list #1787

Conversation

timgl commented Oct 1, 2020

Changes

Checklist

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

fuziontech left a 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

timgl Oct 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgl Oct 1, 2020 •

edited

Loading