Merge branch 'feature/1790-oban-overuse' into 'develop'

removing StatsWorker from Oban cron jobs

See merge request pleroma/pleroma!2963

CHANGELOG.md

@@ -3,12 +3,21 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
+## Unreleased
+
+### Removed
+
+- **Breaking:** Removed `Pleroma.Workers.Cron.StatsWorker` setting from Oban `:crontab`.
+
+
 ## unreleased-patch - ???
 
 ### Added
+
 - Rich media failure tracking (along with `:failure_backoff` option)
 
 ### Fixed
+
 - Possible OOM errors with the default HTTP adapter
 - Mastodon API: Search parameter `following` now correctly returns the followings rather than the followers
 - Mastodon API: Timelines hanging for (`number of posts with links * rich media timeout`) in the worst case.
@@ -16,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Mastodon API: Cards being wrong for preview statuses due to cache key collision
 - Password resets no longer processed for deactivated accounts
 
+
 ## [2.1.0] - 2020-08-28
 
 ### Changed

config/config.exs

@@ -546,7 +546,6 @@
   plugins: [Oban.Plugins.Pruner],
   crontab: [
     {"0 0 * * *", Pleroma.Workers.Cron.ClearOauthTokenWorker},
-    {"0 * * * *", Pleroma.Workers.Cron.StatsWorker},
     {"* * * * *", Pleroma.Workers.Cron.PurgeExpiredActivitiesWorker},
     {"0 0 * * 0", Pleroma.Workers.Cron.DigestEmailsWorker},
     {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker}

config/description.exs

@@ -2291,7 +2291,6 @@
         description: "Settings for cron background jobs",
         suggestions: [
           {"0 0 * * *", Pleroma.Workers.Cron.ClearOauthTokenWorker},
-          {"0 * * * *", Pleroma.Workers.Cron.StatsWorker},
           {"* * * * *", Pleroma.Workers.Cron.PurgeExpiredActivitiesWorker},
           {"0 0 * * 0", Pleroma.Workers.Cron.DigestEmailsWorker},
           {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker}

lib/mix/pleroma.ex

@@ -18,6 +18,7 @@ defmodule Mix.Pleroma do
   @doc "Common functions to be reused in mix tasks"
   def start_pleroma do
     Pleroma.Config.Holder.save_default()
+    Pleroma.Config.Oban.warn()
     Application.put_env(:phoenix, :serve_endpoints, false, persistent: true)
 
     if Pleroma.Config.get(:env) != :test do

lib/pleroma/application.ex

@@ -50,6 +50,7 @@ def start(_type, _args) do
     Pleroma.Telemetry.Logger.attach()
     Config.Holder.save_default()
     Pleroma.HTML.compile_scrubbers()
+    Pleroma.Config.Oban.warn()
     Config.DeprecationWarnings.warn()
     Pleroma.Plugs.HTTPSecurityPlug.warn_if_disabled()
     Pleroma.ApplicationRequirements.verify!()

lib/pleroma/config/oban.ex

@@ -0,0 +1,30 @@
+defmodule Pleroma.Config.Oban do
+  require Logger
+
+  def warn do
+    oban_config = Pleroma.Config.get(Oban)
+
+    crontab =
+      [Pleroma.Workers.Cron.StatsWorker]
+      |> Enum.reduce(oban_config[:crontab], fn removed_worker, acc ->
+        with acc when is_list(acc) <- acc,
+             setting when is_tuple(setting) <-
+               Enum.find(acc, fn {_, worker} -> worker == removed_worker end) do
+          """
+          !!!OBAN CONFIG WARNING!!!
+          You are using old workers in Oban crontab settings, which were removed.
+          Please, remove setting from crontab in your config file (prod.secret.exs): #{
+            inspect(setting)
+          }
+          """
+          |> Logger.warn()
+
+          List.delete(acc, setting)
+        else
+          _ -> acc
+        end
+      end)
+
+    Pleroma.Config.put(Oban, Keyword.put(oban_config, :crontab, crontab))
+  end
+end

lib/pleroma/stats.ex

@@ -3,12 +3,15 @@
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Stats do
+  use GenServer
+
   import Ecto.Query
+
   alias Pleroma.CounterCache
   alias Pleroma.Repo
   alias Pleroma.User
 
-  use GenServer
+  @interval :timer.seconds(60)
 
   def start_link(_) do
     GenServer.start_link(
@@ -18,6 +21,11 @@ def start_link(_) do
     )
   end
 
+  @impl true
+  def init(_args) do
+    {:ok, nil, {:continue, :calculate_stats}}
+  end
+
   @doc "Performs update stats"
   def force_update do
     GenServer.call(__MODULE__, :force_update)
@@ -29,7 +37,11 @@ def do_collect do
   end
 
   @doc "Returns stats data"
-  @spec get_stats() :: %{domain_count: integer(), status_count: integer(), user_count: integer()}
+  @spec get_stats() :: %{
+          domain_count: non_neg_integer(),
+          status_count: non_neg_integer(),
+          user_count: non_neg_integer()
+        }
   def get_stats do
     %{stats: stats} = GenServer.call(__MODULE__, :get_state)
 
@@ -44,25 +56,14 @@ def get_peers do
     peers
   end
 
-  def init(_args) do
-    {:ok, calculate_stat_data()}
-  end
-
-  def handle_call(:force_update, _from, _state) do
-    new_stats = calculate_stat_data()
-    {:reply, new_stats, new_stats}
-  end
-
-  def handle_call(:get_state, _from, state) do
-    {:reply, state, state}
-  end
-
-  def handle_cast(:run_update, _state) do
-    new_stats = calculate_stat_data()
-
-    {:noreply, new_stats}
-  end
-
+  @spec calculate_stat_data() :: %{
+          peers: list(),
+          stats: %{
+            domain_count: non_neg_integer(),
+            status_count: non_neg_integer(),
+            user_count: non_neg_integer()
+          }
+        }
   def calculate_stat_data do
     peers =
       from(
@@ -97,6 +98,7 @@ def calculate_stat_data do
     }
   end
 
+  @spec get_status_visibility_count(String.t() | nil) :: map()
   def get_status_visibility_count(instance \\ nil) do
     if is_nil(instance) do
       CounterCache.get_sum()
@@ -104,4 +106,36 @@ def get_status_visibility_count(instance \\ nil) do
       CounterCache.get_by_instance(instance)
     end
   end
+
+  @impl true
+  def handle_continue(:calculate_stats, _) do
+    stats = calculate_stat_data()
+    Process.send_after(self(), :run_update, @interval)
+    {:noreply, stats}
+  end
+
+  @impl true
+  def handle_call(:force_update, _from, _state) do
+    new_stats = calculate_stat_data()
+    {:reply, new_stats, new_stats}
+  end
+
+  @impl true
+  def handle_call(:get_state, _from, state) do
+    {:reply, state, state}
+  end
+
+  @impl true
+  def handle_cast(:run_update, _state) do
+    new_stats = calculate_stat_data()
+
+    {:noreply, new_stats}
+  end
+
+  @impl true
+  def handle_info(:run_update, _) do
+    new_stats = calculate_stat_data()
+    Process.send_after(self(), :run_update, @interval)
+    {:noreply, new_stats}
+  end
 end

lib/pleroma/workers/cron/stats_worker.ex

@@ -1,17 +0,0 @@
-# Pleroma: A lightweight social networking server
-# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
-# SPDX-License-Identifier: AGPL-3.0-only
-
-defmodule Pleroma.Workers.Cron.StatsWorker do
-  @moduledoc """
-  The worker to update peers statistics.
-  """
-
-  use Oban.Worker, queue: "background"
-
-  @impl Oban.Worker
-  def perform(_job) do
-    Pleroma.Stats.do_collect()
-    :ok
-  end
-end

priv/repo/migrations/20200906072147_remove_cron_stats_worker_from_oban_config.exs

@@ -0,0 +1,19 @@
+defmodule Pleroma.Repo.Migrations.RemoveCronStatsWorkerFromObanConfig do
+  use Ecto.Migration
+
+  def change do
+    with %Pleroma.ConfigDB{} = config <-
+           Pleroma.ConfigDB.get_by_params(%{group: :pleroma, key: Oban}),
+         crontab when is_list(crontab) <- config.value[:crontab],
+         index when is_integer(index) <-
+           Enum.find_index(crontab, fn {_, worker} ->
+             worker == Pleroma.Workers.Cron.StatsWorker
+           end) do
+      updated_value = Keyword.put(config.value, :crontab, List.delete_at(crontab, index))
+
+      config
+      |> Ecto.Changeset.change(value: updated_value)
+      |> Pleroma.Repo.update()
+    end
+  end
+end

test/stats_test.exs

@@ -4,7 +4,10 @@
 
 defmodule Pleroma.StatsTest do
   use Pleroma.DataCase
+
   import Pleroma.Factory
+
+  alias Pleroma.Stats
   alias Pleroma.Web.CommonAPI
 
   describe "user count" do
@@ -13,7 +16,7 @@ test "it ignores internal users" do
       _internal = insert(:user, local: true, nickname: nil)
       _internal = Pleroma.Web.ActivityPub.Relay.get_actor()
 
-      assert match?(%{stats: %{user_count: 1}}, Pleroma.Stats.calculate_stat_data())
+      assert match?(%{stats: %{user_count: 1}}, Stats.calculate_stat_data())
     end
   end
 
@@ -47,23 +50,23 @@ test "on new status" do
       end)
 
       assert %{"direct" => 3, "private" => 4, "public" => 1, "unlisted" => 2} =
-               Pleroma.Stats.get_status_visibility_count()
+               Stats.get_status_visibility_count()
     end
 
     test "on status delete" do
       user = insert(:user)
       {:ok, activity} = CommonAPI.post(user, %{visibility: "public", status: "hey"})
-      assert %{"public" => 1} = Pleroma.Stats.get_status_visibility_count()
+      assert %{"public" => 1} = Stats.get_status_visibility_count()
       CommonAPI.delete(activity.id, user)
-      assert %{"public" => 0} = Pleroma.Stats.get_status_visibility_count()
+      assert %{"public" => 0} = Stats.get_status_visibility_count()
     end
 
     test "on status visibility update" do
       user = insert(:user)
       {:ok, activity} = CommonAPI.post(user, %{visibility: "public", status: "hey"})
-      assert %{"public" => 1, "private" => 0} = Pleroma.Stats.get_status_visibility_count()
+      assert %{"public" => 1, "private" => 0} = Stats.get_status_visibility_count()
       {:ok, _} = CommonAPI.update_activity_scope(activity.id, %{visibility: "private"})
-      assert %{"public" => 0, "private" => 1} = Pleroma.Stats.get_status_visibility_count()
+      assert %{"public" => 0, "private" => 1} = Stats.get_status_visibility_count()
     end
 
     test "doesn't count unrelated activities" do
@@ -75,7 +78,7 @@ test "doesn't count unrelated activities" do
       CommonAPI.repeat(activity.id, other_user)
 
       assert %{"direct" => 0, "private" => 0, "public" => 1, "unlisted" => 0} =
-               Pleroma.Stats.get_status_visibility_count()
+               Stats.get_status_visibility_count()
     end
   end
 
@@ -110,10 +113,10 @@ test "single instance" do
       end)
 
       assert %{"direct" => 10, "private" => 0, "public" => 1, "unlisted" => 5} =
-               Pleroma.Stats.get_status_visibility_count(local_instance)
+               Stats.get_status_visibility_count(local_instance)
 
       assert %{"direct" => 0, "private" => 20, "public" => 0, "unlisted" => 0} =
-               Pleroma.Stats.get_status_visibility_count(instance2)
+               Stats.get_status_visibility_count(instance2)
     end
   end
 end