Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] ErrorStorage agent calls #98

Merged
merged 7 commits into from
Oct 25, 2024
Merged

[Fix] ErrorStorage agent calls #98

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f6bc748
improve agent.get call
joaquinco Sep 5, 2024
f06f043
refactor reset error count
joaquinco Sep 5, 2024
604c4a4
test reset return valule
joaquinco Sep 6, 2024
e495273
fix error storage specs
joaquinco Sep 18, 2024
cc11118
fixes from review
joaquinco Sep 23, 2024
e13cb17
rename ErrorStorage.reset/2 to ErrorStorage.reset_stats/2
joaquinco Oct 2, 2024
69676e9
rename ErrorStorage.get_error_stats/1 to ErrorStorage.get_stats/1
joaquinco Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 53 additions & 44 deletions lib/boom_notifier/error_storage.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,14 @@ defmodule BoomNotifier.ErrorStorage do
%{key: error_hash_key} = error_info
timestamp = error_info.timestamp || DateTime.utc_now()

default_error_storage_info = %__MODULE__{
accumulated_occurrences: 1,
first_occurrence: timestamp,
last_occurrence: timestamp,
__max_storage_capacity__: 1
}

Agent.update(
:boom_notifier,
&Map.update(
&1,
error_hash_key,
default_error_storage_info,
timestamp
|> default_error_storage()
|> Map.put(:accumulated_occurrences, 1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can move this to the top (in line 39) so it's more clear this is the initial value for the Agent.

Something like

initial_error_storage = 
        |> timestamp
        |> default_error_storage()
        |> Map.put(:accumulated_occurrences, 1)

Or instead of calling that function default_error_storage rename it to build_error_storage and have something like this

default_error_storage = 
        |> timestamp
        |> build_error_storage()
        |> Map.put(:accumulated_occurrences, 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea!

fn error_storage_item ->
error_storage_item
|> Map.update!(:accumulated_occurrences, fn current -> current + 1 end)
Expand All @@ -65,12 +60,11 @@ defmodule BoomNotifier.ErrorStorage do
@doc """
Given an error info, it returns the aggregated info stored in the agent.
"""
@spec get_error_stats(ErrorInfo.t()) :: %__MODULE__{}
@spec get_error_stats(ErrorInfo.t()) :: __MODULE__.t() | nil
def get_error_stats(error_info) do
%{key: error_hash_key} = error_info

Agent.get(:boom_notifier, fn state -> state end)
|> Map.get(error_hash_key)
Agent.get(:boom_notifier, &Map.get(&1, error_hash_key))
end

@doc """
Expand All @@ -81,53 +75,59 @@ defmodule BoomNotifier.ErrorStorage do
"""
@spec send_notification?(ErrorInfo.t()) :: boolean()
def send_notification?(error_info) do
%{key: error_hash_key} = error_info

error_storage_item =
Agent.get(:boom_notifier, fn state -> state end)
|> Map.get(error_hash_key)

do_send_notification?(error_storage_item)
error_info
|> get_error_stats()
|> do_send_notification?()
end

@doc """
Reset the accumulated_occurrences for the given error info to zero. It also
increments the max storage capacity based on the notification strategy.

Returns error storage entry before reset
"""
@spec reset_accumulated_errors(error_strategy, ErrorInfo.t()) :: :ok
def reset_accumulated_errors(:exponential, error_info) do
%{key: error_hash_key} = error_info
@spec reset(ErrorInfo.t()) :: __MODULE__.t()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loved the rename of this function 😍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about something like ErrorStorage.reset_stats/2 or ErrorStorage.reset_and_get_stats/2

I'm thinking about the notifications_sender reading:

occurrences = ErrorStorage.reset_stats(error_info, notification_trigger)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to be consistent with other ErrorStorage's functions

@spec reset(ErrorInfo.t(), count_strategy :: :exponential | :always | nil) :: __MODULE__.t()
def reset(error_info), do: reset(error_info, nil)

Agent.update(
:boom_notifier,
&Map.update!(&1, error_hash_key, fn error_storage_item ->
clear_values(error_storage_item)
|> Map.update!(:__max_storage_capacity__, fn current -> current * 2 end)
end)
)
def reset(error_info, :exponential) do
reset_state(error_info, fn value -> value * 2 end)
end

def reset_accumulated_errors([exponential: [limit: limit]], error_info) do
%{key: error_hash_key} = error_info
def reset(error_info, exponential: [limit: limit]) do
reset_state(error_info, fn value -> min(value * 2, limit) end)
end

Agent.update(
:boom_notifier,
&Map.update!(&1, error_hash_key, fn error_storage_item ->
clear_values(error_storage_item)
|> Map.update!(:__max_storage_capacity__, fn current -> min(current * 2, limit) end)
end)
)
def reset(error_info, strategy) when strategy in [:always, nil] do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can remove this guard so it always fallbacks here if the strategy doesn't need to update the max_storage value

reset_state(error_info, nil)
end

def reset_accumulated_errors(:always, error_info) do
defp reset_state(error_info, nil) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: defp reset_state(error_info, _limit_updater_function = nil) do

reset_state(error_info, fn _ -> 1 end)
end

defp reset_state(error_info, limit_updater_function) do
%{key: error_hash_key} = error_info

Agent.update(
Agent.get_and_update(
:boom_notifier,
&Map.update!(&1, error_hash_key, fn error_storage_item ->
clear_values(error_storage_item)
|> Map.replace!(:__max_storage_capacity__, 1)
end)
fn state ->
error_storage_item = Map.get(state, error_hash_key)

state =
Map.update(
state,
error_hash_key,
default_error_storage(),
fn error_storage_item ->
error_storage_item
|> clear_values()
|> Map.update!(:__max_storage_capacity__, limit_updater_function)
end
)

{error_storage_item, state}
end
)
end

Expand All @@ -138,7 +138,7 @@ defmodule BoomNotifier.ErrorStorage do
|> Map.replace!(:last_occurrence, nil)
end

@spec do_send_notification?(ErrorInfo.t() | nil) :: boolean()
@spec do_send_notification?(nil | __MODULE__.t()) :: boolean()
defp do_send_notification?(nil), do: false

defp do_send_notification?(error_storage_item) do
Expand All @@ -147,4 +147,13 @@ defmodule BoomNotifier.ErrorStorage do

accumulated_occurrences >= max_storage_capacity
end

defp default_error_storage(timestamp \\ nil) do
%__MODULE__{
accumulated_occurrences: 0,
first_occurrence: timestamp,
last_occurrence: timestamp,
__max_storage_capacity__: 1
}
end
end
7 changes: 3 additions & 4 deletions lib/boom_notifier/notification_sender.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,12 @@ defmodule BoomNotifier.NotificationSender do

def notify_all(settings, error_info) do
notification_trigger = Keyword.get(settings, :notification_trigger, :always)
occurrences = Map.put(error_info, :occurrences, ErrorStorage.get_error_stats(error_info))

ErrorStorage.reset_accumulated_errors(notification_trigger, error_info)
occurrences = ErrorStorage.reset(error_info, notification_trigger)
error_info = Map.put(error_info, :occurrences, occurrences)

BoomNotifier.walkthrough_notifiers(
settings,
fn notifier, options -> notify(notifier, occurrences, options) end
fn notifier, options -> notify(notifier, error_info, options) end
)
end

Expand Down
43 changes: 29 additions & 14 deletions test/unit/error_storage_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ defmodule ErrorStorageTest do
test "returns false when count is smaller than the error length" do
# increase the max capacity to 2
ErrorStorage.store_error(@error_info)
ErrorStorage.reset_accumulated_errors(:exponential, @error_info)
ErrorStorage.reset(@error_info, :exponential)
ErrorStorage.store_error(@error_info)

refute ErrorStorage.send_notification?(@error_info)
Expand All @@ -115,7 +115,7 @@ defmodule ErrorStorageTest do
# creates the error key
ErrorStorage.store_error(@error_info)
# increase the max capacity to 2
ErrorStorage.reset_accumulated_errors(:exponential, @error_info)
ErrorStorage.reset(@error_info, :exponential)
ErrorStorage.store_error(@error_info)
ErrorStorage.store_error(@error_info)

Expand All @@ -128,7 +128,7 @@ defmodule ErrorStorageTest do
# creates the error key
ErrorStorage.store_error(another_error_info)
# increase the max capacity to 2
ErrorStorage.reset_accumulated_errors(:exponential, another_error_info)
ErrorStorage.reset(another_error_info, :exponential)
ErrorStorage.store_error(another_error_info)
ErrorStorage.store_error(another_error_info)
ErrorStorage.store_error(another_error_info)
Expand All @@ -142,11 +142,26 @@ defmodule ErrorStorageTest do
end
end

describe "reset_accumulated_errors/2" do
describe "reset/2" do
test "it returns the error storage info" do
ErrorStorage.store_error(@error_info)
error_storage_item = ErrorStorage.reset(@error_info)

assert %{accumulated_occurrences: 1} = error_storage_item
end

test "it does not fail if error_info is not present" do
error_storage_before = ErrorStorage.reset(@error_info)
error_storage_after = ErrorStorage.get_error_stats(@error_info)

assert is_nil(error_storage_before)
assert %ErrorStorage{accumulated_occurrences: 0} = error_storage_after
end

test "increases the counter when notification trigger is :exponential" do
ErrorStorage.store_error(@error_info)

ErrorStorage.reset_accumulated_errors(:exponential, @error_info)
ErrorStorage.reset(@error_info, :exponential)
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -156,7 +171,7 @@ defmodule ErrorStorageTest do
last_occurrence: nil
}

ErrorStorage.reset_accumulated_errors(:exponential, @error_info)
ErrorStorage.reset(@error_info, :exponential)
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -166,7 +181,7 @@ defmodule ErrorStorageTest do
last_occurrence: nil
}

ErrorStorage.reset_accumulated_errors(:exponential, @error_info)
ErrorStorage.reset(@error_info, :exponential)
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -180,7 +195,7 @@ defmodule ErrorStorageTest do
test "increases the counter when notification trigger is :exponential and :limit is set" do
ErrorStorage.store_error(@error_info)

ErrorStorage.reset_accumulated_errors([exponential: [limit: 5]], @error_info)
ErrorStorage.reset(@error_info, exponential: [limit: 5])
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -190,7 +205,7 @@ defmodule ErrorStorageTest do
last_occurrence: nil
}

ErrorStorage.reset_accumulated_errors([exponential: [limit: 5]], @error_info)
ErrorStorage.reset(@error_info, exponential: [limit: 5])
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -200,7 +215,7 @@ defmodule ErrorStorageTest do
last_occurrence: nil
}

ErrorStorage.reset_accumulated_errors([exponential: [limit: 5]], @error_info)
ErrorStorage.reset(@error_info, exponential: [limit: 5])
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -214,7 +229,7 @@ defmodule ErrorStorageTest do
test "does not increase the counter when notification_trigger is :always" do
ErrorStorage.store_error(@error_info)

ErrorStorage.reset_accumulated_errors(:always, @error_info)
ErrorStorage.reset(@error_info, :always)
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -224,7 +239,7 @@ defmodule ErrorStorageTest do
last_occurrence: nil
}

