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

Scroll depth: update site.scroll_depth_visible_at in ingestion #5035

Merged
merged 8 commits into from
Feb 3, 2025
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Simplify code and test genserver directly
macobo committed Jan 30, 2025

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
commit 76d5dc98b274e7b3ec290e30d407f6ea8f789499
70 changes: 20 additions & 50 deletions lib/plausible/ingestion/scroll_depth_visible_at.ex
Original file line number Diff line number Diff line change
@@ -1,85 +1,55 @@
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
alias Plausible.Repo

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
GenServer.cast(__MODULE__, {:update_site, site_id})
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),
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."
"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
14 changes: 14 additions & 0 deletions test/plausible/ingestion/scroll_depth_visible_at_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
defmodule Plausible.Ingestion.ScrollDepthVisibleAtTest do
use Plausible.DataCase
use Plausible.Teams.Test

test "mark_scroll_depth_visible" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test can never fail. We should assert on the return value of TestUtils.eventually

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