diff --git a/CHANGELOG.md b/CHANGELOG.md index fa4292529979..c9fecbc2ad42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ All notable changes to this project will be documented in this file. ### Fixed +- Fixed main graph crash for time:minute dimension and goal conversion filter - Fixed Stats API timeseries returning time buckets falling outside the queried range - Fixed issue with all non-interactive events being counted as interactive - Fixed countries map countries staying highlighted on Chrome diff --git a/lib/plausible/stats/query.ex b/lib/plausible/stats/query.ex index b12802d6d897..d6fa62f91be9 100644 --- a/lib/plausible/stats/query.ex +++ b/lib/plausible/stats/query.ex @@ -30,7 +30,8 @@ defmodule Plausible.Stats.Query do # Contains information to determine how to combine legacy and new time on page metrics time_on_page_data: %{}, sql_join_type: :left, - smear_session_metrics: false + session_smearing_applied?: false, + bypass_session_smearing?: false require OpenTelemetry.Tracer, as: Tracer diff --git a/lib/plausible/stats/query_optimizer.ex b/lib/plausible/stats/query_optimizer.ex index 27a9452c45eb..ae0e0c22117f 100644 --- a/lib/plausible/stats/query_optimizer.ex +++ b/lib/plausible/stats/query_optimizer.ex @@ -200,7 +200,7 @@ defmodule Plausible.Stats.QueryOptimizer do defp build_split_query(:sessions_smeared, metrics, query) do {_, query} = build_split_query(:sessions, metrics, query) - {:sessions, Query.set(query, smear_session_metrics: true)} + {:sessions, Query.set(query, session_smearing_applied?: true)} end on_ee do diff --git a/lib/plausible/stats/sql/expression.ex b/lib/plausible/stats/sql/expression.ex index 221c1900da0d..07c7a15aee57 100644 --- a/lib/plausible/stats/sql/expression.ex +++ b/lib/plausible/stats/sql/expression.ex @@ -114,7 +114,8 @@ defmodule Plausible.Stats.SQL.Expression do }) end - def select_dimension(q, key, "time:hour", :sessions, query) when query.smear_session_metrics do + def select_dimension(q, key, "time:hour", :sessions, query) + when query.session_smearing_applied? do # :TRICKY: ClickHouse timeSlots works off of unix epoch and is not # timezone-aware. This means that for e.g. Asia/Katmandu (GMT+5:45) # to work, we divide time into 15-minute buckets and later combine these @@ -140,7 +141,7 @@ defmodule Plausible.Stats.SQL.Expression do # :NOTE: This is not exposed in Query APIv2 def select_dimension(q, key, "time:minute", :sessions, query) - when query.smear_session_metrics do + when query.session_smearing_applied? do {first, last} = Time.utc_boundaries(query) q @@ -435,7 +436,7 @@ defmodule Plausible.Stats.SQL.Expression do }) end - def session_metric(:visits, query) when query.smear_session_metrics do + def session_metric(:visits, query) when query.session_smearing_applied? do wrap_alias([s], %{ visits: scale_sample(fragment("uniq(?)", s.session_id)) }) diff --git a/lib/plausible/stats/sql/query_builder.ex b/lib/plausible/stats/sql/query_builder.ex index 91caed4c9655..c5529130859c 100644 --- a/lib/plausible/stats/sql/query_builder.ex +++ b/lib/plausible/stats/sql/query_builder.ex @@ -287,7 +287,8 @@ defmodule Plausible.Stats.SQL.QueryBuilder do end defp select_from(dimension, query, queries) do - smeared? = Enum.any?(queries, fn {_table_type, query, _q} -> query.smear_session_metrics end) + smeared? = + Enum.any?(queries, fn {_table_type, query, _q} -> query.session_smearing_applied? end) cond do query.sql_join_type == :left -> :leftmost_table diff --git a/lib/plausible/stats/sql/special_metrics.ex b/lib/plausible/stats/sql/special_metrics.ex index 04b429e6df30..92a10a5c76c6 100644 --- a/lib/plausible/stats/sql/special_metrics.ex +++ b/lib/plausible/stats/sql/special_metrics.ex @@ -104,7 +104,11 @@ defmodule Plausible.Stats.SQL.SpecialMetrics do order_by: [], include_imported: query.include_imported, preloaded_goals: Map.put(query.preloaded_goals, :matching_toplevel_filters, []), - pagination: nil + pagination: nil, + # Smearing spreads session visitors across every time bucket the session + # was active in. For conversion rate we need unsmeared counts + # (one visitor counted exactly once per time bucket they were seen in) + bypass_session_smearing?: true ) from(e in subquery(q), diff --git a/lib/plausible/stats/table_decider.ex b/lib/plausible/stats/table_decider.ex index 2a19400ebffd..df9121987b85 100644 --- a/lib/plausible/stats/table_decider.ex +++ b/lib/plausible/stats/table_decider.ex @@ -110,9 +110,15 @@ defmodule Plausible.Stats.TableDecider do # the length of the session, not just when events occurred or when session started. # For this reason, we smear the session metrics across the length of the session. # See `time_slots` usage in `Plausible.Stats.SQL.Expression` to understand how this is done. + # + # Smearing must be suppressed when :group_conversion_rate is in the query + # or when query.bypass_session_smearing? is true (used by group_conversion_rate + # totals subquery). @smearable_metrics [:visitors, :visits] defp smear_session_metrics({:sessions, metrics} = value, query) do - if "time:minute" in query.dimensions or "time:hour" in query.dimensions do + if ("time:minute" in query.dimensions or "time:hour" in query.dimensions) and + :group_conversion_rate not in query.metrics and + not query.bypass_session_smearing? do # Split metrics into two groups: one with visitors and visits, and the remaining ones {smearable_metrics, session_metrics} = Enum.split_with(metrics, &(&1 in @smearable_metrics)) diff --git a/test/plausible/stats/query/query_test.exs b/test/plausible/stats/query/query_test.exs index 60aa33ee793a..31cab8e64b60 100644 --- a/test/plausible/stats/query/query_test.exs +++ b/test/plausible/stats/query/query_test.exs @@ -317,4 +317,61 @@ defmodule Plausible.Stats.QueryTest do ] end end + + describe "group_conversion_rate with time:minute and time:hour dimensions" do + test "group_conversion_rate with time:hour dimension and pageview goal filter does not crash", + %{site: site} do + insert(:goal, site: site, page_path: "/blog") + + populate_stats(site, [ + build(:pageview, user_id: 1, pathname: "/blog", timestamp: ~N[2021-01-01 00:00:00]), + build(:pageview, user_id: 2, pathname: "/other", timestamp: ~N[2021-01-01 00:30:00]), + build(:pageview, user_id: 3, pathname: "/blog", timestamp: ~N[2021-01-01 01:00:00]), + build(:pageview, user_id: 4, pathname: "/other", timestamp: ~N[2021-01-01 01:30:00]) + ]) + + {:ok, query} = + QueryBuilder.build(site, %ParsedQueryParams{ + metrics: [:visitors, :group_conversion_rate], + input_date_range: {:date_range, ~D[2021-01-01], ~D[2021-01-01]}, + dimensions: ["time:hour"], + filters: [[:is, "event:goal", ["Visit /blog"]]], + skip_goal_existence_check: true + }) + + %Stats.QueryResult{results: results} = Stats.query(site, query) + + assert [ + %{dimensions: ["2021-01-01 00:00:00"], metrics: [1, 50.0]}, + %{dimensions: ["2021-01-01 01:00:00"], metrics: [1, 50.0]} + ] = results + end + + test "group_conversion_rate with time:minute dimension and custom event goal filter does not crash", + %{site: site} do + insert(:goal, site: site, event_name: "Signup") + + populate_stats(site, [ + build(:event, name: "Signup", user_id: 1, timestamp: ~N[2021-01-01 00:00:00]), + build(:pageview, user_id: 2, timestamp: ~N[2021-01-01 00:00:00]), + build(:event, name: "Signup", user_id: 3, timestamp: ~N[2021-01-01 00:05:00]) + ]) + + {:ok, query} = + QueryBuilder.build(site, %ParsedQueryParams{ + metrics: [:visitors, :group_conversion_rate], + input_date_range: {:datetime_range, ~U[2021-01-01 00:00:00Z], ~U[2021-01-01 00:10:00Z]}, + dimensions: ["time:minute"], + filters: [[:is, "event:goal", ["Signup"]]], + skip_goal_existence_check: true + }) + + %Stats.QueryResult{results: results} = Stats.query(site, query) + + assert [ + %{dimensions: ["2021-01-01 00:00:00"], metrics: [1, 50.0]}, + %{dimensions: ["2021-01-01 00:05:00"], metrics: [1, 100.0]} + ] = results + end + end end