ErrorStorage.reset_accumulated_errors(:always, @error_info)
ErrorStorage.reset(@error_info, :always)
[error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values()

assert error_stat == %ErrorStorage{
Expand All @@ -234,7 +249,7 @@ defmodule ErrorStorageTest do
last_occurrence: nil
}

ErrorStorage.reset_accumulated_errors(:always, @error_info)
ErrorStorage.reset(@error_info, :always)
end

test "updates the proper error max capacity" do
Expand All @@ -254,7 +269,7 @@ defmodule ErrorStorageTest do
ErrorStorage.store_error(@error_info)
ErrorStorage.store_error(another_error_info)

ErrorStorage.reset_accumulated_errors(:exponential, @error_info)
ErrorStorage.reset(@error_info, :exponential)

%{@error_hash => error_stat_1, ^another_error_hash => error_stat_2} =
Agent.get(:boom_notifier, & &1)
Expand Down
6 changes: 4 additions & 2 deletions test/unit/notification_sender_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ defmodule BoomNotifier.NotificationSenderTest do

test "does not send a second notification", %{error_info: error_info} do
ErrorStorage.store_error(error_info)
ErrorStorage.reset_accumulated_errors(:exponential, error_info)
ErrorStorage.reset(error_info, :exponential)

trigger_notify_resp = NotificationSender.trigger_notify(@settings_groupping, error_info)

Expand Down Expand Up @@ -105,7 +105,9 @@ defmodule BoomNotifier.NotificationSenderTest do
describe "repeated async call with exponential notification trigger" do
setup(%{error_info: error_info}) do
ErrorStorage.store_error(error_info)
ErrorStorage.reset_accumulated_errors(:exponential, error_info)
ErrorStorage.reset(error_info, :exponential)

:ok
end

test "sends a second notification after a timeout", %{error_info: error_info} do
Expand Down
Loading