feat(data-exploration): bugfixes and improvements #13461

mariusandra · 2022-12-22T08:32:10Z

Problem

Data exploration is not over the line yet, which means it's not live and enabled for some users.

Changes

Tries to take it over the line. In scope (might change):

fix persons search
add "full: true" to the nodes
replace EventsNode with EventsQuery everywhere, except insight nodes
restore load more
restore load new
api request cancellation into dataNodeLogic
add query running time next to "reload" button (behind a flag)

It should again work stable enough that we can enable the flag for ourselves.

Will get to in the next PR

Planned to, but didn't manage to get this in here.

add a date filter on top of the events scene (date_from & date_to !== before & after, need some unfangling)
add date in-between rows - fixes Regressions in new data table component #13223 - but I'm still not convinced we should do it - you either have so many events the day boundary is invisible, or so few that you only see day boundaries every few lines... I'm not sure it makes a lot of sense for both cases to show this, but I'll make something. I'll probably be a hack into LemonTable, renderInBetweenRowOrNull(prev, next) or something like that
convert cells to filters on click

Out of scope for now

Unfangling query_event_list.py and event.py
Add "unique users count" aggregation, and the other new ones

How did you test this code?

…s. Create HogQLExpression

pauldambra

A few thoughts but nothing blocking

(I didn't run this :D)

As discussed at standup really we need at least E2E tests on this. Would be good for the storybook section to have a "how do I start using data exploration" page too

pauldambra · 2022-12-23T09:10:48Z

frontend/src/queries/examples.ts

@@ -74,11 +71,13 @@ const PropertyFormulas: EventsQuery = {

 const PropertyFormulasTable: DataTableNode = {
    kind: NodeKind.DataTableNode,
+    full: true,


Is this something more like showAllTableChrome?

full feels too vague

Normally I'd agree, but in this case I'm hoping we can convert this into a common notation that's easy to use everywhere.

E.g. { kind: 'DataTableNode', full: true, source: {} }, { kind: 'SessionRecording', full: true, id: 'http://' }, { kind: 'InsightViz', full: true, source: {} }

If you pass full: true, you get the full scene with our curated set of widgets and tools. Without full, we just display the element (recording, graph, etc). Then you can control it more with various showBla: true.

I wanted it to be different from the other showOneThing keywords. I thought about "Chrome", but 1) it's a browser sadly and I do know where its name comes from, 2) some of the OtherThings in showOtherThings are not "chrome".

When in doubt, the simpler solution wins, so full it was 😅

pauldambra · 2022-12-23T09:15:55Z

frontend/src/queries/nodes/DataNode/dataNodeLogic.ts

+                        actions.setElapsedTime(performance.now() - now)
+                        return data
+                    } catch (e: any) {
+                        cache.abortController = null


I don't think we need to reset the abortController.

It's safe to call abort after a call is finished, and we reset to a new controller when we start the next API request

pauldambra · 2022-12-23T09:17:02Z

frontend/src/queries/nodes/DataNode/dataNodeLogic.ts

-                loadData: async () => {
-                    return (await query<DataNode>(values.query)) ?? null
+                loadData: async (_, breakpoint) => {
+                    if (cache.abortController) {


We also need to be able to trigger the abort from outside the component. There are two triggers

(covered here) a new API request is started

In case of navigating away from a page

I added an abortQuery action

pauldambra · 2022-12-23T09:17:51Z

frontend/src/queries/nodes/DataNode/dataNodeLogic.ts

+                    } catch (e: any) {
+                        cache.abortController = null
+                        if (e.name === 'AbortError' || e.message?.name === 'AbortError') {
+                            return values.response


we also need to call in to the cancel API with a query id when the API call is cancelled so we can release any ongoing ClickHouse resource usage

And we need to make sure this is reporting timeToSee data too.

Although fine to do all of this as follow-up since this is behind a flag

happy to do this in a followup 😅

pauldambra · 2022-12-23T09:21:39Z

frontend/src/queries/nodes/DataNode/dataNodeLogic.ts

        if (values.autoLoadRunning) {
            actions.stopAutoLoad()
        }
+        if (cache.abortController) {


For simplicity, I've removed this elsewhere. In practice turbo mode means we're not unmounting the logics early enough to make a difference

hmm... makes sense, though if all data goes through this dataNodeLogic, then we'll have cases where cancelling this before unmount will pay off... So I'd keep it. I changed it a bit to go through a new abortQuery action

mariusandra · 2022-12-23T14:35:59Z

Thanks for the reviews! I cleaned up a few things, and already had to merge with master. Since this PR lightly touches a lot of files (most of which are behind a flag), and fixes a few bad bugs, I'd happily merge this in and volunteer for oncall support for it over the next days 😅. Otherwise I fear work on other data exploration tasks next week will yield in a lot of annoying merge conflicts...

* update schema * slight touchups * always use the latest query from props * useCallback to memoise setQuerySource * handroll some debouncing * add full:true * fixes * make only EventsQuery-s queriable. EventsNode is just used in insights. Create HogQLExpression * yeet EventsNode from data table, support loading next results * get new results working again * remove default sorting if editing the live events page * fix test * query cancellation * show query time in the interface * load next people * more fixes * more fixes * last touchers * fix sorting again * combine some aborting

mariusandra added 5 commits December 21, 2022 22:51

update schema

8be0d68

slight touchups

aecff22

always use the latest query from props

f4c517a

useCallback to memoise setQuerySource

6428b1a

handroll some debouncing

3b29018

mariusandra marked this pull request as draft December 22, 2022 08:32

mariusandra added 10 commits December 22, 2022 10:40

add full:true

9bd7b8e

fixes

9d808f3

make only EventsQuery-s queriable. EventsNode is just used in insight…

5a5370f

…s. Create HogQLExpression

yeet EventsNode from data table, support loading next results

9ce7337

get new results working again

e0a89be

remove default sorting if editing the live events page

690f730

fix test

0afd620

query cancellation

2986338

show query time in the interface

b3789c4

load next people

9b7fd65

mariusandra marked this pull request as ready for review December 22, 2022 15:49

mariusandra added 3 commits December 22, 2022 16:51

more fixes

7c606aa

more fixes

e321c7a

last touchers

b24fde7

mariusandra requested review from thmsobrmlr and pauldambra and removed request for thmsobrmlr December 22, 2022 16:03

mariusandra added 2 commits December 22, 2022 17:04

Merge branch 'master' into data-exploration-final-vol2-thisisit--2

7e6f00e

Merge branch 'master' into data-exploration-final-vol2-thisisit--2

2f95904

mariusandra mentioned this pull request Dec 22, 2022

feat: test query cancellation #13459

Merged

fix sorting again

e294937

pauldambra approved these changes Dec 23, 2022

View reviewed changes

mariusandra requested a review from thmsobrmlr December 23, 2022 12:42

thmsobrmlr approved these changes Dec 23, 2022

View reviewed changes

mariusandra added 2 commits December 23, 2022 15:30

combine some aborting

24357fe

Merge branch 'master' into data-exploration-final-vol2-thisisit--2

b00384c

mariusandra merged commit a4ab4ea into master Dec 23, 2022

mariusandra deleted the data-exploration-final-vol2-thisisit--2 branch December 23, 2022 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(data-exploration): bugfixes and improvements #13461

feat(data-exploration): bugfixes and improvements #13461

mariusandra commented Dec 22, 2022 •

edited

Loading

pauldambra left a comment

pauldambra Dec 23, 2022

mariusandra Dec 23, 2022 •

edited

Loading

pauldambra Dec 23, 2022

pauldambra Dec 23, 2022

mariusandra Dec 23, 2022

pauldambra Dec 23, 2022

pauldambra Dec 23, 2022

mariusandra Dec 23, 2022

pauldambra Dec 23, 2022

mariusandra Dec 23, 2022

mariusandra commented Dec 23, 2022

feat(data-exploration): bugfixes and improvements #13461

feat(data-exploration): bugfixes and improvements #13461

Conversation

mariusandra commented Dec 22, 2022 • edited Loading

Problem

Changes

Will get to in the next PR

Out of scope for now

How did you test this code?

pauldambra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra Dec 23, 2022 • 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

Choose a reason for hiding this comment

mariusandra commented Dec 23, 2022

mariusandra commented Dec 22, 2022 •

edited

Loading

mariusandra Dec 23, 2022 •

edited

Loading