From 9261cb3642e3a531f0e1fbb1ef0578665455f9fe Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Wed, 29 Jan 2025 13:50:53 +0200 Subject: [PATCH 1/8] WIP: Update scroll_depth_visible_at in ingestion --- lib/plausible/application.ex | 1 + lib/plausible/ingestion/event.ex | 12 ++- .../ingestion/scroll_depth_visible_at.ex | 85 +++++++++++++++++++ lib/plausible/site/cache.ex | 1 + lib/plausible/stats/scroll_depth.ex | 6 +- .../api/external_controller_test.exs | 30 +++++++ 6 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 lib/plausible/ingestion/scroll_depth_visible_at.ex diff --git a/lib/plausible/application.ex b/lib/plausible/application.ex index 1e0e6a23c551..d17f1ba103b4 100644 --- a/lib/plausible/application.ex +++ b/lib/plausible/application.ex @@ -24,6 +24,7 @@ defmodule Plausible.Application do {Plausible.Auth.TOTP.Vault, key: totp_vault_key()}, {Plausible.RateLimit, clean_period: :timer.minutes(10)}, Plausible.Ingestion.Counters, + Plausible.Ingestion.ScrollDepthVisibleAt, {Finch, name: Plausible.Finch, pools: finch_pool_config()}, {Phoenix.PubSub, name: Plausible.PubSub}, Plausible.Session.Salts, diff --git a/lib/plausible/ingestion/event.ex b/lib/plausible/ingestion/event.ex index e113ae8047ca..6b28d130c8ac 100644 --- a/lib/plausible/ingestion/event.ex +++ b/lib/plausible/ingestion/event.ex @@ -130,7 +130,8 @@ defmodule Plausible.Ingestion.Event do put_user_id: &put_user_id/2, validate_clickhouse_event: &validate_clickhouse_event/2, register_session: ®ister_session/2, - write_to_buffer: &write_to_buffer/2 + write_to_buffer: &write_to_buffer/2, + register_scroll_depth_visible: ®ister_scroll_depth_visible/2 ] end @@ -405,6 +406,15 @@ defmodule Plausible.Ingestion.Event do event end + defp register_scroll_depth_visible(%__MODULE__{clickhouse_event: ev} = event, _context) do + if is_nil(event.site.scroll_depth_visible_at) and ev.name in ["pageleave", "engagement"] and + not is_nil(ev.scroll_depth) and ev.scroll_depth >= 0 and ev.scroll_depth <= 100 do + Plausible.Ingestion.ScrollDepthVisibleAt.mark_scroll_depth_visible(event.site.id) + end + + event + end + @click_id_params ["gclid", "gbraid", "wbraid", "msclkid", "fbclid", "twclid"] defp get_click_id_param(nil), do: nil diff --git a/lib/plausible/ingestion/scroll_depth_visible_at.ex b/lib/plausible/ingestion/scroll_depth_visible_at.ex new file mode 100644 index 000000000000..103146295020 --- /dev/null +++ b/lib/plausible/ingestion/scroll_depth_visible_at.ex @@ -0,0 +1,85 @@ +defmodule Plausible.Ingestion.ScrollDepthVisibleAt do + @moduledoc false + + use GenServer + require Logger + alias Plausible.Repo + + import Ecto.Query + + @initial_state %{ + updated_sites: MapSet.new(), + pending: MapSet.new() + } + + def start_link(opts) do + opts = Keyword.merge(opts, name: __MODULE__) + GenServer.start_link(__MODULE__, @initial_state, opts) + end + + def mark_scroll_depth_visible(site_id) do + GenServer.cast(__MODULE__, {:update_site, site_id}) + end + + @impl true + def init(state) do + {:ok, state} + end + + @impl true + def handle_cast({:update_site, site_id}, state) do + cond do + MapSet.member?(state.updated_sites, site_id) -> + {:noreply, state} + + MapSet.member?(state.pending, site_id) -> + {:noreply, state} + + true -> + Task.start(fn -> attempt_update_repo(site_id) end) + + {:noreply, + %{ + updated_sites: state.updated_sites, + pending: MapSet.put(state.pending, site_id) + }} + end + end + + @impl true + def handle_cast({:mark_updated, site_id}, state) do + {:noreply, + %{ + updated_sites: MapSet.put(state.updated_sites, site_id), + pending: MapSet.delete(state.pending, site_id) + }} + end + + @impl true + def handle_cast({:mark_update_failed, site_id}, state) do + {:noreply, + %{ + updated_sites: state.updated_sites, + pending: MapSet.delete(state.pending, site_id) + }} + end + + defp attempt_update_repo(site_id) do + Repo.update_all( + from(s in Plausible.Site, where: s.id == ^site_id), + set: [ + scroll_depth_visible_at: DateTime.utc_now() + ] + ) + + # Call genserver with {:mark_updated, site_id} + GenServer.cast(__MODULE__, {:mark_updated, site_id}) + rescue + error -> + Logger.error( + "Error updating scroll depth visible at for site #{site_id}: #{inspect(error)}. Will retry later." + ) + + GenServer.cast(__MODULE__, {:mark_update_failed, site_id}) + end +end diff --git a/lib/plausible/site/cache.ex b/lib/plausible/site/cache.ex index 1514bc006ae3..8cfba5196709 100644 --- a/lib/plausible/site/cache.ex +++ b/lib/plausible/site/cache.ex @@ -31,6 +31,7 @@ defmodule Plausible.Site.Cache do domain_changed_from ingest_rate_limit_scale_seconds ingest_rate_limit_threshold + scroll_depth_visible_at )a @impl true diff --git a/lib/plausible/stats/scroll_depth.ex b/lib/plausible/stats/scroll_depth.ex index 246c8a852723..a5670ac6c7e7 100644 --- a/lib/plausible/stats/scroll_depth.ex +++ b/lib/plausible/stats/scroll_depth.ex @@ -33,9 +33,9 @@ defmodule Plausible.Stats.ScrollDepth do is_nil(site.scroll_depth_visible_at) -> visible? = has_scroll_data_last_30d?(site) - if visible? do - Plausible.Sites.set_scroll_depth_visible_at(site) - end + # if visible? do + # Plausible.Sites.set_scroll_depth_visible_at(site) + # end visible? end diff --git a/test/plausible_web/controllers/api/external_controller_test.exs b/test/plausible_web/controllers/api/external_controller_test.exs index c4d0e52b2315..75ecca940dad 100644 --- a/test/plausible_web/controllers/api/external_controller_test.exs +++ b/test/plausible_web/controllers/api/external_controller_test.exs @@ -3,6 +3,8 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do use Plausible.ClickhouseRepo use Plausible.Teams.Test + alias Plausible.Repo + @user_agent "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.110 Safari/537.36" @user_agent_mobile "Mozilla/5.0 (Linux; Android 6.0; U007 Pro Build/MRA58K; wv) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/44.0.2403.119 Mobile Safari/537.36" @user_agent_tablet "Mozilla/5.0 (Linux; U; Android 4.2.2; it-it; Surfing TAB B 9.7 3G Build/JDQ39) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30" @@ -1291,6 +1293,7 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do pageleave = get_events(site) |> Enum.find(&(&1.name == "pageleave")) assert pageleave.scroll_depth == 255 + assert not scroll_depth_visible_at_set?(site) end test "sd field is ignored if name is not pageleave", %{conn: conn, site: site} do @@ -1298,6 +1301,7 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do post(conn, "/api/event", %{n: "custom_e", u: "https://test.com", d: site.domain, sd: 10}) assert [%{scroll_depth: 0}, %{scroll_depth: 0}] = get_events(site) + assert not scroll_depth_visible_at_set?(site) end test "ingests valid scroll_depth for a pageleave", %{conn: conn, site: site} do @@ -1307,6 +1311,7 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do pageleave = get_events(site) |> Enum.find(&(&1.name == "pageleave")) assert pageleave.scroll_depth == 25 + assert scroll_depth_visible_at_set?(site) end test "ingests scroll_depth as 100 when sd > 100", %{conn: conn, site: site} do @@ -1316,6 +1321,7 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do pageleave = get_events(site) |> Enum.find(&(&1.name == "pageleave")) assert pageleave.scroll_depth == 100 + assert scroll_depth_visible_at_set?(site) end test "ingests scroll_depth as 255 when sd is a string", %{conn: conn, site: site} do @@ -1325,6 +1331,7 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do pageleave = get_events(site) |> Enum.find(&(&1.name == "pageleave")) assert pageleave.scroll_depth == 255 + assert not scroll_depth_visible_at_set?(site) end test "ingests scroll_depth as 255 when sd is a negative integer", %{conn: conn, site: site} do @@ -1334,6 +1341,7 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do pageleave = get_events(site) |> Enum.find(&(&1.name == "pageleave")) assert pageleave.scroll_depth == 255 + assert not scroll_depth_visible_at_set?(site) end test "ingests valid scroll_depth for a engagement event", %{conn: conn, site: site} do @@ -1343,6 +1351,23 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do event = get_events(site) |> Enum.find(&(&1.name == "engagement")) assert event.scroll_depth == 25 + assert scroll_depth_visible_at_set?(site) + end + + test "does not update scroll_depth_visible_at twice", %{conn: conn, site: site} do + post(conn, "/api/event", %{n: "pageview", u: "https://test.com", d: site.domain}) + post(conn, "/api/event", %{n: "pageleave", u: "https://test.com", d: site.domain, sd: 25}) + Plausible.Event.WriteBuffer.flush() + + site1 = Repo.reload!(site) + assert not is_nil(site1.scroll_depth_visible_at) + + post(conn, "/api/event", %{n: "pageleave", u: "https://test.com", d: site.domain, sd: 50}) + Plausible.Event.WriteBuffer.flush() + + site2 = Repo.reload!(site) + assert not is_nil(site2.scroll_depth_visible_at) + assert site1.scroll_depth_visible_at == site2.scroll_depth_visible_at end end @@ -3584,4 +3609,9 @@ defmodule PlausibleWeb.Api.ExternalControllerTest do ) ) end + + defp scroll_depth_visible_at_set?(site) do + site = Repo.reload!(site) + not is_nil(site.scroll_depth_visible_at) + end end From 76d5dc98b274e7b3ec290e30d407f6ea8f789499 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 15:21:51 +0200 Subject: [PATCH 2/8] Simplify code and test genserver directly --- .../ingestion/scroll_depth_visible_at.ex | 70 ++++++------------- .../scroll_depth_visible_at_test.exs | 14 ++++ 2 files changed, 34 insertions(+), 50 deletions(-) create mode 100644 test/plausible/ingestion/scroll_depth_visible_at_test.exs diff --git a/lib/plausible/ingestion/scroll_depth_visible_at.ex b/lib/plausible/ingestion/scroll_depth_visible_at.ex index 103146295020..5a84061b4c0b 100644 --- a/lib/plausible/ingestion/scroll_depth_visible_at.ex +++ b/lib/plausible/ingestion/scroll_depth_visible_at.ex @@ -1,5 +1,11 @@ defmodule Plausible.Ingestion.ScrollDepthVisibleAt do - @moduledoc false + @moduledoc """ + GenServer that updates the `scroll_depth_visible_at` column for a site when needed. + + This is called in a hot loop in ingestion, so it: + 1. Only updates the database once per site (if SiteCache doesn't know about visibility yet) + 2. Does not retry the update if it fails, to be retried on server restart + """ use GenServer require Logger @@ -7,14 +13,9 @@ defmodule Plausible.Ingestion.ScrollDepthVisibleAt do import Ecto.Query - @initial_state %{ - updated_sites: MapSet.new(), - pending: MapSet.new() - } - - def start_link(opts) do + def start_link(opts \\ []) do opts = Keyword.merge(opts, name: __MODULE__) - GenServer.start_link(__MODULE__, @initial_state, opts) + GenServer.start_link(__MODULE__, opts, opts) end def mark_scroll_depth_visible(site_id) do @@ -22,48 +23,22 @@ defmodule Plausible.Ingestion.ScrollDepthVisibleAt do end @impl true - def init(state) do - {:ok, state} + def init(_opts) do + {:ok, MapSet.new()} end @impl true - def handle_cast({:update_site, site_id}, state) do - cond do - MapSet.member?(state.updated_sites, site_id) -> - {:noreply, state} - - MapSet.member?(state.pending, site_id) -> - {:noreply, state} - - true -> - Task.start(fn -> attempt_update_repo(site_id) end) - - {:noreply, - %{ - updated_sites: state.updated_sites, - pending: MapSet.put(state.pending, site_id) - }} + def handle_cast({:update_site, site_id}, touched_sites) do + # When receiving multiple update requests for a site, only process the first one + if MapSet.member?(touched_sites, site_id) do + {:noreply, touched_sites} + else + Task.start(fn -> attempt_update_repo(site_id) end) + + {:noreply, MapSet.put(touched_sites, site_id)} end end - @impl true - def handle_cast({:mark_updated, site_id}, state) do - {:noreply, - %{ - updated_sites: MapSet.put(state.updated_sites, site_id), - pending: MapSet.delete(state.pending, site_id) - }} - end - - @impl true - def handle_cast({:mark_update_failed, site_id}, state) do - {:noreply, - %{ - updated_sites: state.updated_sites, - pending: MapSet.delete(state.pending, site_id) - }} - end - defp attempt_update_repo(site_id) do Repo.update_all( from(s in Plausible.Site, where: s.id == ^site_id), @@ -71,15 +46,10 @@ defmodule Plausible.Ingestion.ScrollDepthVisibleAt do scroll_depth_visible_at: DateTime.utc_now() ] ) - - # Call genserver with {:mark_updated, site_id} - GenServer.cast(__MODULE__, {:mark_updated, site_id}) rescue error -> Logger.error( - "Error updating scroll depth visible at for site #{site_id}: #{inspect(error)}. Will retry later." + "Error updating scroll_depth_visible_at for site #{site_id}: #{inspect(error)}. This will not be retried until server restart." ) - - GenServer.cast(__MODULE__, {:mark_update_failed, site_id}) end end diff --git a/test/plausible/ingestion/scroll_depth_visible_at_test.exs b/test/plausible/ingestion/scroll_depth_visible_at_test.exs new file mode 100644 index 000000000000..fbc89f003e67 --- /dev/null +++ b/test/plausible/ingestion/scroll_depth_visible_at_test.exs @@ -0,0 +1,14 @@ +defmodule Plausible.Ingestion.ScrollDepthVisibleAtTest do + use Plausible.DataCase + use Plausible.Teams.Test + + test "mark_scroll_depth_visible" do + site = new_site() + Plausible.Ingestion.ScrollDepthVisibleAt.mark_scroll_depth_visible(site.id) + + Plausible.TestUtils.eventually(fn -> + site = Plausible.Repo.reload!(site) + {not is_nil(site.scroll_depth_visible_at), site.scroll_depth_visible_at} + end) + end +end From 37eae5299c1a536e99bc8d16aa05c29e7458e8f0 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 15:23:52 +0200 Subject: [PATCH 3/8] No more check_scroll_depth_visible! --- lib/plausible/exports.ex | 4 +- lib/plausible/stats/scroll_depth.ex | 49 ------------------- .../controllers/stats_controller.ex | 12 ++--- 3 files changed, 4 insertions(+), 61 deletions(-) diff --git a/lib/plausible/exports.ex b/lib/plausible/exports.ex index f082e5f5f2da..70f15b2b8f1e 100644 --- a/lib/plausible/exports.ex +++ b/lib/plausible/exports.ex @@ -418,8 +418,6 @@ defmodule Plausible.Exports do site = Plausible.Repo.get(Plausible.Site, site_id) current_user = current_user_id && Plausible.Repo.get(Plausible.Auth.User, current_user_id) - include_scroll_depth? = Plausible.Stats.ScrollDepth.check_feature_visible!(site, current_user) - base_q = from(e in sampled("events_v2"), where: ^export_filter(site_id, date_range), @@ -428,7 +426,7 @@ defmodule Plausible.Exports do order_by: selected_as(:date) ) - if include_scroll_depth? do + if Plausible.Stats.ScrollDepth.feature_visible?(site, current_user) do max_scroll_depth_per_visitor_q = from(e in "events_v2", where: ^export_filter(site_id, date_range), diff --git a/lib/plausible/stats/scroll_depth.ex b/lib/plausible/stats/scroll_depth.ex index a5670ac6c7e7..4ebd80fe7c8b 100644 --- a/lib/plausible/stats/scroll_depth.ex +++ b/lib/plausible/stats/scroll_depth.ex @@ -3,11 +3,6 @@ defmodule Plausible.Stats.ScrollDepth do Module to check whether the scroll depth metric is available and visible for a site. """ - import Ecto.Query - require Logger - - alias Plausible.ClickhouseRepo - def feature_available?(site, user) do FunWithFlags.enabled?(:scroll_depth, for: user) || FunWithFlags.enabled?(:scroll_depth, for: site) @@ -16,48 +11,4 @@ defmodule Plausible.Stats.ScrollDepth do def feature_visible?(site, user) do feature_available?(site, user) && not is_nil(site.scroll_depth_visible_at) end - - @doc """ - Checks whether the scroll depth feature is visible for a site and updates the site record if it is. - - Note this function queries ClickHouse and may take a while to complete. - """ - def check_feature_visible!(site, user) do - cond do - not feature_available?(site, user) -> - false - - not is_nil(site.scroll_depth_visible_at) -> - true - - is_nil(site.scroll_depth_visible_at) -> - visible? = has_scroll_data_last_30d?(site) - - # if visible? do - # Plausible.Sites.set_scroll_depth_visible_at(site) - # end - - visible? - end - end - - defp has_scroll_data_last_30d?(site) do - try do - ClickhouseRepo.exists?( - from(e in "events_v2", - where: - e.site_id == ^site.id and - e.name == "pageleave" and - e.timestamp >= fragment("toStartOfDay(now() - toIntervalDay(30))") and - e.scroll_depth > 0 and e.scroll_depth <= 100 - ) - ) - rescue - # Avoid propagating error to the user, bringing down the site. - error -> - Logger.error("Error checking scroll data for site #{site.id}: #{inspect(error)}") - - false - end - end end diff --git a/lib/plausible_web/controllers/stats_controller.ex b/lib/plausible_web/controllers/stats_controller.ex index 8d5dc4b8cbb3..4df58abbf2c8 100644 --- a/lib/plausible_web/controllers/stats_controller.ex +++ b/lib/plausible_web/controllers/stats_controller.ex @@ -45,7 +45,7 @@ defmodule PlausibleWeb.StatsController do use Plausible.Repo alias Plausible.Sites - alias Plausible.Stats.{Filters, Query} + alias Plausible.Stats.{Filters, Query, ScrollDepth} alias PlausibleWeb.Api plug(PlausibleWeb.Plugs.AuthorizeSiteAccess when action in [:stats, :csv_export]) @@ -59,9 +59,6 @@ defmodule PlausibleWeb.StatsController do dogfood_page_path = if demo, do: "/#{site.domain}", else: "/:dashboard" skip_to_dashboard? = conn.params["skip_to_dashboard"] == "true" - scroll_depth_visible? = - Plausible.Stats.ScrollDepth.check_feature_visible!(site, current_user) - cond do (stats_start_date && can_see_stats?) || (can_see_stats? && skip_to_dashboard?) -> conn @@ -72,7 +69,7 @@ defmodule PlausibleWeb.StatsController do revenue_goals: list_revenue_goals(site), funnels: list_funnels(site), has_props: Plausible.Props.configured?(site), - scroll_depth_visible: scroll_depth_visible?, + scroll_depth_visible: ScrollDepth.feature_visible?(site, current_user), stats_start_date: stats_start_date, native_stats_start_date: NaiveDateTime.to_date(site.native_stats_start_at), title: title(conn, site), @@ -350,9 +347,6 @@ defmodule PlausibleWeb.StatsController do shared_link = Plausible.Repo.preload(shared_link, site: :owner) stats_start_date = Plausible.Sites.stats_start_date(shared_link.site) - scroll_depth_visible? = - Plausible.Stats.ScrollDepth.check_feature_visible!(shared_link.site, current_user) - conn |> put_resp_header("x-robots-tag", "noindex, nofollow") |> delete_resp_header("x-frame-options") @@ -362,7 +356,7 @@ defmodule PlausibleWeb.StatsController do revenue_goals: list_revenue_goals(shared_link.site), funnels: list_funnels(shared_link.site), has_props: Plausible.Props.configured?(shared_link.site), - scroll_depth_visible: scroll_depth_visible?, + scroll_depth_visible: ScrollDepth.feature_visible?(shared_link.site, current_user), stats_start_date: stats_start_date, native_stats_start_date: NaiveDateTime.to_date(shared_link.site.native_stats_start_at), title: title(conn, shared_link.site), From 865d65b2791776547c6212b655f73f8315d4d32f Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 15:37:37 +0200 Subject: [PATCH 4/8] Update a test --- test/plausible/imported/csv_importer_test.exs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/plausible/imported/csv_importer_test.exs b/test/plausible/imported/csv_importer_test.exs index a205355be5b9..41745a8f59bd 100644 --- a/test/plausible/imported/csv_importer_test.exs +++ b/test/plausible/imported/csv_importer_test.exs @@ -1065,6 +1065,7 @@ defmodule Plausible.Imported.CSVImporterTest do |> Enum.map(fn event -> Map.put(event, :hostname, "csv.test") end) populate_stats(exported_site, stats) + Plausible.Sites.set_scroll_depth_visible_at(exported_site) initial_context = %{ user: user, @@ -1081,9 +1082,6 @@ defmodule Plausible.Imported.CSVImporterTest do |> upload_csvs() |> run_import() - assert %NaiveDateTime{} = - Plausible.Repo.reload!(exported_site).scroll_depth_visible_at - assert %SiteImport{ start_date: start_date, end_date: end_date, From 589349476ef6e5cd88007eb46048d324cda6be5c Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 15:44:11 +0200 Subject: [PATCH 5/8] Update a test --- .../controllers/stats_controller_test.exs | 20 ++----------------- 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/test/plausible_web/controllers/stats_controller_test.exs b/test/plausible_web/controllers/stats_controller_test.exs index 5fd9f620a66d..999c0d3818f4 100644 --- a/test/plausible_web/controllers/stats_controller_test.exs +++ b/test/plausible_web/controllers/stats_controller_test.exs @@ -85,7 +85,6 @@ defmodule PlausibleWeb.StatsControllerTest do populate_stats(site, [build(:pageview)]) - # No pageleaves yet - `scroll_depth_visible_at` will remain `nil` html = conn |> get("/#{site.domain}") @@ -93,15 +92,8 @@ defmodule PlausibleWeb.StatsControllerTest do assert text_of_attr(html, @react_container, "data-scroll-depth-visible") == "false" - site = Repo.reload!(site) - assert is_nil(site.scroll_depth_visible_at) + Plausible.Sites.set_scroll_depth_visible_at(site) - populate_stats(site, [ - build(:pageview, user_id: 123), - build(:pageleave, user_id: 123, scroll_depth: 20) - ]) - - # Pageleaves exist now - `scroll_depth_visible_at` gets set to `utc_now` html = conn |> get("/#{site.domain}") @@ -1098,7 +1090,6 @@ defmodule PlausibleWeb.StatsControllerTest do site = insert(:site) link = insert(:shared_link, site: site) - # No pageleaves yet - `scroll_depth_visible_at` will remain `nil` html = conn |> get("/share/#{site.domain}/?auth=#{link.slug}") @@ -1106,15 +1097,8 @@ defmodule PlausibleWeb.StatsControllerTest do assert text_of_attr(html, @react_container, "data-scroll-depth-visible") == "false" - site = Repo.reload!(site) - assert is_nil(site.scroll_depth_visible_at) - - populate_stats(site, [ - build(:pageview, user_id: 123), - build(:pageleave, user_id: 123, scroll_depth: 20) - ]) + Plausible.Sites.set_scroll_depth_visible_at(site) - # Pageleaves exist now - `scroll_depth_visible_at` gets set to `utc_now` html = conn |> get("/share/#{site.domain}/?auth=#{link.slug}") From 7da10b967891ca7cad2583f4c0c900f2805505f0 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 17:37:24 +0200 Subject: [PATCH 6/8] GenServer -> ets --- lib/plausible/application.ex | 2 +- .../ingestion/scroll_depth_visible_at.ex | 33 +++++++------------ 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/lib/plausible/application.ex b/lib/plausible/application.ex index d17f1ba103b4..98e2cfec58c9 100644 --- a/lib/plausible/application.ex +++ b/lib/plausible/application.ex @@ -24,7 +24,6 @@ defmodule Plausible.Application do {Plausible.Auth.TOTP.Vault, key: totp_vault_key()}, {Plausible.RateLimit, clean_period: :timer.minutes(10)}, Plausible.Ingestion.Counters, - Plausible.Ingestion.ScrollDepthVisibleAt, {Finch, name: Plausible.Finch, pools: finch_pool_config()}, {Phoenix.PubSub, name: Plausible.PubSub}, Plausible.Session.Salts, @@ -121,6 +120,7 @@ defmodule Plausible.Application do setup_geolocation() Location.load_all() Plausible.Ingestion.Source.init() + Plausible.Ingestion.ScrollDepthVisibleAt.init() Plausible.Geo.await_loader() Supervisor.start_link(List.flatten(children), opts) diff --git a/lib/plausible/ingestion/scroll_depth_visible_at.ex b/lib/plausible/ingestion/scroll_depth_visible_at.ex index 5a84061b4c0b..8ab97e8d9004 100644 --- a/lib/plausible/ingestion/scroll_depth_visible_at.ex +++ b/lib/plausible/ingestion/scroll_depth_visible_at.ex @@ -1,41 +1,30 @@ defmodule Plausible.Ingestion.ScrollDepthVisibleAt do @moduledoc """ - GenServer that updates the `scroll_depth_visible_at` column for a site when needed. + Module that updates the `scroll_depth_visible_at` column for a site when needed. This is called in a hot loop in ingestion, so it: - 1. Only updates the database once per site (if SiteCache doesn't know about visibility yet) + 1. Only updates the database once per site async (if SiteCache doesn't know about visibility yet) 2. Does not retry the update if it fails, to be retried on server restart """ - use GenServer require Logger alias Plausible.Repo import Ecto.Query - def start_link(opts \\ []) do - opts = Keyword.merge(opts, name: __MODULE__) - GenServer.start_link(__MODULE__, opts, opts) + def init() do + :ets.new(__MODULE__, [ + :named_table, + :set, + :public, + {:read_concurrency, true}, + {:write_concurrency, true} + ]) end def mark_scroll_depth_visible(site_id) do - GenServer.cast(__MODULE__, {:update_site, site_id}) - end - - @impl true - def init(_opts) do - {:ok, MapSet.new()} - end - - @impl true - def handle_cast({:update_site, site_id}, touched_sites) do - # When receiving multiple update requests for a site, only process the first one - if MapSet.member?(touched_sites, site_id) do - {:noreply, touched_sites} - else + if :ets.insert_new(__MODULE__, {site_id}) do Task.start(fn -> attempt_update_repo(site_id) end) - - {:noreply, MapSet.put(touched_sites, site_id)} end end From 7e873c0dcd01dc5ff67b8fd01a0520f35bf4293d Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 30 Jan 2025 18:56:26 +0200 Subject: [PATCH 7/8] Additional where --- lib/plausible/ingestion/scroll_depth_visible_at.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plausible/ingestion/scroll_depth_visible_at.ex b/lib/plausible/ingestion/scroll_depth_visible_at.ex index 8ab97e8d9004..215c9e880cd9 100644 --- a/lib/plausible/ingestion/scroll_depth_visible_at.ex +++ b/lib/plausible/ingestion/scroll_depth_visible_at.ex @@ -30,7 +30,7 @@ defmodule Plausible.Ingestion.ScrollDepthVisibleAt do defp attempt_update_repo(site_id) do Repo.update_all( - from(s in Plausible.Site, where: s.id == ^site_id), + from(s in Plausible.Site, where: s.id == ^site_id and is_nil(s.scroll_depth_visible_at)), set: [ scroll_depth_visible_at: DateTime.utc_now() ] From 7bc91dc3543b1c187ca2e0b5826742a2f987b140 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Mon, 3 Feb 2025 11:30:17 +0200 Subject: [PATCH 8/8] Fix a test --- .../plausible/ingestion/scroll_depth_visible_at_test.exs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/plausible/ingestion/scroll_depth_visible_at_test.exs b/test/plausible/ingestion/scroll_depth_visible_at_test.exs index fbc89f003e67..8eb23c7ef616 100644 --- a/test/plausible/ingestion/scroll_depth_visible_at_test.exs +++ b/test/plausible/ingestion/scroll_depth_visible_at_test.exs @@ -6,9 +6,10 @@ defmodule Plausible.Ingestion.ScrollDepthVisibleAtTest do site = new_site() Plausible.Ingestion.ScrollDepthVisibleAt.mark_scroll_depth_visible(site.id) - Plausible.TestUtils.eventually(fn -> - site = Plausible.Repo.reload!(site) - {not is_nil(site.scroll_depth_visible_at), site.scroll_depth_visible_at} - end) + assert Plausible.TestUtils.eventually(fn -> + site = Plausible.Repo.reload!(site) + success = not is_nil(site.scroll_depth_visible_at) + {success, success} + end) end end