Repository navigation
Conversation
event:goal was the only dimension with its own GROUP BY clause: an ARRAY JOIN over the event_goal_join macros, fed by parallel arrays from Goals.goal_join_data/1. Macros can't take runtime dynamics, so the matching SQL couldn't be built from smaller parts. - Plausible.Stats.SQL.GoalMatching builds the same matching expression from dynamics. - event:goal is selected with arrayJoin() in Expression.select_dimension like other dimensions, so the special dimension_group_by clause goes. - Goals.expected_event_name/1 and Goals.pathname_regex/1 replace the copies in Goals, WhereBuilder and the removed macros. The matching expression and the WHERE clause are unchanged; only ARRAY JOIN becomes arrayJoin() in the SELECT. On a local benchmark the results are identical and the queries are no slower.
The goal breakdown checked every event against every goal: one regex per goal plus a per-goal lambda on each row. When any goal had custom props, the lambda also copied meta.key and meta.value once per goal per row, which reached 59 GB for a single query in production. Each goal is now matched in the cheapest way its definition allows: - Event goals by event name and exact page goals by pathname, with a transform() lookup. The unique indexes on goals keep each of these keys to one goal. - Wildcard page goals, scroll goals and goals with custom props are scanned: pathname regexes first, then the event name and scroll threshold, then custom props. Only events whose name one of them expects are scanned. - Checks that can't fail are left out of the SQL. Only scroll goals check the scroll depth, which also keeps engagements without one from completing them. - Exact page goals match by equality, as goal filters already do. Goals on the reserved engagement event can't be created and none are left on Plausible Cloud, so they're no longer special-cased. Goals saved with surrounding whitespace before #2382 can share a key with a newer goal once trimmed on load. For those, the breakdown now shows only the newer goal instead of two identical rows. A follow-up will remove them from the database. New breakdown tests cover what the existing ones missed: a | in a page goal path, custom events on a page goal's path, and pageviews with the custom props of a custom event goal. They also pass on the previous matching. On a local benchmark with production-shaped data the results are identical to before. Breakdowns are 6-15x faster on most goal-heavy cases, and peak memory drops from up to 11.4 GiB to under 0.6 GiB.
apata
left a comment
apata
left a comment
There was a problem hiding this comment.
I had a go at reviewing this. I'm pretty new to goals code, so the comments may be somewhat superficial. Still, FWIW, I didn't find the complexity too bad. If anything, I think we could be even more explicit, e.g. scroll goals go down this path, goals with custom props another and so on, with dead ends rather than catch-alls. New feature -> new explicit path.
On rollout: if we're still unsure if this works with all prod queries, we can deploy it side-by-side with the current implementation for a period, run both queries and validate at runtime that the result is the same (for unsampled queries). Just wanted this on the table if merging seems too risky.
| %{ | ||
| by_name: for({{:name, name}, _goal, index} <- keyed, do: {name, index}), | ||
| by_pathname: for({{:pathname, path}, _goal, index} <- keyed, do: {path, index}), | ||
| scanned: for({nil, goal, index} <- keyed, do: {goal, index}) | ||
| } |
There was a problem hiding this comment.
question, non-blocking: This and lookup_key are one of the main events in this module, yet it categorizes goals only partially. Down the line, in scanning, we break the scanned goals down further: have any with custom props? have any scroll goals? I think there may be something to win in code / intent clarity if we categorized fully without a true -> nil fallback.
There was a problem hiding this comment.
Agree with the comment. I removed the fallback in 4e036ea
The scan path still does conditionals instead of breaking down the goals beforehand. I didn't find a way to break down goals beforehand for that branch that felt better than current code. Open to suggestions though!
|
|
||
| dynamic( | ||
| fragment( | ||
| "arrayFilter(i -> i != 0, [transform(?, ?, CAST(? AS Array(UInt64)), toUInt64(0))])", |
There was a problem hiding this comment.
Nice find on transform vs has!
…uery_goal_dimension_test.exs Co-authored-by: Artur Pata <4290938+apata@users.noreply.github.com>
Replaces the has() fragment with a plain `in`. IN takes about the same time for any number of names: over 33M events the check alone takes 24-29 ms with 3-1000 names, against 49-58 ms for has(). In the full goal breakdown the difference is within noise.
This branch has not been deployed
2 participants
Goal breakdowns currently scale poorly with goal count: each additional goal adds work for every event, and custom-property goals multiply the memory cost.
Goal breakdowns with many entries make up a large fraction of slowest and most expensive queries on prod.
I'll be honest - the code for goal breakdowns used to be complex and with this it becomes even more complex. I cannot think of a way to simplify this though and the performance gains are significant.
This PR refactors the goal breakdown in the first commit and applies performance optimizations on the second commit:
transform()for event goals without custom properties, instead of expensive regex matchingtransform()for page goals without wildcards or custom properties, instead of expensive regex matchingLocal benchmarks on ClickHouse 25.11 returned identical results to master in every case. Most large-goal breakdowns were 6–15× faster, with 5–46× lower peak memory.