Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Page Scroll Goals #5029

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ecba7d7
migration: add scroll_threshold to goals
RobertJoonas Oct 31, 2024
ecbdc8e
update goal schema
RobertJoonas Jan 24, 2025
d5631de
setup simple UI for creating scroll goals
RobertJoonas Jan 24, 2025
58e930f
add ability to filter and breakdown scroll goals
RobertJoonas Jan 29, 2025
6c30e0f
fix goals form tests
RobertJoonas Jan 29, 2025
7efca92
add valiation for page path exists
RobertJoonas Jan 29, 2025
d19fe65
move todo comments to expression.ex
RobertJoonas Jan 29, 2025
1848d44
move tests
RobertJoonas Jan 29, 2025
5bef111
make it clear that scroll_threshold is optional
RobertJoonas Jan 29, 2025
ec5f801
avoid calling Plausible.Goal.type() too many times
RobertJoonas Jan 29, 2025
1ac92fb
do not consider 255 scroll depth a conversion
RobertJoonas Jan 29, 2025
08883b0
migration: add scroll_threshold to goals
RobertJoonas Oct 31, 2024
0439793
do not drop the old index yet
RobertJoonas Jan 30, 2025
dbaf9b3
More efficient goals join again
macobo Jan 30, 2025
8f28b28
Refactor: move goals stats code explicitly under Stats.Goals module
macobo Jan 30, 2025
7e2e780
Move code under Plausible.Stats.Goals
macobo Jan 30, 2025
ebf0dfb
254 -> 100
macobo Jan 30, 2025
c475382
Merge branch 'master' into page-scroll-goals-migration
RobertJoonas Jan 30, 2025
82e70dc
add scroll_threshold field to goal schema + new unique constraint
RobertJoonas Jan 30, 2025
3d0a131
adjust test to test what it claims to
RobertJoonas Jan 30, 2025
72e2934
mix format
RobertJoonas Jan 30, 2025
d17893d
add migration
RobertJoonas Jan 30, 2025
890690e
consider imported query unsupported when page scroll goal filter
RobertJoonas Feb 3, 2025
f8914e6
add missing tests
RobertJoonas Feb 3, 2025
9c51ba8
pattern match imported argument
RobertJoonas Feb 3, 2025
0becb4a
silence credo
RobertJoonas Feb 3, 2025
0da6616
Merge branch 'master' into page-scroll-goals
RobertJoonas Feb 3, 2025
537d22a
Merge branch 'master' into remove-goals-page-path-unique-index-migration
RobertJoonas Feb 3, 2025
1896791
Update lib/plausible/stats/sql/expression.ex
RobertJoonas Feb 3, 2025
a8cda3c
Merge with migration step 3 (PR #5036)
RobertJoonas Feb 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions lib/plausible/goal/schema.ex
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
defmodule Plausible.Goal do

Check failure on line 1 in lib/plausible/goal/schema.ex

View workflow job for this annotation

GitHub Actions / App code does not change at the same time

Code and migrations shouldn't be changed at the same time
use Plausible
use Ecto.Schema
import Ecto.Changeset
Expand All @@ -8,6 +8,7 @@
schema "goals" do
field :event_name, :string
field :page_path, :string
field :scroll_threshold, :integer, default: -1
field :display_name, :string

on_ee do
Expand All @@ -23,7 +24,7 @@
timestamps()
end

@fields [:id, :site_id, :event_name, :page_path, :display_name] ++
@fields [:id, :site_id, :event_name, :page_path, :scroll_threshold, :display_name] ++
on_ee(do: [:currency], else: [])

@max_event_name_length 120
Expand All @@ -37,11 +38,19 @@
|> cast_assoc(:site)
|> update_leading_slash()
|> validate_event_name_and_page_path()
|> validate_page_path_for_scroll_goal()
|> maybe_put_display_name()
|> unique_constraint(:event_name, name: :goals_event_name_unique)
|> unique_constraint(:page_path, name: :goals_page_path_unique)
|> unique_constraint([:page_path, :scroll_threshold],
name: :goals_page_path_and_scroll_threshold_unique
)
|> unique_constraint(:display_name, name: :goals_site_id_display_name_index)
|> validate_length(:event_name, max: @max_event_name_length)
|> validate_number(:scroll_threshold,
greater_than_or_equal_to: -1,
less_than_or_equal_to: 100,
message: "Should be -1 (missing) or in range [0, 100]"
)
|> check_constraint(:event_name,
name: :check_event_name_or_page_path,
message: "cannot co-exist with page_path"
Expand All @@ -54,9 +63,14 @@
goal.display_name
end

@spec type(t()) :: :event | :page
def type(%{event_name: event_name}) when is_binary(event_name), do: :event
def type(%{page_path: page_path}) when is_binary(page_path), do: :page
@spec type(t()) :: :event | :scroll | :page
def type(goal) do
cond do
is_binary(goal.event_name) -> :event
is_binary(goal.page_path) && goal.scroll_threshold > -1 -> :scroll
is_binary(goal.page_path) -> :page
end
end

defp update_leading_slash(changeset) do
case get_field(changeset, :page_path) do
Expand Down Expand Up @@ -86,6 +100,18 @@
end
end

defp validate_page_path_for_scroll_goal(changeset) do
scroll_threshold = get_field(changeset, :scroll_threshold)
page_path = get_field(changeset, :page_path)

if scroll_threshold > -1 and is_nil(page_path) do
changeset
|> add_error(:scroll_threshold, "page_path field missing for page scroll goal")
else
changeset
end
end

defp validate_page_path(changeset) do
value = get_field(changeset, :page_path)

Expand Down
2 changes: 1 addition & 1 deletion lib/plausible/stats/filters/query_parser.ex
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ defmodule Plausible.Stats.Filters.QueryParser do

def preload_goals_and_revenue(site, metrics, filters, dimensions) do
preloaded_goals =
Plausible.Goals.Filters.preload_needed_goals(site, dimensions, filters)
Plausible.Stats.Goals.preload_needed_goals(site, dimensions, filters)

{revenue_warning, revenue_currencies} =
preload_revenue(site, preloaded_goals, metrics, dimensions)
Expand Down
14 changes: 0 additions & 14 deletions lib/plausible/stats/filters/utils.ex
Original file line number Diff line number Diff line change
@@ -1,20 +1,6 @@
defmodule Plausible.Stats.Filters.Utils do
@moduledoc false

def split_goals(goals) do
Enum.split_with(goals, fn goal -> Plausible.Goal.type(goal) == :event end)
end

def split_goals_query_expressions(goals) do
{event_goals, pageview_goals} = split_goals(goals)
events = Enum.map(event_goals, fn goal -> goal.event_name end)

page_regexes =
Enum.map(pageview_goals, fn goal -> page_regex(goal.page_path) end)

{events, page_regexes}
end

def page_regex(expr) do
escaped =
expr
Expand Down
103 changes: 81 additions & 22 deletions lib/plausible/goals/filters.ex → lib/plausible/stats/goals.ex
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
defmodule Plausible.Goals.Filters do
@moduledoc false
defmodule Plausible.Stats.Goals do
@moduledoc """
Stats code related to filtering and grouping by goals.
"""

import Ecto.Query
import Plausible.Stats.Filters.Utils, only: [page_regex: 1]
Expand Down Expand Up @@ -59,6 +61,49 @@ defmodule Plausible.Goals.Filters do
end)
end

@type goal_join_data() :: %{
indices: [non_neg_integer()],
types: [String.t()],
event_names_imports: [String.t()],
event_names_by_type: [String.t()],
page_regexes: [String.t()],
scroll_thresholds: [non_neg_integer()]
}

@doc """
Returns data needed to perform a GROUP BY on goals in an ecto query.
"""
@spec goal_join_data(Plausible.Stats.Query.t()) :: goal_join_data()
def goal_join_data(query) do
goals = query.preloaded_goals.matching_toplevel_filters

%{
indices: Enum.with_index(goals, 1) |> Enum.map(fn {_goal, idx} -> idx end),
types: Enum.map(goals, &to_string(Plausible.Goal.type(&1))),
# :TRICKY: This will contain "" for non-event goals
event_names_imports: Enum.map(goals, &to_string(&1.event_name)),
event_names_by_type:
Enum.map(goals, fn goal ->
case Plausible.Goal.type(goal) do
:event -> goal.event_name
:page -> "pageview"
:scroll -> "pageleave"
end
end),
# :TRICKY: event goals are considered to match everything for the sake of efficient queries in query_builder.ex
# See also Plausible.Stats.SQL.Expression#event_goal_join
page_regexes:
Enum.map(goals, fn goal ->
case Plausible.Goal.type(goal) do
:event -> ".?"
:page -> Filters.Utils.page_regex(goal.page_path)
:scroll -> Filters.Utils.page_regex(goal.page_path)
end
end),
scroll_thresholds: Enum.map(goals, & &1.scroll_threshold)
}
end

defp filter_preloaded(goals, filter, clause) do
Enum.filter(goals, fn goal -> matches?(goal, filter, clause) end)
end
Expand Down Expand Up @@ -106,33 +151,47 @@ defmodule Plausible.Goals.Filters do

defp build_condition(filtered_goals, imported?) do
Enum.reduce(filtered_goals, false, fn goal, dynamic_statement ->
case goal do
nil ->
dynamic([e], ^dynamic_statement)

%Plausible.Goal{event_name: event_name} when is_binary(event_name) ->
dynamic([e], e.name == ^event_name or ^dynamic_statement)

%Plausible.Goal{page_path: page_path} when is_binary(page_path) ->
dynamic([e], ^page_filter_condition(page_path, imported?) or ^dynamic_statement)
if is_nil(goal) do
dynamic([e], ^dynamic_statement)
else
type = Plausible.Goal.type(goal)
dynamic([e], ^goal_condition(type, goal, imported?) or ^dynamic_statement)
end
end)
end

defp page_filter_condition(page_path, imported?) do
db_field = page_path_db_field(imported?)
defp goal_condition(:event, goal, _) do
dynamic([e], e.name == ^goal.event_name)
end

page_condition =
if String.contains?(page_path, "*") do
dynamic([e], fragment("match(?, ?)", field(e, ^db_field), ^page_regex(page_path)))
else
dynamic([e], field(e, ^db_field) == ^page_path)
end
defp goal_condition(:scroll, goal, false = _imported?) do
pathname_condition = page_path_condition(goal.page_path, _imported? = false)
name_condition = dynamic([e], e.name == "pageleave")

scroll_condition =
dynamic([e], e.scroll_depth <= 100 and e.scroll_depth >= ^goal.scroll_threshold)

dynamic([e], ^pathname_condition and ^name_condition and ^scroll_condition)
end
macobo marked this conversation as resolved.
Show resolved Hide resolved

defp goal_condition(:page, goal, true = _imported?) do
page_path_condition(goal.page_path, _imported? = true)
end

defp goal_condition(:page, goal, false = _imported?) do
name_condition = dynamic([e], e.name == "pageview")
pathname_condition = page_path_condition(goal.page_path, _imported? = false)

dynamic([e], ^pathname_condition and ^name_condition)
end

defp page_path_condition(page_path, imported?) do
db_field = page_path_db_field(imported?)

if imported? do
dynamic([e], ^page_condition)
if String.contains?(page_path, "*") do
dynamic([e], fragment("match(?, ?)", field(e, ^db_field), ^page_regex(page_path)))
else
dynamic([e], ^page_condition and e.name == "pageview")
dynamic([e], field(e, ^db_field) == ^page_path)
end
end

Expand Down
1 change: 1 addition & 0 deletions lib/plausible/stats/imported/base.ex
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ defmodule Plausible.Stats.Imported.Base do
|> Enum.map(fn
:event -> "imported_custom_events"
:page -> "imported_pages"
:scroll -> nil
end)

case Enum.uniq(table_candidates ++ filter_goal_table_candidates) do
Expand Down
26 changes: 19 additions & 7 deletions lib/plausible/stats/imported/imported.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ defmodule Plausible.Stats.Imported do
import Ecto.Query
import Plausible.Stats.Imported.SQL.Expression

alias Plausible.Stats.Filters
alias Plausible.Stats.Imported
alias Plausible.Stats.Query
alias Plausible.Stats.SQL.QueryBuilder
Expand Down Expand Up @@ -239,16 +238,20 @@ defmodule Plausible.Stats.Imported do
end

def merge_imported(q, site, %Query{dimensions: ["event:goal"]} = query, metrics) do
{events, page_regexes} =
Filters.Utils.split_goals_query_expressions(query.preloaded_goals.matching_toplevel_filters)
goal_join_data = Plausible.Stats.Goals.goal_join_data(query)

Imported.Base.decide_tables(query)
|> Enum.map(fn
"imported_custom_events" ->
Imported.Base.query_imported("imported_custom_events", site, query)
|> where([i], i.visitors > 0)
|> select_merge_as([i], %{
dim0: fragment("-indexOf(?, ?)", type(^events, {:array, :string}), i.name)
dim0:
fragment(
"indexOf(?, ?)",
type(^goal_join_data.event_names_imports, {:array, :string}),
i.name
)
})
|> select_imported_metrics(metrics)
|> group_by([], selected_as(:dim0))
Expand All @@ -260,15 +263,24 @@ defmodule Plausible.Stats.Imported do
|> where(
[i],
fragment(
"notEmpty(multiMatchAllIndices(?, ?) as indices)",
"""
notEmpty(
arrayFilter(
goal_idx -> ?[goal_idx] = 'page' AND match(?, ?[goal_idx]),
?
) as indices
)
""",
type(^goal_join_data.types, {:array, :string}),
i.page,
type(^page_regexes, {:array, :string})
type(^goal_join_data.page_regexes, {:array, :string}),
type(^goal_join_data.indices, {:array, :integer})
)
)
|> join(:inner, [_i], index in fragment("indices"), hints: "ARRAY", on: true)
|> group_by([_i, index], index)
|> select_merge_as([_i, index], %{
dim0: type(fragment("?", index), :integer)
dim0: fragment("CAST(?, 'UInt64')", index)
})
|> select_imported_metrics(metrics)
end)
Expand Down
2 changes: 1 addition & 1 deletion lib/plausible/stats/imported/sql/where_builder.ex
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ defmodule Plausible.Stats.Imported.SQL.WhereBuilder do
db_field = Plausible.Stats.Filters.without_prefix(dimension)

if db_field == :goal do
Plausible.Goals.Filters.add_filter(query, filter, imported?: true)
Plausible.Stats.Goals.add_filter(query, filter, imported?: true)
else
mapped_db_field = Map.get(@db_field_mappings, db_field, db_field)

Expand Down
11 changes: 3 additions & 8 deletions lib/plausible/stats/query_runner.ex
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ defmodule Plausible.Stats.QueryRunner do
QueryOptimizer,
QueryResult,
Legacy,
Filters,
SQL,
Util,
Time
Expand Down Expand Up @@ -137,15 +136,11 @@ defmodule Plausible.Stats.QueryRunner do
end

defp dimension_label("event:goal", entry, query) do
{events, paths} = Filters.Utils.split_goals(query.preloaded_goals.matching_toplevel_filters)

goal_index = Map.get(entry, Util.shortname(query, "event:goal"))

# Closely coupled logic with SQL.Expression.event_goal_join/2
cond do
goal_index < 0 -> Enum.at(events, -goal_index - 1) |> Plausible.Goal.display_name()
goal_index > 0 -> Enum.at(paths, goal_index - 1) |> Plausible.Goal.display_name()
end
query.preloaded_goals.matching_toplevel_filters
|> Enum.at(goal_index - 1)
|> Plausible.Goal.display_name()
end

defp dimension_label("time:" <> _ = time_dimension, entry, query) do
Expand Down
23 changes: 16 additions & 7 deletions lib/plausible/stats/sql/expression.ex
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ defmodule Plausible.Stats.SQL.Expression do
})
end

