From cb075aa2469273f606896c4c2036c1a6ca7d951d Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Tue, 26 Mar 2024 15:07:10 -0500 Subject: [PATCH 01/10] refactor Airbrake.Payload functions --- lib/airbrake/payload.ex | 49 +++++++++++++---------------------------- 1 file changed, 15 insertions(+), 34 deletions(-) diff --git a/lib/airbrake/payload.ex b/lib/airbrake/payload.ex index 8895b7e..1b8d0b1 100644 --- a/lib/airbrake/payload.ex +++ b/lib/airbrake/payload.ex @@ -28,34 +28,25 @@ defmodule Airbrake.Payload do end def new(exception, stacktrace, options) when is_list(exception) do - %__MODULE__{} - |> add_error( - exception, - stacktrace, - Keyword.get(options, :context), - Keyword.get(options, :env), - Keyword.get(options, :params), - Keyword.get(options, :session) - ) + %__MODULE__{ + errors: [build_error(exception, stacktrace)], + context: Map.merge(%{environment: env(), hostname: hostname()}, get_option(options, :context) || %{}), + environment: options |> get_option(:env) |> filter_environment(), + params: options |> get_option(:params) |> filter_parameters(), + session: get_option(options, :session) + } end - defp add_error(payload, exception, stacktrace, context, env, params, session) do - payload - |> add_exception_info(exception, stacktrace) - |> add_context(context) - |> add_env(env) - |> add_params(params) - |> add_session(session) + defp get_option(options, key) do + Keyword.get(options, key) end - defp add_exception_info(payload, exception, stacktrace) do - error = %{ + defp build_error(exception, stacktrace) do + %{ type: exception[:type], message: exception[:message], backtrace: Backtrace.from_stacktrace(stacktrace) } - - Map.put(payload, :errors, [error]) end defp env do @@ -72,22 +63,12 @@ defmodule Airbrake.Payload do System.get_env("HOST") || to_string(elem(:inet.gethostname(), 1)) end - defp add_context(payload, context) do - context = Map.merge(%{environment: env(), hostname: hostname()}, context || %{}) - Map.put(payload, :context, context) - end - - defp add_env(payload, nil), do: payload - defp add_env(payload, env), do: Map.put(payload, :environment, filter_environment(env)) - - defp add_params(payload, nil), do: payload - defp add_params(payload, params), do: Map.put(payload, :params, filter_parameters(params)) - - defp add_session(payload, nil), do: payload - defp add_session(payload, session), do: Map.put(payload, :session, session) - defp filter_parameters(params), do: filter(params, :filter_parameters) + defp filter_environment(nil) do + nil + end + defp filter_environment(env) do if Map.has_key?(env, "headers"), do: Map.update!(env, "headers", &filter(&1, :filter_headers)), From 5621f73bc554f7d71bb61e2efc31e7413e214680 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Tue, 26 Mar 2024 15:12:24 -0500 Subject: [PATCH 02/10] refactor Airbrake.Payload functions --- lib/airbrake/payload.ex | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/lib/airbrake/payload.ex b/lib/airbrake/payload.ex index 1b8d0b1..ac5085d 100644 --- a/lib/airbrake/payload.ex +++ b/lib/airbrake/payload.ex @@ -30,17 +30,13 @@ defmodule Airbrake.Payload do def new(exception, stacktrace, options) when is_list(exception) do %__MODULE__{ errors: [build_error(exception, stacktrace)], - context: Map.merge(%{environment: env(), hostname: hostname()}, get_option(options, :context) || %{}), - environment: options |> get_option(:env) |> filter_environment(), - params: options |> get_option(:params) |> filter_parameters(), - session: get_option(options, :session) + context: build(:context, options), + environment: build(:environment, options), + params: build(:params, options), + session: build(:session, options) } end - defp get_option(options, key) do - Keyword.get(options, key) - end - defp build_error(exception, stacktrace) do %{ type: exception[:type], @@ -49,6 +45,25 @@ defmodule Airbrake.Payload do } end + defp build(:context, options) do + Map.merge( + %{environment: env(), hostname: hostname()}, + Keyword.get(options, :context, %{}) + ) + end + + defp build(:environment, options) do + options |> Keyword.get(:env) |> filter_environment() + end + + defp build(:params, options) do + options |> Keyword.get(:params) |> filter_parameters() + end + + defp build(key, options) do + Keyword.get(options, key) + end + defp env do case Application.get_env(:airbrake_client, :environment) do nil -> hostname() From 469f0c13451329ad588ef751187fa58f8298cab8 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Tue, 26 Mar 2024 15:17:00 -0500 Subject: [PATCH 03/10] rename a function --- lib/airbrake/payload.ex | 2 +- lib/airbrake/worker.ex | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/airbrake/payload.ex b/lib/airbrake/payload.ex index ac5085d..a62cfc1 100644 --- a/lib/airbrake/payload.ex +++ b/lib/airbrake/payload.ex @@ -91,6 +91,6 @@ defmodule Airbrake.Payload do end defp filter(map, attributes_key) do - Utils.filter(map, Airbrake.Worker.get_env(attributes_key)) + Utils.filter(map, Airbrake.Worker.get_config(attributes_key)) end end diff --git a/lib/airbrake/worker.ex b/lib/airbrake/worker.ex index e6298d5..8f8f282 100644 --- a/lib/airbrake/worker.ex +++ b/lib/airbrake/worker.ex @@ -108,7 +108,7 @@ defmodule Airbrake.Worker do end defp build_options(current_options) do - case get_env(:options) do + case get_config(:options) do {mod, fun, 1} -> apply(mod, fun, [current_options]) @@ -126,7 +126,7 @@ defmodule Airbrake.Worker do end defp ignore?(type: type, message: message) do - ignore?(get_env(:ignore), type, message) + ignore?(get_config(:ignore), type, message) end defp ignore?(nil, _type, _message), do: false @@ -139,20 +139,20 @@ defmodule Airbrake.Worker do defp notify_url do Path.join([ - get_env(:host, @default_host), + get_config(:host, @default_host), "api/v3/projects", - :project_id |> get_env() |> to_string(), - "notices?key=#{get_env(:api_key)}" + :project_id |> get_config() |> to_string(), + "notices?key=#{get_config(:api_key)}" ]) end - def get_env(key, default \\ nil) do + def get_config(key, default \\ nil) do :airbrake_client |> Application.get_env(key, default) - |> process_env() + |> process_config() end - defp process_env({:system, key, default}), do: System.get_env(key) || default - defp process_env({:system, key}), do: System.get_env(key) - defp process_env(value), do: value + defp process_config({:system, key, default}), do: System.get_env(key) || default + defp process_config({:system, key}), do: System.get_env(key) + defp process_config(value), do: value end From 3fb38f877127bbf1a5bc6d6439afebc237e49a5d Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Mon, 1 Apr 2024 12:55:23 -0500 Subject: [PATCH 04/10] extract config from config/dev.exs to config/runtime.exs --- config/dev.exs | 5 ++--- config/runtime.exs | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) create mode 100644 config/runtime.exs diff --git a/config/dev.exs b/config/dev.exs index db78276..c1a11b6 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -1,8 +1,7 @@ import Config # These settings can be used on the iex console. +# More are set in `config/runtime.exs`. config :airbrake_client, - api_key: {:system, "AIRBRAKE_API_KEY"}, - project_id: {:system, "AIRBRAKE_PROJECT_ID"}, - host: {:system, "AIRBRAKE_HOST", "https://api.airbrake.io"}, + session: :include_logger_metadata, private: [http_adapter: HTTPoison] diff --git a/config/runtime.exs b/config/runtime.exs new file mode 100644 index 0000000..bb33950 --- /dev/null +++ b/config/runtime.exs @@ -0,0 +1,15 @@ +import Config + +case config_env() do + :dev -> + config :airbrake_client, + api_key: System.get_env("AIRBRAKE_API_KEY"), + project_id: System.get_env("AIRBRAKE_PROJECT_ID"), + host: System.get_env("AIRBRAKE_HOST", "https://api.airbrake.io") + + :test -> + nil + + :prod -> + nil +end From bc68e5dc0137179b24cbfa74b05dc0e89c923390 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Tue, 26 Mar 2024 15:25:39 -0500 Subject: [PATCH 05/10] extract Airbrake.Config --- lib/airbrake/config.ex | 47 +++++++++++++++++++++++++++++++++++++++++ lib/airbrake/payload.ex | 22 +++++-------------- lib/airbrake/worker.ex | 24 +++++++++------------ test/support/mocks.ex | 1 + 4 files changed, 63 insertions(+), 31 deletions(-) create mode 100644 lib/airbrake/config.ex diff --git a/lib/airbrake/config.ex b/lib/airbrake/config.ex new file mode 100644 index 0000000..bb60b7f --- /dev/null +++ b/lib/airbrake/config.ex @@ -0,0 +1,47 @@ +defmodule Airbrake.Config do + @moduledoc false + + defmodule Behaviour do + @moduledoc false + + @callback get(atom()) :: any() + + @callback get(atom(), any()) :: any() + + @callback env :: String.t() + + @callback hostname :: String.t() + end + + @behaviour Airbrake.Config.Behaviour + + # Gets a value from the `:airbrake_client` config. + @impl Airbrake.Config.Behaviour + def get(key, default \\ nil) do + :airbrake_client + |> Application.get_env(key, default) + |> resolve() + end + + # Returns the name of the environment. + @impl Airbrake.Config.Behaviour + def env do + case Application.get_env(:airbrake_client, :environment) do + nil -> hostname() + {:system, var} -> System.get_env(var, hostname()) + atom_env when is_atom(atom_env) -> to_string(atom_env) + str_env when is_binary(str_env) -> str_env + fun_env when is_function(fun_env) -> fun_env.() + end + end + + # Returns a hostname. + @impl Airbrake.Config.Behaviour + def hostname do + System.get_env("HOST") || to_string(elem(:inet.gethostname(), 1)) + end + + defp resolve({:system, key, default}), do: System.get_env(key) || default + defp resolve({:system, key}), do: System.get_env(key) + defp resolve(value), do: value +end diff --git a/lib/airbrake/payload.ex b/lib/airbrake/payload.ex index a62cfc1..1d55be8 100644 --- a/lib/airbrake/payload.ex +++ b/lib/airbrake/payload.ex @@ -1,6 +1,8 @@ defmodule Airbrake.Payload do @moduledoc false + alias Airbrake.Config + @notifier_info %{ name: "Airbrake Client", version: Airbrake.Mixfile.project()[:version], @@ -47,7 +49,7 @@ defmodule Airbrake.Payload do defp build(:context, options) do Map.merge( - %{environment: env(), hostname: hostname()}, + %{environment: Config.env(), hostname: Config.hostname()}, Keyword.get(options, :context, %{}) ) end @@ -64,20 +66,6 @@ defmodule Airbrake.Payload do Keyword.get(options, key) end - defp env do - case Application.get_env(:airbrake_client, :environment) do - nil -> hostname() - {:system, var} -> System.get_env(var) || hostname() - atom_env when is_atom(atom_env) -> to_string(atom_env) - str_env when is_binary(str_env) -> str_env - fun_env when is_function(fun_env) -> fun_env.() - end - end - - def hostname do - System.get_env("HOST") || to_string(elem(:inet.gethostname(), 1)) - end - defp filter_parameters(params), do: filter(params, :filter_parameters) defp filter_environment(nil) do @@ -90,7 +78,7 @@ defmodule Airbrake.Payload do else: env end - defp filter(map, attributes_key) do - Utils.filter(map, Airbrake.Worker.get_config(attributes_key)) + defp filter(map, config_key) do + Utils.filter(map, Config.get(config_key)) end end diff --git a/lib/airbrake/worker.ex b/lib/airbrake/worker.ex index 8f8f282..3c6b984 100644 --- a/lib/airbrake/worker.ex +++ b/lib/airbrake/worker.ex @@ -7,6 +7,8 @@ defmodule Airbrake.Worker do defstruct refs: %{}, last_exception: nil end + alias Airbrake.Config + @name __MODULE__ @request_headers [{"Content-Type", "application/json"}] @default_host "https://api.airbrake.io" @@ -108,7 +110,7 @@ defmodule Airbrake.Worker do end defp build_options(current_options) do - case get_config(:options) do + case Config.get(:options) do {mod, fun, 1} -> apply(mod, fun, [current_options]) @@ -126,7 +128,7 @@ defmodule Airbrake.Worker do end defp ignore?(type: type, message: message) do - ignore?(get_config(:ignore), type, message) + ignore?(Config.get(:ignore), type, message) end defp ignore?(nil, _type, _message), do: false @@ -139,20 +141,14 @@ defmodule Airbrake.Worker do defp notify_url do Path.join([ - get_config(:host, @default_host), + Config.get(:host, @default_host), "api/v3/projects", - :project_id |> get_config() |> to_string(), - "notices?key=#{get_config(:api_key)}" + :project_id |> Config.get() |> to_string(), + "notices?key=#{Config.get(:api_key)}" ]) end - def get_config(key, default \\ nil) do - :airbrake_client - |> Application.get_env(key, default) - |> process_config() - end - - defp process_config({:system, key, default}), do: System.get_env(key) || default - defp process_config({:system, key}), do: System.get_env(key) - defp process_config(value), do: value + @deprecated "Use Airbrake.Config.get/2 instead." + def get_env(key, default \\ nil), + do: Config.get(key, default) end diff --git a/test/support/mocks.ex b/test/support/mocks.ex index a47b665..f242119 100644 --- a/test/support/mocks.ex +++ b/test/support/mocks.ex @@ -1 +1,2 @@ Mox.defmock(Airbrake.HTTPMock, for: HTTPoison.Base) +Mox.defmock(MockConfig, for: Airbrake.Config.Behaviour) From f11ec158ef0da65d166da4df434d23f203725f1d Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Wed, 27 Mar 2024 11:39:24 -0500 Subject: [PATCH 06/10] bump dialyxir to ~> 1.4 --- mix.exs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/mix.exs b/mix.exs index f2585c4..31bd295 100644 --- a/mix.exs +++ b/mix.exs @@ -59,9 +59,7 @@ defmodule Airbrake.Mixfile do defp deps do [ {:credo, "~> 1.7", only: [:dev, :test], runtime: false}, - # 1.4 is not compilable with Elixir <1.12. - # For test CI, we just need to _compile_ dialyxir on earlier versions. - {:dialyxir, "~> 1.3.0", only: [:dev, :test], runtime: false}, + {:dialyxir, "~> 1.4", only: [:dev, :test], runtime: false}, {:ex_doc, "~> 0.30", only: [:dev, :test]}, {:excoveralls, "~> 0.18", only: :test}, {:httpoison, "~> 1.0 or ~> 2.0"}, From 7134926df96360663cb09d49c6d676f737f7236e Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Wed, 27 Mar 2024 14:18:06 -0500 Subject: [PATCH 07/10] extract Airbrake.Payload.Builder with better tests --- config/test.exs | 3 +- lib/airbrake/payload.ex | 66 ++----- lib/airbrake/payload/builder.ex | 76 ++++++++ test/airbrake/payload/builder_test.exs | 229 +++++++++++++++++++++++++ test/airbrake/payload_test.exs | 11 +- 5 files changed, 323 insertions(+), 62 deletions(-) create mode 100644 lib/airbrake/payload/builder.ex create mode 100644 test/airbrake/payload/builder_test.exs diff --git a/config/test.exs b/config/test.exs index d78227a..54db463 100644 --- a/config/test.exs +++ b/config/test.exs @@ -4,4 +4,5 @@ import Config config :airbrake_client, api_key: "TESTING_API_KEY", project_id: 8_675_309, - private: [http_adapter: Airbrake.HTTPMock] + private: [http_adapter: Airbrake.HTTPMock], + filter_parameters: ["password"] diff --git a/lib/airbrake/payload.ex b/lib/airbrake/payload.ex index 1d55be8..0e29028 100644 --- a/lib/airbrake/payload.ex +++ b/lib/airbrake/payload.ex @@ -1,7 +1,7 @@ defmodule Airbrake.Payload do @moduledoc false - alias Airbrake.Config + alias Airbrake.Payload.Builder @notifier_info %{ name: "Airbrake Client", @@ -20,65 +20,19 @@ defmodule Airbrake.Payload do params: nil, session: nil - alias Airbrake.Payload.Backtrace - alias Airbrake.Utils + def new(exception, stacktrace, opts \\ []) - def new(exception, stacktrace, options \\ []) - - def new(%{__exception__: true} = exception, stacktrace, options) do - new(Airbrake.Worker.exception_info(exception), stacktrace, options) + def new(%{__exception__: true} = exception, stacktrace, opts) do + new(Airbrake.Worker.exception_info(exception), stacktrace, opts) end - def new(exception, stacktrace, options) when is_list(exception) do + def new(exception, stacktrace, opts) when is_list(exception) do %__MODULE__{ - errors: [build_error(exception, stacktrace)], - context: build(:context, options), - environment: build(:environment, options), - params: build(:params, options), - session: build(:session, options) + errors: [Builder.build_error(exception, stacktrace)], + context: Builder.build(:context, opts), + environment: Builder.build(:environment, opts), + params: Builder.build(:params, opts), + session: Builder.build(:session, opts) } end - - defp build_error(exception, stacktrace) do - %{ - type: exception[:type], - message: exception[:message], - backtrace: Backtrace.from_stacktrace(stacktrace) - } - end - - defp build(:context, options) do - Map.merge( - %{environment: Config.env(), hostname: Config.hostname()}, - Keyword.get(options, :context, %{}) - ) - end - - defp build(:environment, options) do - options |> Keyword.get(:env) |> filter_environment() - end - - defp build(:params, options) do - options |> Keyword.get(:params) |> filter_parameters() - end - - defp build(key, options) do - Keyword.get(options, key) - end - - defp filter_parameters(params), do: filter(params, :filter_parameters) - - defp filter_environment(nil) do - nil - end - - defp filter_environment(env) do - if Map.has_key?(env, "headers"), - do: Map.update!(env, "headers", &filter(&1, :filter_headers)), - else: env - end - - defp filter(map, config_key) do - Utils.filter(map, Config.get(config_key)) - end end diff --git a/lib/airbrake/payload/builder.ex b/lib/airbrake/payload/builder.ex new file mode 100644 index 0000000..6e6053a --- /dev/null +++ b/lib/airbrake/payload/builder.ex @@ -0,0 +1,76 @@ +defmodule Airbrake.Payload.Builder do + @moduledoc false + + alias Airbrake.Payload.Backtrace + alias Airbrake.Utils + + def build_error(exception, stacktrace) do + %{ + type: exception[:type], + message: exception[:message], + backtrace: Backtrace.from_stacktrace(stacktrace) + } + end + + def build(:context, opts) do + config = get_config(opts) + + Map.merge( + %{environment: config.env(), hostname: config.hostname()}, + opts |> Keyword.get(:context, %{}) |> Enum.into(%{}) + ) + end + + def build(:environment, opts) do + environment = + Keyword.get_lazy(opts, :environment, fn -> + Keyword.get(opts, :env) + end) + + case environment do + nil -> nil + env -> env |> Enum.into(%{}) |> filter_environment(opts) + end + end + + def build(:params, opts) do + case Keyword.get(opts, :params) do + nil -> nil + params -> params |> Enum.into(%{}) |> filter_parameters(opts) + end + end + + def build(:session, opts) do + if Keyword.has_key?(opts, :session), + do: opts |> Keyword.get(:session) |> Enum.into(%{}), + else: nil + end + + def filter_parameters(params, opts) do + filter_parameters = get_config(opts).get(:filter_parameters, []) + + Utils.filter(params, filter_parameters) + end + + def filter_environment(nil) do + nil + end + + def filter_environment(environment, opts) do + filter_headers = get_config(opts).get(:filter_headers, []) + + cond do + Map.has_key?(environment, "headers") -> + Map.update!(environment, "headers", &Utils.filter(&1, filter_headers)) + + Map.has_key?(environment, :headers) -> + Map.update!(environment, :headers, &Utils.filter(&1, filter_headers)) + + true -> + environment + end + end + + defp get_config(opts), + do: Keyword.get(opts, :config, Airbrake.Config) +end diff --git a/test/airbrake/payload/builder_test.exs b/test/airbrake/payload/builder_test.exs new file mode 100644 index 0000000..56077c9 --- /dev/null +++ b/test/airbrake/payload/builder_test.exs @@ -0,0 +1,229 @@ +defmodule Airbrake.Payload.BuilderTest do + use ExUnit.Case, async: true + use ExUnitProperties + + import Mox + + alias Airbrake.Payload.Builder + + setup :verify_on_exit! + + describe "build/1 :context" do + property "builds default/initial context using env and hostname from config" do + opts = [config: MockConfig] + + check all env <- env(), + hostname <- hostname() do + MockConfig + |> stub(:env, fn -> env end) + |> stub(:hostname, fn -> hostname end) + + assert Builder.build(:context, opts) == %{environment: env, hostname: hostname} + end + end + + property "can add more context" do + check all env <- env(), + hostname <- hostname(), + foo <- one_of([integer(), string(:alphanumeric)]), + bar <- one_of([integer(), string(:alphanumeric)]) do + MockConfig + |> stub(:env, fn -> env end) + |> stub(:hostname, fn -> hostname end) + + opts = [ + config: MockConfig, + context: %{foo: foo, bar: bar} + ] + + assert Builder.build(:context, opts) == %{ + environment: env, + hostname: hostname, + foo: foo, + bar: bar + } + end + end + + property "can add more context with keyword list" do + check all env <- env(), + hostname <- hostname(), + foo <- one_of([integer(), string(:alphanumeric)]), + bar <- one_of([integer(), string(:alphanumeric)]) do + MockConfig + |> stub(:env, fn -> env end) + |> stub(:hostname, fn -> hostname end) + + opts = [ + config: MockConfig, + context: [foo: foo, bar: bar] + ] + + assert Builder.build(:context, opts) == %{ + environment: env, + hostname: hostname, + foo: foo, + bar: bar + } + end + end + + property "context in opts overwrites defaults" do + check all env <- env(), + hostname <- hostname(), + opts_env <- env(), + opts_hostname <- hostname(), + foo <- integer() do + MockConfig + |> stub(:env, fn -> env end) + |> stub(:hostname, fn -> hostname end) + + opts = [ + config: MockConfig, + context: %{foo: foo, environment: opts_env, hostname: opts_hostname} + ] + + assert Builder.build(:context, opts) == %{ + environment: opts_env, + hostname: opts_hostname, + foo: foo + } + end + end + end + + describe "build/1 :environment" do + test "returns nil if unspecified" do + opts = [] + + assert is_nil(Builder.build(:environment, opts)) + end + + property "returns value from opts" do + check all environment <- map_of(atom(:alphanumeric), string(:alphanumeric)) do + opts = [ + environment: environment + ] + + assert Builder.build(:environment, opts) == environment + end + end + + property "can specify with deprecated :env key" do + check all environment <- map_of(atom(:alphanumeric), string(:alphanumeric)) do + opts = [ + env: environment + ] + + assert Builder.build(:environment, opts) == environment + end + end + + property "returns keyword list from opts as map" do + check all environment <- keyword_of(string(:alphanumeric)) do + opts = [ + environment: environment + ] + + assert Builder.build(:environment, opts) == Map.new(environment) + end + end + + property "filters headers" do + check all headers1 <- map_of(string_key(), string(:alphanumeric)), + headers2 <- map_of(string_key(), string(:alphanumeric)), + headers = Map.merge(headers1, headers2) do + # filter keys in headers1 + stub(MockConfig, :get, fn :filter_headers, _ -> Map.keys(headers1) end) + + opts = [ + config: MockConfig, + environment: %{headers: headers} + ] + + assert %{headers: filtered_headers} = Builder.build(:environment, opts) + + assert filtered_headers + |> Map.take(Map.keys(headers1)) + |> Map.values() + |> Enum.all?(&(&1 == "[FILTERED]")) + + assert filtered_headers |> Map.take(Map.keys(headers2)) |> Enum.all?(fn {k, v} -> v == Map.get(headers, k) end) + end + end + end + + describe "build/1 :params" do + test "returns nil if unspecified" do + opts = [] + + assert is_nil(Builder.build(:params, opts)) + end + + property "returns value from opts" do + check all params <- map_of(string_key(), string(:alphanumeric)) do + opts = [ + params: params + ] + + assert Builder.build(:params, opts) == params + end + end + + property "filters params" do + # NOTE: this does NOT test nested filtering. + # That's tested with the Util function. + + check all params1 <- map_of(string_key(), string(:alphanumeric)), + params2 <- map_of(string_key(), string(:alphanumeric)), + params = Map.merge(params1, params2) do + # filter keys in params1 + stub(MockConfig, :get, fn :filter_parameters, _ -> Map.keys(params1) end) + + opts = [ + config: MockConfig, + params: params + ] + + assert %{} = filtered_params = Builder.build(:params, opts) + + assert filtered_params + |> Map.take(Map.keys(params1)) + |> Map.values() + |> Enum.all?(&(&1 == "[FILTERED]")) + + assert filtered_params |> Map.take(Map.keys(params2)) |> Enum.all?(fn {k, v} -> v == Map.get(params, k) end) + end + end + end + + describe "build/1 :session" do + test "nil by default" do + opts = [] + + assert is_nil(Builder.build(:session, opts)) + end + + property "returns value from opts" do + check all session <- map_of(string_key(), string(:alphanumeric)) do + opts = [ + session: session + ] + + assert Builder.build(:session, opts) == session + end + end + end + + defp string_key do + string(:alphanumeric, min_length: 3) + end + + defp env do + member_of(["dev", "uat", "staging", "prod"]) + end + + defp hostname do + string(:alphanumeric, min_length: 3) + end +end diff --git a/test/airbrake/payload_test.exs b/test/airbrake/payload_test.exs index 6fd94f4..9ddc56c 100644 --- a/test/airbrake/payload_test.exs +++ b/test/airbrake/payload_test.exs @@ -146,18 +146,19 @@ defmodule Airbrake.PayloadTest do test "sets params when given" do params = %{foo: 55, bar: "qux"} - assert %Payload{params: ^params} = Payload.new(@exception, @stacktrace, params: params) + + assert %Payload{ + params: %{"foo" => 55, "bar" => "qux"} + } = Payload.new(@exception, @stacktrace, params: params) end test "filters sensitive params" do - Application.put_env(:airbrake_client, :filter_parameters, ["password"]) - + # "password" is filtered out in `config/test.exs` + # Filtering is tested in more depth for `Airbrake.Payload.Builder`. params = %{"password" => "top_secret", "x" => "y"} assert %Payload{params: %{"password" => "[FILTERED]", "x" => "y"}} = Payload.new(@exception, @stacktrace, params: params) - - Application.delete_env(:airbrake_client, :filter_parameters) end test "sets session when given" do From 154c6c12c028b2e43f8c318d18d5d64003c8eee4 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Thu, 28 Mar 2024 15:55:29 -0500 Subject: [PATCH 08/10] optionally add Logger metadata to session --- lib/airbrake/payload/builder.ex | 16 ++++-- lib/airbrake/worker.ex | 14 ++++- test/airbrake/payload/builder_test.exs | 73 +++++++++++++++++++++++--- 3 files changed, 93 insertions(+), 10 deletions(-) diff --git a/lib/airbrake/payload/builder.ex b/lib/airbrake/payload/builder.ex index 6e6053a..ffe16d0 100644 --- a/lib/airbrake/payload/builder.ex +++ b/lib/airbrake/payload/builder.ex @@ -41,9 +41,19 @@ defmodule Airbrake.Payload.Builder do end def build(:session, opts) do - if Keyword.has_key?(opts, :session), - do: opts |> Keyword.get(:session) |> Enum.into(%{}), - else: nil + config = get_config(opts) + + logger_metadata = + if config.get(:session) == :include_logger_metadata, + do: Keyword.get(opts, :logger_metadata, []), + else: [] + + opts_session = opts |> Keyword.get(:session, %{}) |> Enum.into(%{}) + full_session = logger_metadata |> Enum.into(%{}) |> Map.merge(opts_session) + + if full_session == %{}, + do: nil, + else: full_session end def filter_parameters(params, opts) do diff --git a/lib/airbrake/worker.ex b/lib/airbrake/worker.ex index 3c6b984..56d094b 100644 --- a/lib/airbrake/worker.ex +++ b/lib/airbrake/worker.ex @@ -28,7 +28,13 @@ defmodule Airbrake.Worker do def report([type: _, message: _] = exception, options) when is_list(options) do stacktrace = options[:stacktrace] || get_stacktrace() - GenServer.cast(@name, {:report, exception, stacktrace, Keyword.delete(options, :stacktrace)}) + + options = + options + |> Keyword.delete(:stacktrace) + |> maybe_add_logger_metadata() + + GenServer.cast(@name, {:report, exception, stacktrace, options}) end def report(_, _) do @@ -136,6 +142,12 @@ defmodule Airbrake.Worker do defp ignore?(fun, type, message) when is_function(fun), do: fun.(type, message) defp ignore?(types, type, _message), do: MapSet.member?(types, type) + defp maybe_add_logger_metadata(opts) do + if Config.get(:session) == :include_logger_metadata, + do: Keyword.put(opts, :logger_metadata, Logger.metadata()), + else: opts + end + defp process_name(pid, pid), do: "Process [#{inspect(pid)}]" defp process_name(pname, pid), do: "#{inspect(pname)} [#{inspect(pid)}]" diff --git a/test/airbrake/payload/builder_test.exs b/test/airbrake/payload/builder_test.exs index 56077c9..8fa094e 100644 --- a/test/airbrake/payload/builder_test.exs +++ b/test/airbrake/payload/builder_test.exs @@ -197,22 +197,83 @@ defmodule Airbrake.Payload.BuilderTest do end end - describe "build/1 :session" do - test "nil by default" do - opts = [] + describe "build/1 :session and Logger metadata" do + test "nil if logger metadata is empty and :session not set" do + session_includes_metadata() + + opts = [ + config: MockConfig, + logger_metadata: [] + ] - assert is_nil(Builder.build(:session, opts)) + assert Builder.build(:session, opts) == nil end - property "returns value from opts" do - check all session <- map_of(string_key(), string(:alphanumeric)) do + test "nil if logger metadata is nil and :session not set" do + session_includes_metadata() + + opts = [ + config: MockConfig + ] + + assert Builder.build(:session, opts) == nil + end + + property "returns :session from opts over logger metadata" do + session_includes_metadata() + + check all logger_metadata <- keyword_of(string(:alphanumeric)), + session <- map_of(string_key(), string(:alphanumeric)), + logger_metadata != [] or session != %{} do opts = [ + config: MockConfig, + logger_metadata: logger_metadata, + session: session + ] + + assert Builder.build(:session, opts) == + Map.merge(Map.new(logger_metadata), session) + end + end + + defp session_includes_metadata do + stub(MockConfig, :get, fn :session -> :include_logger_metadata end) + end + end + + describe "build/1 :session WITHOUT Logger metadata" do + property "nil if :session not set regardless of Logger metadata" do + session_does_not_includes_metadata() + + check all logger_metadata <- keyword_of(string(:alphanumeric)) do + opts = [ + config: MockConfig, + logger_metadata: logger_metadata + ] + + assert Builder.build(:session, opts) == nil + end + end + + property "returns :session from opts only" do + session_does_not_includes_metadata() + + check all logger_metadata <- keyword_of(string(:alphanumeric)), + session <- map_of(string_key(), string(:alphanumeric)), + session != %{} do + opts = [ + config: MockConfig, + logger_metadata: logger_metadata, session: session ] assert Builder.build(:session, opts) == session end end + + defp session_does_not_includes_metadata do + stub(MockConfig, :get, fn :session -> nil end) + end end defp string_key do From fdad7795147ccfd6297c6c47f68256b261307494 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Thu, 28 Mar 2024 16:06:01 -0500 Subject: [PATCH 09/10] update README for new config option --- README.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/README.md b/README.md index cbebf17..f3eb7b3 100644 --- a/README.md +++ b/README.md @@ -48,12 +48,16 @@ config :airbrake_client, environment: Mix.env(), filter_parameters: ["password"], filter_headers: ["authorization"], + session: :include_logger_metadata, host: "https://api.airbrake.io" # or your Errbit host config :logger, backends: [{Airbrake.LoggerBackend, :error}, :console] ``` +Split this config across your `config/*.exs` files (especially the runtime +setting in `config/runtime.exs`). + Required configuration arguments: * `:api_key` - (binary) the token needed to access the [Airbrake @@ -74,6 +78,24 @@ Optional configuration arguments: to ignore some or all exceptions. See examples below. * `:options` - (keyword list or function returning keyword list) values that are included in all reports to Airbrake.io. See examples below. + * `:session` - can be set to `:include_logger_metadata` to include Logger + metadata in the `session` field of the report; omit this option if you do + not want Logger metadata. See below for more information. + +### Logger metadata in the `session` + +If you set the `:session` config to `:include_logger_metadata`, the Logger +metadata from the process that invokes `Airbrake.report/2` will be the initial +session data for the `session` field. The values passed as `:session` in the +`options` parameter of `Airbrake.report/2` are _added_ to the session value, +overwriting any Logger metadata values. + +If you do not set the `:session` config, only the `:session` value passed as the +options to `Airbrake.report/2` will be used for the `session` field in the +report. + +If the `session` turns out to be empty (for whatever reason), it is instead set +to `nil` (and should not show up in the report). ### Ignoring some exceptions From b64271ffa17460e1332abfe9a2334f5f4de05ee8 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Frens" Date: Thu, 28 Mar 2024 16:06:29 -0500 Subject: [PATCH 10/10] update CHANGELOG and bump to version 2.1.0 --- CHANGELOG.md | 10 +++++++++- .../jason_only_app/test/jason_only_app_test.exs | 4 ++-- .../poison_only_app/test/poison_only_app_test.exs | 4 ++-- mix.exs | 2 +- test/airbrake/payload_test.exs | 12 ++++++------ 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d230ba5..362bdf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog for v0.x +## v2.1.0 (??????????) + +### Enhancements + + * New config option: if `:session` is set to `:include_logger_metadata`, the + Logger metadata from `Logger.metadata/0` is added to the `session` field of + the report. (If the option is not set, the metadata is not included.) + ## v2.0.0 (2024-03-11) ### Enhancements @@ -10,7 +18,7 @@ ### Breaking Change * Drop support for Elixir <1.12 - + ## v1.0.0 (2023-10-12) ### Enhancements diff --git a/integration_test_apps/jason_only_app/test/jason_only_app_test.exs b/integration_test_apps/jason_only_app/test/jason_only_app_test.exs index 42e73d2..9067992 100644 --- a/integration_test_apps/jason_only_app/test/jason_only_app_test.exs +++ b/integration_test_apps/jason_only_app/test/jason_only_app_test.exs @@ -39,7 +39,7 @@ defmodule JasonOnlyAppTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => nil, "session" => nil @@ -88,7 +88,7 @@ defmodule JasonOnlyAppTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => %{"foo" => 55}, "session" => %{"foo" => 555} diff --git a/integration_test_apps/poison_only_app/test/poison_only_app_test.exs b/integration_test_apps/poison_only_app/test/poison_only_app_test.exs index 2865e89..c7a2ff2 100644 --- a/integration_test_apps/poison_only_app/test/poison_only_app_test.exs +++ b/integration_test_apps/poison_only_app/test/poison_only_app_test.exs @@ -40,7 +40,7 @@ defmodule PoisonOnlyAppTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => nil, "session" => nil @@ -89,7 +89,7 @@ defmodule PoisonOnlyAppTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => %{"foo" => 55}, "session" => %{"foo" => 555} diff --git a/mix.exs b/mix.exs index 31bd295..73330f6 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Airbrake.Mixfile do def project do [ app: :airbrake_client, - version: "2.0.0", + version: "2.1.0", elixir: "~> 1.12", elixirc_paths: elixirc_paths(Mix.env()), package: package(), diff --git a/test/airbrake/payload_test.exs b/test/airbrake/payload_test.exs index 9ddc56c..8956e43 100644 --- a/test/airbrake/payload_test.exs +++ b/test/airbrake/payload_test.exs @@ -48,7 +48,7 @@ defmodule Airbrake.PayloadTest do notifier: %{ name: "Airbrake Client", url: "https://github.com/CityBaseInc/airbrake_client", - version: "2.0.0" + version: "2.1.0" } } = Payload.new(exception, stacktrace) end @@ -105,7 +105,7 @@ defmodule Airbrake.PayloadTest do notifier: %{ name: "Airbrake Client", url: "https://github.com/CityBaseInc/airbrake_client", - version: "2.0.0" + version: "2.1.0" } } = Payload.new(@exception, @stacktrace) end @@ -198,7 +198,7 @@ defmodule Airbrake.PayloadTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => nil, "session" => nil @@ -242,7 +242,7 @@ defmodule Airbrake.PayloadTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => %{"foo" => 55}, "session" => %{"foo" => 555} @@ -281,7 +281,7 @@ defmodule Airbrake.PayloadTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => nil, "session" => nil @@ -325,7 +325,7 @@ defmodule Airbrake.PayloadTest do "notifier" => %{ "name" => "Airbrake Client", "url" => "https://github.com/CityBaseInc/airbrake_client", - "version" => "2.0.0" + "version" => "2.1.0" }, "params" => %{"foo" => 55}, "session" => %{"foo" => 555}