diff --git a/.sampo/changesets/resolute-prince-akka.md b/.sampo/changesets/resolute-prince-akka.md new file mode 100644 index 0000000..d15e887 --- /dev/null +++ b/.sampo/changesets/resolute-prince-akka.md @@ -0,0 +1,5 @@ +--- +hex/posthog: patch +--- + +Dedupe feature flag called events by response diff --git a/lib/posthog/feature_flags.ex b/lib/posthog/feature_flags.ex index 3e97cad..33d4ba2 100644 --- a/lib/posthog/feature_flags.ex +++ b/lib/posthog/feature_flags.ex @@ -620,7 +620,9 @@ defmodule PostHog.FeatureFlags do |> maybe_put(:"$feature_flag_payload", result.payload) |> maybe_put(:"$feature_flag_error", errors) - PostHog.capture(name, "$feature_flag_called", properties) + if PostHog.FeatureFlags.CalledCache.first_seen?(name, distinct_id, result.key, value) do + PostHog.capture(name, "$feature_flag_called", properties) + end if flag_missing? do :ok diff --git a/lib/posthog/feature_flags/called_cache.ex b/lib/posthog/feature_flags/called_cache.ex new file mode 100644 index 0000000..0ba41c6 --- /dev/null +++ b/lib/posthog/feature_flags/called_cache.ex @@ -0,0 +1,80 @@ +defmodule PostHog.FeatureFlags.CalledCache do + @moduledoc false + + use GenServer + + @max_size 50_000 + + @spec start_link(keyword()) :: GenServer.on_start() + def start_link(opts) do + supervisor_name = Keyword.fetch!(opts, :supervisor_name) + + GenServer.start_link(__MODULE__, supervisor_name, + name: PostHog.Registry.via(supervisor_name, __MODULE__) + ) + end + + @impl GenServer + def init(supervisor_name) do + table = + supervisor_name + |> table_name() + |> :ets.new([ + :set, + :public, + :named_table, + read_concurrency: true, + write_concurrency: true + ]) + + {:ok, %{table: table}} + end + + @spec first_seen?(PostHog.supervisor_name(), PostHog.distinct_id(), String.t(), any()) :: + boolean() + def first_seen?(supervisor_name, distinct_id, flag_key, value) do + key = {to_string(distinct_id), flag_key, value} + table = table_name(supervisor_name) + + case :ets.insert_new(table, {key}) do + true -> + rollover_if_full(supervisor_name, table, key) + true + + false -> + false + end + rescue + ArgumentError -> true + catch + :exit, _ -> true + end + + @impl GenServer + def handle_call({:rollover, key}, _from, %{table: table} = state) do + if over_max_size?(table) do + # Intentionally flush instead of evicting individual entries to keep + # the hot path simple. Previously seen values may emit again after + # the cache rolls over. + :ets.delete_all_objects(table) + :ets.insert(table, {key}) + end + + {:reply, :ok, state} + end + + defp rollover_if_full(supervisor_name, table, key) do + if over_max_size?(table) do + GenServer.call(PostHog.Registry.via(supervisor_name, __MODULE__), {:rollover, key}) + end + end + + defp over_max_size?(table) do + case :ets.info(table, :size) do + size when is_integer(size) -> size > @max_size + _ -> false + end + end + + defp table_name(supervisor_name), do: Module.concat(supervisor_name, CalledCacheTable) +end diff --git a/lib/posthog/supervisor.ex b/lib/posthog/supervisor.ex index 9a50539..ac0e6ec 100644 --- a/lib/posthog/supervisor.ex +++ b/lib/posthog/supervisor.ex @@ -57,7 +57,8 @@ defmodule PostHog.Supervisor do {Registry, keys: :unique, name: PostHog.Registry.registry_name(config.supervisor_name), - meta: [config: config]} + meta: [config: config]}, + {PostHog.FeatureFlags.CalledCache, supervisor_name: config.supervisor_name} ] ++ senders(config) ++ sources(config) Process.put(:"$callers", callers) diff --git a/test/posthog/feature_flags/called_cache_test.exs b/test/posthog/feature_flags/called_cache_test.exs new file mode 100644 index 0000000..1a4ba13 --- /dev/null +++ b/test/posthog/feature_flags/called_cache_test.exs @@ -0,0 +1,45 @@ +defmodule PostHog.FeatureFlags.CalledCacheTest do + use PostHog.Case, async: true + + alias PostHog.FeatureFlags.CalledCache + + @max_size 50_000 + + setup :setup_supervisor + + test "returns true when the supervisor registry is not running" do + supervisor_name = __MODULE__.MissingSupervisor + + refute Process.whereis(PostHog.Registry.registry_name(supervisor_name)) + + assert CalledCache.first_seen?(supervisor_name, "user", "flag", true) + end + + test "flushes the cache when it reaches the maximum size", %{config: config} do + supervisor_name = config.supervisor_name + seed_key = {"seed-user", "flag", true} + table = table(supervisor_name) + + full_cache = + 1..(@max_size - 1) + |> Enum.map(&{{"user-#{&1}", "flag", true}}) + |> then(&[{seed_key} | &1]) + + :ets.insert(table, full_cache) + + refute CalledCache.first_seen?(supervisor_name, "seed-user", "flag", true) + + assert CalledCache.first_seen?(supervisor_name, "overflow-user", "flag", true) + assert CalledCache.first_seen?(supervisor_name, "seed-user", "flag", true) + refute CalledCache.first_seen?(supervisor_name, "overflow-user", "flag", true) + end + + defp table(supervisor_name) do + [{pid, _value}] = + Registry.lookup(PostHog.Registry.registry_name(supervisor_name), CalledCache) + + pid + |> :sys.get_state() + |> Map.fetch!(:table) + end +end diff --git a/test/posthog/feature_flags/evaluations_test.exs b/test/posthog/feature_flags/evaluations_test.exs index 406631f..85d7ad8 100644 --- a/test/posthog/feature_flags/evaluations_test.exs +++ b/test/posthog/feature_flags/evaluations_test.exs @@ -224,12 +224,29 @@ defmodule PostHog.FeatureFlags.EvaluationsTest do assert properties["$feature/variant-flag"] == "control" end - test "fires on every access (no dedup in this PR)", %{snapshot: snapshot} do + test "dedupes repeated access for the same flag value", %{snapshot: snapshot} do Evaluations.enabled?(snapshot, "boolean-flag") Evaluations.enabled?(snapshot, "boolean-flag") Evaluations.get_flag(snapshot, "boolean-flag") - assert length(all_captured()) == 3 + assert [%{event: "$feature_flag_called"}] = all_captured() + end + + test "fires again when the same flag returns a different value" do + true_result = %FeatureFlags.Result{key: "changing-flag", enabled: true} + false_result = %FeatureFlags.Result{key: "changing-flag", enabled: false} + + FeatureFlags.log_feature_flag_usage(PostHog, "foo", true_result) + FeatureFlags.log_feature_flag_usage(PostHog, "foo", false_result) + FeatureFlags.log_feature_flag_usage(PostHog, "foo", true_result) + FeatureFlags.log_feature_flag_usage(PostHog, "foo", false_result) + + events = all_captured() + assert length(events) == 2 + + assert events + |> Enum.map(& &1.properties[:"$feature_flag_response"]) + |> Enum.sort() == [false, true] end end