# TO DO: make it possible to query events from pageleave events (total conversions for page scroll goals)
RobertJoonas marked this conversation as resolved.
Show resolved Hide resolved
def event_metric(:events) do
wrap_alias([e], %{
events: fragment("toUInt64(round(countIf(? != 'pageleave') * any(_sample_factor)))", e.name)
Expand Down Expand Up @@ -327,19 +328,27 @@ defmodule Plausible.Stats.SQL.Expression do
def session_metric(:conversion_rate, _query), do: %{}
def session_metric(:group_conversion_rate, _query), do: %{}

defmacro event_goal_join(events, page_regexes) do
defmacro event_goal_join(goal_join_data) do
quote do
fragment(
"""
arrayPushFront(
CAST(multiMatchAllIndices(?, ?) AS Array(Int64)),
-indexOf(?, ?)
arrayIntersect(
multiMatchAllIndices(?, ?),
arrayMap(
(expected_name, threshold, index) -> if(expected_name = ? and ? between threshold and 100, index, -1),
?,
?,
?
)
)
""",
e.pathname,
type(^unquote(page_regexes), {:array, :string}),
type(^unquote(events), {:array, :string}),
e.name
type(^unquote(goal_join_data).page_regexes, {:array, :string}),
e.name,
e.scroll_depth,
type(^unquote(goal_join_data).event_names_by_type, {:array, :string}),
type(^unquote(goal_join_data).scroll_thresholds, {:array, :integer}),
type(^unquote(goal_join_data).indices, {:array, :integer})
)
end
end
Expand Down
Loading
Loading