From cf6cfe955dc84f71e1f67e687ea64b675f289328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93scar=20de=20Arriba?= Date: Tue, 27 Aug 2024 21:36:36 +0200 Subject: [PATCH 1/4] Validate context and allow storing the occurrence anyways This change updates how an occurrence is stored in order to validate if the context can be serialized to be stored on the database. If the context cannot be serialized, we emit a warning log message and store an error as the context. --- lib/error_tracker.ex | 6 ++- lib/error_tracker/schemas/occurrence.ex | 52 +++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/lib/error_tracker.ex b/lib/error_tracker.ex index 292569f..130f8c0 100644 --- a/lib/error_tracker.ex +++ b/lib/error_tracker.ex @@ -70,6 +70,7 @@ defmodule ErrorTracker do import Ecto.Query alias ErrorTracker.Error + alias ErrorTracker.Occurrence alias ErrorTracker.Repo alias ErrorTracker.Telemetry @@ -212,11 +213,12 @@ defmodule ErrorTracker do occurrence = error - |> Ecto.build_assoc(:occurrences, + |> Ecto.build_assoc(:occurrences) + |> Occurrence.changeset(%{ stacktrace: stacktrace, context: context, reason: reason - ) + }) |> Repo.insert!() {error, occurrence} diff --git a/lib/error_tracker/schemas/occurrence.ex b/lib/error_tracker/schemas/occurrence.ex index f36c583..837b724 100644 --- a/lib/error_tracker/schemas/occurrence.ex +++ b/lib/error_tracker/schemas/occurrence.ex @@ -8,6 +8,9 @@ defmodule ErrorTracker.Occurrence do use Ecto.Schema + require Logger + import Ecto.Changeset + schema "error_tracker_occurrences" do field :context, :map field :reason, :string @@ -17,4 +20,53 @@ defmodule ErrorTracker.Occurrence do timestamps(type: :utc_datetime_usec, updated_at: false) end + + def changeset(occurrence, attrs) do + occurrence + |> cast(attrs, [:context, :reason]) + |> maybe_put_stacktrace() + |> validate_context() + |> validate_required([:context, :reason, :stacktrace]) + |> foreign_key_constraint(:error) + end + + # This function validates if the context can be serialized to JSON before + # storing it to the DB. + # + # If it cannot be serialized a warning log message is emitted and an error + # is stored in the context. + # + defp validate_context(changeset) do + if changeset.valid? do + context = get_field(changeset, :context, %{}) + + json_encoder = + ErrorTracker.Repo.with_adapter(fn + :postgres -> Application.get_env(:postgrex, :json_library, Jason) + :sqlite -> Application.get_env(:ecto_sqlite3, :json_library, Jason) + end) + + case json_encoder.encode_to_iodata(context) do + {:ok, _} -> + put_change(changeset, :context, context) + + {:error, _} -> + Logger.warning( + "[ErrorTracker] Context has been ignored: it is not serializable to JSON." + ) + + put_change(changeset, :context, %{ + error: "Context not stored because it contains information not serializable to JSON." + }) + end + else + changeset + end + end + + defp maybe_put_stacktrace(changeset) do + if stacktrace = Map.get(changeset.params, "stacktrace"), + do: put_embed(changeset, :stacktrace, stacktrace), + else: changeset + end end From c15f00c09cb780e377110c62e5eb495cf6942fc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93scar=20de=20Arriba?= Date: Tue, 27 Aug 2024 21:58:13 +0200 Subject: [PATCH 2/4] Add tests. --- .../error_tracker/schemas/occurrence_test.exs | 39 +++++++++++++++++++ test/error_tracker_test.exs | 8 ++++ test/support/case.ex | 6 +-- 3 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 test/error_tracker/schemas/occurrence_test.exs diff --git a/test/error_tracker/schemas/occurrence_test.exs b/test/error_tracker/schemas/occurrence_test.exs new file mode 100644 index 0000000..43535d0 --- /dev/null +++ b/test/error_tracker/schemas/occurrence_test.exs @@ -0,0 +1,39 @@ +defmodule ErrorTracker.OccurrenceTest do + use ErrorTracker.Test.Case + + import Ecto.Changeset + + alias ErrorTracker.Stacktrace + alias ErrorTracker.Occurrence + + describe inspect(&Occurrence.changeset/2) do + test "works as expected with valid data" do + attrs = %{context: %{foo: :bar}, reason: "Test reason", stacktrace: %Stacktrace{}} + changeset = Occurrence.changeset(%Occurrence{}, attrs) + + assert changeset.valid? + end + + test "validates required fields" do + changeset = Occurrence.changeset(%Occurrence{}, %{}) + + refute changeset.valid? + assert {_, [validation: :required]} = changeset.errors[:reason] + assert {_, [validation: :required]} = changeset.errors[:stacktrace] + end + + @tag capture_log: true + test "if context is not serializable, an error messgae is stored" do + attrs = %{ + context: %{foo: %ErrorTracker.Error{}}, + reason: "Test reason", + stacktrace: %Stacktrace{} + } + + changeset = Occurrence.changeset(%Occurrence{}, attrs) + + assert %{error: err} = get_field(changeset, :context) + assert err =~ "not serializable to JSON" + end + end +end diff --git a/test/error_tracker_test.exs b/test/error_tracker_test.exs index 3afa438..090880f 100644 --- a/test/error_tracker_test.exs +++ b/test/error_tracker_test.exs @@ -64,6 +64,14 @@ defmodule ErrorTrackerTest do assert error.reason == "This is a test" assert error.source_line =~ @relative_file_path end + + @tag capture_log: true + test "reports errors with invalid context" do + # It's invalid because cannot be serialized to JSON + invalid_context = %{foo: %ErrorTracker.Error{}} + + assert %Occurrence{} = report_error(fn -> raise "test" end, invalid_context) + end end describe inspect(&ErrorTracker.resolve/1) do diff --git a/test/support/case.ex b/test/support/case.ex index 2c61a4f..2e35fa4 100644 --- a/test/support/case.ex +++ b/test/support/case.ex @@ -16,16 +16,16 @@ defmodule ErrorTracker.Test.Case do @doc """ Reports the error produced by the given function. """ - def report_error(fun) do + def report_error(fun, context \\ %{}) do occurrence = try do fun.() rescue exception -> - ErrorTracker.report(exception, __STACKTRACE__) + ErrorTracker.report(exception, __STACKTRACE__, context) catch kind, reason -> - ErrorTracker.report({kind, reason}, __STACKTRACE__) + ErrorTracker.report({kind, reason}, __STACKTRACE__, context) end repo().preload(occurrence, :error) From 24483e64700c59e41d644c13c8c6137c7870ce68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93scar=20de=20Arriba?= Date: Tue, 27 Aug 2024 22:05:38 +0200 Subject: [PATCH 3/4] Avoid doc --- lib/error_tracker/schemas/occurrence.ex | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/error_tracker/schemas/occurrence.ex b/lib/error_tracker/schemas/occurrence.ex index 837b724..4729e2e 100644 --- a/lib/error_tracker/schemas/occurrence.ex +++ b/lib/error_tracker/schemas/occurrence.ex @@ -21,12 +21,13 @@ defmodule ErrorTracker.Occurrence do timestamps(type: :utc_datetime_usec, updated_at: false) end + @doc false def changeset(occurrence, attrs) do occurrence |> cast(attrs, [:context, :reason]) |> maybe_put_stacktrace() + |> validate_required([:reason, :stacktrace]) |> validate_context() - |> validate_required([:context, :reason, :stacktrace]) |> foreign_key_constraint(:error) end From 062c58845e37b70671b3d03fb8a7e6ac9443c906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93scar=20de=20Arriba?= Date: Tue, 27 Aug 2024 22:08:06 +0200 Subject: [PATCH 4/4] Happy credo --- test/error_tracker/schemas/occurrence_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/error_tracker/schemas/occurrence_test.exs b/test/error_tracker/schemas/occurrence_test.exs index 43535d0..3695d2a 100644 --- a/test/error_tracker/schemas/occurrence_test.exs +++ b/test/error_tracker/schemas/occurrence_test.exs @@ -3,8 +3,8 @@ defmodule ErrorTracker.OccurrenceTest do import Ecto.Changeset - alias ErrorTracker.Stacktrace alias ErrorTracker.Occurrence + alias ErrorTracker.Stacktrace describe inspect(&Occurrence.changeset/2) do test "works as expected with valid data" do