From 26831641032392ed47fbcd7fb787e79b7c8b26f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Jan=20Niemier?= Date: Wed, 27 Nov 2024 14:43:05 +0100 Subject: [PATCH] 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 | 17 +++++++++++------ test/integration/proxy_test.exs | 14 +++++++------- test/supavisor/prom_ex_test.exs | 19 +++++++------------ test/support/fixtures/single_connection.ex | 8 ++++++++ 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/lib/supavisor/monitoring/prom_ex.ex b/lib/supavisor/monitoring/prom_ex.ex index 1d0faa72..58f43e6b 100644 --- a/lib/supavisor/monitoring/prom_ex.ex +++ b/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() diff --git a/test/integration/proxy_test.exs b/test/integration/proxy_test.exs index 68f15bb1..c792f7d2 100644 --- a/test/integration/proxy_test.exs +++ b/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,7 +376,7 @@ 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], @@ -387,7 +384,10 @@ defmodule Supavisor.Integration.ProxyTest do 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 diff --git a/test/supavisor/prom_ex_test.exs b/test/supavisor/prom_ex_test.exs index 27edfc0c..de68f590 100644 --- a/test/supavisor/prom_ex_test.exs +++ b/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 diff --git a/test/support/fixtures/single_connection.ex b/test/support/fixtures/single_connection.ex index 2f3d89b5..b35d6918 100644 --- a/test/support/fixtures/single_connection.ex +++ b/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