fix: cleanup Peep metrics (#499)

As we are depending on the undocumented internals of the Peep library,
we accidentally were broken by recent update in Peep where they have
added feature of stripped metrics storage. In this change there were no
more named ETS tables and there may be more than one ETS table in case
of striped tables. Now we are traversing all tables used by Peep to
cleanup them all (even though we are currently using regular store, but
this fix should work in case of striped tables as well).

lib/supavisor/monitoring/prom_ex.ex

@@ -29,7 +29,7 @@ defmodule Supavisor.Monitoring.PromEx do
     ]
   end
 
-  @spec remove_metrics(S.id()) :: non_neg_integer
+  @spec remove_metrics(S.id()) :: :ok
   def remove_metrics({{type, tenant}, user, mode, db_name, search_path} = id) do
     Logger.debug("Removing metrics for #{inspect(id)}")
 
@@ -42,11 +42,16 @@ defmodule Supavisor.Monitoring.PromEx do
       search_path: search_path
     }
 
-    Supavisor.Monitoring.PromEx.Metrics
-    |> :ets.select_delete([
-      {{{:_, meta}, :_}, [], [true]},
-      {{{:_, meta, :_}, :_}, [], [true]}
-    ])
+    {_, tids} = Peep.Persistent.storage(Supavisor.Monitoring.PromEx.Metrics)
+
+    tids
+    |> List.wrap()
+    |> Enum.each(
+      &:ets.select_delete(&1, [
+        {{{:_, meta}, :_}, [], [true]},
+        {{{:_, meta, :_}, :_}, [], [true]}
+      ])
+    )
   end
 
   @spec set_metrics_tags() :: map()

test/integration/proxy_test.exs

@@ -156,7 +156,6 @@ defmodule Supavisor.Integration.ProxyTest do
   end
 
   test "too many clients in session mode" do
-    Process.flag(:trap_exit, true)
     db_conf = Application.get_env(:supavisor, Repo)
     port = Application.get_env(:supavisor, :proxy_port_session)
 
@@ -166,8 +165,8 @@ defmodule Supavisor.Integration.ProxyTest do
         port: port
       )
 
-    {:ok, pid1} = single_connection(connection_opts)
-    {:ok, pid2} = single_connection(connection_opts)
+    assert {:ok, _} = single_connection(connection_opts)
+    assert {:ok, _} = single_connection(connection_opts)
 
     :timer.sleep(1000)
 
@@ -182,8 +181,6 @@ defmodule Supavisor.Integration.ProxyTest do
                 pg_code: "XX000"
               }
             }} = single_connection(connection_opts)
-
-    for pid <- [pid1, pid2], do: :gen_statem.stop(pid)
   end
 
   test "http to proxy server returns 200 OK" do
@@ -379,15 +376,18 @@ defmodule Supavisor.Integration.ProxyTest do
   defp single_connection(db_conf, c_port \\ nil) when is_list(db_conf) do
     port = c_port || db_conf[:port]
 
-    [
+    opts = [
       hostname: db_conf[:hostname],
       port: port,
       database: db_conf[:database],
       password: db_conf[:password],
       username: db_conf[:username],
       pool_size: 1
     ]
-    |> SingleConnection.connect()
+
+    with {:error, {error, _}} <- start_supervised({SingleConnection, opts}) do
+      {:error, error}
+    end
   end
 
   defp parse_uri(uri) do

test/supavisor/prom_ex_test.exs

@@ -1,19 +1,12 @@
 defmodule Supavisor.PromExTest do
-  use ExUnit.Case, async: true
-  use Supavisor.DataCase
+  use Supavisor.DataCase, async: true
 
   import Supavisor.Asserts
 
-  alias Supavisor.Monitoring.PromEx
+  @subject Supavisor.Monitoring.PromEx
 
   @tenant "prom_tenant"
 
-  # These tests are known to be flaky, and while these do not affect users
-  # directly we can run them independently when needed. In future we probably
-  # should make them pass "regularly", but for now that is easier to filter them
-  # out.
-  @moduletag flaky: true
-
   setup do
     db_conf = Application.get_env(:supavisor, Repo)
 
@@ -34,12 +27,14 @@ defmodule Supavisor.PromExTest do
   end
 
   test "remove tenant tag upon termination", %{proxy: proxy, user: user, db_name: db_name} do
-    assert PromEx.get_metrics() =~ "tenant=\"#{@tenant}\""
+    assert @subject.get_metrics() =~ "tenant=\"#{@tenant}\""
 
     :ok = GenServer.stop(proxy)
     :ok = Supavisor.stop({{:single, @tenant}, user, :transaction, db_name, nil})
 
-    refute_eventually(10, fn -> PromEx.get_metrics() =~ "tenant=\"#{@tenant}\"" end)
+    Process.sleep(1000)
+
+    refute_eventually(10, fn -> @subject.get_metrics() =~ "tenant=\"#{@tenant}\"" end)
   end
 
   test "clean_string/1 removes extra spaces from metric string" do
@@ -49,6 +44,6 @@ defmodule Supavisor.PromExTest do
     expected_output =
       "db_name=\"postgres\",mode=\"transaction\",tenant=\"dev_tenant\",type=\"single\",user=\"postgres\""
 
-    assert expected_output == PromEx.clean_string(input)
+    assert expected_output == @subject.clean_string(input)
   end
 end

test/support/fixtures/single_connection.ex

@@ -5,6 +5,14 @@ defmodule SingleConnection do
 
   @behaviour P.SimpleConnection
 
+  def child_spec(conf) do
+    %{
+      id: {__MODULE__, System.unique_integer()},
+      start: {__MODULE__, :connect, [conf]},
+      restart: :temporary
+    }
+  end
+
   def connect(conf) do
     P.SimpleConnection.start_link(__MODULE__, [], conf)
   end