Average Scroll Depth Metric: imported data #4915
Conversation
RobertJoonas
force-pushed
the
scroll-depth-full-csv
branch
from
a083472 to
03327af
Compare
RobertJoonas
changed the title
RobertJoonas
force-pushed
the
scroll-depth-full-csv
branch
from
dbde2ce to
8543cee
Compare
RobertJoonas
changed the title
|
RobertJoonas
force-pushed
the
scroll-depth-full-csv
branch
from
8543cee to
9470502
Compare
RobertJoonas
force-pushed
the
scroll-depth-full-csv
branch
from
9470502 to
1d7beb7
Compare
| ) | ||
| |> group_by([e], [selected_as(:date), selected_as(:page)]) | ||
| |> order_by([e], selected_as(:date)) | ||
| else |
There was a problem hiding this comment.
I feel like both the if and else clauses have the same basic query from events_v2. Maybe there's an opportunity to extract it outside the conditional
There was a problem hiding this comment.
Agreed. I've extracted a base_q in 7b5eeeb. The select clause however is a bit tricky to extract because we want to select a list. One possible solution is to select_merge which requires the select to be a map, but then we'd still need to convert it back to a list in the end. I figured it's not worth the hustle as the FF is not expected to be there forever.
| @@ -345,6 +345,7 @@ defmodule Plausible.Stats.Imported do | |||
|
|
|||
| defp can_order_by?(query) do | |||
| Enum.all?(query.order_by, fn | |||
| {:scroll_depth, _} -> false | |||
There was a problem hiding this comment.
why is ordering by scroll depth prevented?
There was a problem hiding this comment.
Because since scroll_depth is a "special metric", it's not yet selected into the query yet at this point, and any attempt to order by it will result in an invalid query.
| fragment( | ||
| """ | ||
| case | ||
| when isNotNull(?) AND isNotNull(?) then |
There was a problem hiding this comment.
Can we not instead return 0 values when scroll depth is missing from either side of the join? Other words - can we avoid this big conditional somehow? There's nothing wrong with the conditonal itself but I find managing raw fragments like this is a pain because of how the params must be passed.
There was a problem hiding this comment.
The fragment is pretty hideous indeed, but I'm afraid there's no way around it if we want the query to explicitly return NULL when there's no data. The main problem here is that zero can be a valid scroll_depth value.
| defp select_metric(:scroll_depth, "imported_pages") do | ||
| wrap_alias([i], %{ | ||
| scroll_depth_sum: sum(i.scroll_depth), | ||
| total_visitors: sum(i.pageleave_visitors) |
There was a problem hiding this comment.
Is there any need/benefit to renaming pageleave_visitors to total_visitors here? Could be a bit misleading
There was a problem hiding this comment.
💯 Agreed. It's misleading and could be confused with total_visitors used in the calculation of conversion rate. I've renamed it to pageleave_visitors all around - including the native query. 3203218
Changes
This PR starts making use of two new columns in the
imported_pagestable. The relevant migrations are already merged and executed:pageleave_visitorsfield)The objectives of this PR are to:
scroll_depthmetric in the full CSV export/import featurescroll_depthfrom imported data (if it exists) in stats queriesReturning warnings about imported data not being included in scroll_depth calculation will be implemented in a follow up PR.
Tests
Changelog
Documentation
Dark mode