Bugfix: adjustments to querying time:minutes and time:hours dimensions for goal conversion

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

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
 

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

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))
     })

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

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),

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))
 

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