From 6a971dd8ca308fa23798ba932a9a0c4b195eb79f Mon Sep 17 00:00:00 2001 From: Andrea Leopardi Date: Wed, 28 Aug 2024 18:06:29 +0200 Subject: [PATCH] Move source code maps to ETS (#777) --- lib/sentry/application.ex | 4 +- lib/sentry/event.ex | 5 ++- lib/sentry/sources.ex | 70 ++++++++++++++++++++++++---------- lib/sentry/transport/sender.ex | 4 ++ test/sources_test.exs | 32 +++++----------- 5 files changed, 69 insertions(+), 46 deletions(-) diff --git a/lib/sentry/application.ex b/lib/sentry/application.ex index e4386bd4..bc269c79 100644 --- a/lib/sentry/application.ex +++ b/lib/sentry/application.ex @@ -3,7 +3,7 @@ defmodule Sentry.Application do use Application - alias Sentry.{Config, Sources} + alias Sentry.Config @impl true def start(_type, _opts) do @@ -24,6 +24,7 @@ defmodule Sentry.Application do children = [ {Registry, keys: :unique, name: Sentry.Transport.SenderRegistry}, + Sentry.Sources, Sentry.Dedupe, {Sentry.Integrations.CheckInIDMappings, [ @@ -35,7 +36,6 @@ defmodule Sentry.Application do [Sentry.Transport.SenderPool] cache_loaded_applications() - _ = Sources.load_source_code_map_if_present() with {:ok, pid} <- Supervisor.start_link(children, strategy: :one_for_one, name: Sentry.Supervisor) do diff --git a/lib/sentry/event.ex b/lib/sentry/event.ex index f8ca7865..99f4dd6b 100644 --- a/lib/sentry/event.ex +++ b/lib/sentry/event.ex @@ -551,8 +551,9 @@ defmodule Sentry.Event do not Config.enable_source_code_context?() -> frame - source_map = Sources.get_source_code_map_from_persistent_term() -> - {pre_context, context, post_context} = Sources.get_source_context(source_map, file, line) + source_map_for_file = Sources.get_lines_for_file(file) -> + {pre_context, context, post_context} = + Sources.get_source_context(source_map_for_file, line) %Interfaces.Stacktrace.Frame{ frame diff --git a/lib/sentry/sources.ex b/lib/sentry/sources.ex index b89ca4a0..1376f060 100644 --- a/lib/sentry/sources.ex +++ b/lib/sentry/sources.ex @@ -1,15 +1,49 @@ defmodule Sentry.Sources do @moduledoc false + use GenServer + alias Sentry.Config + @type source_map_for_file :: %{ + optional(line_no :: pos_integer()) => line_contents :: String.t() + } + @type source_map :: %{ - optional(String.t()) => %{ - (line_no :: pos_integer()) => line_contents :: String.t() - } + optional(String.t()) => source_map_for_file() } - @source_code_map_key {:sentry, :source_code_map} + ## GenServer + + @table __MODULE__ + + @spec start_link(keyword()) :: GenServer.on_start() + def start_link([] = _) do + GenServer.start_link(__MODULE__, nil, name: __MODULE__) + end + + @impl true + def init(nil) do + _ = :ets.new(@table, [:public, :named_table, read_concurrency: true]) + {:ok, :no_state, {:continue, :load_source_code_map}} + end + + @impl true + def handle_continue(:load_source_code_map, state) do + :ok = + with {:loaded, source_map} <- load_source_code_map_if_present() do + Enum.each(source_map, fn {path, lines_map} -> + :ets.insert(@table, {path, lines_map}) + end) + else + _error -> :ok + end + + {:noreply, state} + end + + ## Other functions + @compression_level if Mix.env() == :test, do: 0, else: 9 # Default argument is here for testing. @@ -21,7 +55,6 @@ defmodule Sentry.Sources do with {:ok, contents} <- File.read(path), {:ok, source_map} <- decode_source_code_map(contents) do - :persistent_term.put(@source_code_map_key, source_map) {:loaded, source_map} else {:error, :binary_to_term} -> @@ -67,11 +100,6 @@ defmodule Sentry.Sources do end end - @spec get_source_code_map_from_persistent_term() :: source_map() | nil - def get_source_code_map_from_persistent_term do - :persistent_term.get(@source_code_map_key, nil) - end - @spec load_files(keyword()) :: {:ok, source_map()} | {:error, message :: String.t()} def load_files(config \\ []) when is_list(config) do config = Sentry.Config.validate!(config) @@ -106,23 +134,25 @@ defmodule Sentry.Sources do source_map -> {:ok, source_map} end - @spec get_source_context(source_map(), String.t() | nil, pos_integer() | nil) :: - {[String.t()], String.t() | nil, [String.t()]} - def get_source_context(%{} = files, file_name, line_number) do - context_lines = Config.context_lines() - - case Map.fetch(files, file_name) do - :error -> {[], nil, []} - {:ok, file} -> get_source_context_for_file(file, line_number, context_lines) + @spec get_lines_for_file(Path.t()) :: map() | nil + def get_lines_for_file(file) do + case :ets.lookup(@table, file) do + [{^file, lines}] -> lines + [] -> nil end end - defp get_source_context_for_file(file, line_number, context_lines) do + @spec get_source_context(source_map_for_file(), pos_integer() | nil) :: + {[String.t()], String.t() | nil, [String.t()]} + def get_source_context(source_map_for_file, line_number) + when is_map(source_map_for_file) and (is_integer(line_number) or is_nil(line_number)) do + context_lines = Config.context_lines() + context_line_indices = 0..(2 * context_lines) Enum.reduce(context_line_indices, {[], nil, []}, fn i, {pre_context, context, post_context} -> context_line_number = line_number - context_lines + i - source = Map.get(file, context_line_number) + source = Map.get(source_map_for_file, context_line_number) cond do context_line_number == line_number && source -> diff --git a/lib/sentry/transport/sender.ex b/lib/sentry/transport/sender.ex index 5a0f0d36..ca1151c7 100644 --- a/lib/sentry/transport/sender.ex +++ b/lib/sentry/transport/sender.ex @@ -32,6 +32,10 @@ defmodule Sentry.Transport.Sender do @impl GenServer def init([]) do + if function_exported?(Process, :set_label, 1) do + apply(Process, :set_label, [__MODULE__]) + end + {:ok, %__MODULE__{}} end diff --git a/test/sources_test.exs b/test/sources_test.exs index 12f41386..aa93c2ce 100644 --- a/test/sources_test.exs +++ b/test/sources_test.exs @@ -82,45 +82,33 @@ defmodule Sentry.SourcesTest do end @tag :tmp_dir - test "stores the source code map in :persistent_term if valid", %{tmp_dir: tmp_dir} do + test "reads the source code map from the file", %{tmp_dir: tmp_dir} do encoded_map = Sources.encode_source_code_map(%{"foo.ex" => %{}}) path = Path.join(tmp_dir, "valid.map") File.write!(path, encoded_map) assert {:loaded, map} = Sources.load_source_code_map_if_present(path) assert map == %{"foo.ex" => %{}} - assert :persistent_term.get({:sentry, :source_code_map}) == %{"foo.ex" => %{}} - after - :persistent_term.erase({:sentry, :source_code_map}) end end - describe "get_source_context/3" do + describe "get_source_context/2" do test "returns the correct context" do map = %{ - "foo.ex" => %{ - 1 => "defmodule Foo do", - 2 => " def bar do", - 3 => " \"bar\"", - 4 => " end", - 5 => "end" - }, - "bar.ex" => %{ - 1 => "defmodule Bar do", - 2 => " def baz do", - 3 => " \"baz\"", - 4 => " end", - 5 => "end" - } + 1 => "defmodule Foo do", + 2 => " def bar do", + 3 => " \"bar\"", + 4 => " end", + 5 => "end" } - assert {pre, context, post} = Sources.get_source_context(map, "foo.ex", 4) + assert {pre, context, post} = Sources.get_source_context(map, 4) assert pre == ["defmodule Foo do", " def bar do", " \"bar\""] assert context == " end" assert post == ["end"] end - test "works if the file doesn't exist" do - assert {[], nil, []} = Sources.get_source_context(%{}, "foo.ex", 4) + test "works if the line number doesn't exist" do + assert {[], nil, []} = Sources.get_source_context(%{}, 4) end end end