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.

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

Add :ignore behavior for discovery events #3069

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 35 additions & 16 deletions lib/trento/discovery.ex
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,7 @@ defmodule Trento.Discovery do
:ok <- dispatch(commands) do
:ok
else
{:error, reason} = error ->
Logger.error("Failed to handle discovery event: #{inspect(reason)}")
store_discarded_discovery_event(event, inspect(reason))

error
result -> handle_not_dispatched(event, result)
end
end

Expand Down Expand Up @@ -164,24 +160,47 @@ defmodule Trento.Discovery do
defp do_handle(_),
do: {:error, :unknown_discovery_type}

@spec dispatch(command | [command]) :: :ok | {:error, any}
@spec dispatch(command | [command]) :: :ok | {:error, any} | {:ignore, any}
defp dispatch(commands) when is_list(commands) do
Enum.reduce(commands, :ok, fn command, acc ->
case {commanded().dispatch(command), acc} do
{:ok, :ok} ->
:ok

{{:error, error}, :ok} ->
{:error, [error]}

{{:error, error}, {:error, errors}} ->
{:error, [error | errors]}
end
result = commanded().dispatch(command)
aggregate_dispatch_results(acc, result)
end)
end

defp dispatch(command), do: commanded().dispatch(command)

defp aggregate_dispatch_results(:ok, :ok), do: :ok
defp aggregate_dispatch_results(:ok, {:ignore, reason}), do: {:ignore, [reason]}
defp aggregate_dispatch_results(:ok, {:error, reason}), do: {:error, [reason]}
defp aggregate_dispatch_results({:ignore, reasons}, :ok), do: {:ignore, reasons}

defp aggregate_dispatch_results({:ignore, reasons}, {:ignore, reason}),
do: {:ignore, [reason | reasons]}

defp aggregate_dispatch_results({:ignore, reasons}, {:error, reason}),
do: {:error, [reason | reasons]}

defp aggregate_dispatch_results({:error, reasons}, :ok), do: {:errors, reasons}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should return {:error, reasons} in any case


defp aggregate_dispatch_results({:error, reasons}, {:ignore, reason}),
do: {:error, [reason | reasons]}

defp aggregate_dispatch_results({:error, reasons}, {:error, reason}),
do: {:error, [reason | reasons]}

defp commanded,
do: Application.fetch_env!(:trento, Trento.Commanded)[:adapter]

defp handle_not_dispatched(event, {:ignore, reasons}) do
Logger.warning("Ignored discovery event: #{inspect(reasons)}")
store_discarded_discovery_event(event, inspect(reasons))
:ok
end

defp handle_not_dispatched(event, {:error, reasons} = error) do
Logger.error("Failed to handle discovery event: #{inspect(reasons)}")
store_discarded_discovery_event(event, inspect(reasons))
error
end
end
5 changes: 5 additions & 0 deletions lib/trento/infrastructure/commanded/middleware/enrich.ex
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ defmodule Trento.Infrastructure.Commanded.Middleware.Enrich do
pipeline
|> respond({:error, reason})
|> halt

{:ignore, reason} ->
Copy link
Member Author

Choose a reason for hiding this comment

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

question(naming): I'm open to alternative names for this: halt, skip, noop... ‏

pipeline
|> respond({:ignore, reason})
|> halt
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ defimpl Trento.Infrastructure.Commanded.Middleware.Enrichable,
}}

nil ->
{:error, :database_not_registered}
{:ignore, :database_not_registered}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add here, before returning, a concise message of what's happening, to give real undoubetable hint about what's their issue.
Something like:

Logger.error("database instance associated to application instance #{sid} not registered in Trento. Please make sure that Trento agent is running on database instances associated to this SAP system")

You can get the sid in the RegisterApplicationInstance command I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be an error or a warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, maybe an warning is more appropriate

end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ defmodule Trento.Infrastructure.Commanded.Middleware.EnrichRegisterApplicationIn
health: :passing
)

assert {:error, :database_not_registered} = Enrichable.enrich(command, %{})
assert {:ignore, :database_not_registered} = Enrichable.enrich(command, %{})
end

test "should return an error if the database was not found" do
Expand All @@ -85,6 +85,6 @@ defmodule Trento.Infrastructure.Commanded.Middleware.EnrichRegisterApplicationIn
health: :passing
)

assert {:error, :database_not_registered} = Enrichable.enrich(command, %{})
assert {:ignore, :database_not_registered} = Enrichable.enrich(command, %{})
end
end
28 changes: 25 additions & 3 deletions test/trento_web/controllers/v1/discovery_controller_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ defmodule TrentoWeb.V1.DiscoveryControllerTest do
|> json_response(202)
end

test "collect action discards application instance registrations when the associated database does not exists",
test "collect action discards and store events when the command fails",
%{conn: conn} do
body =
load_discovery_event_fixture("sap_system_discovery_application")

expect(Trento.Commanded.Mock, :dispatch, fn _ ->
{:error, :any_error}
{:error, :any_reason}
end)

%{status: status} =
Expand All @@ -59,7 +59,29 @@ defmodule TrentoWeb.V1.DiscoveryControllerTest do

[discarded_event] = Discovery.get_discarded_discovery_events(1)

assert %DiscardedDiscoveryEvent{payload: ^body, reason: "[:any_error]"} =
assert %DiscardedDiscoveryEvent{payload: ^body, reason: "[:any_reason]"} =
discarded_event
end

test "collect action discards and store events when the command is not dispatched",
%{conn: conn} do
body =
load_discovery_event_fixture("sap_system_discovery_application")

expect(Trento.Commanded.Mock, :dispatch, fn _ ->
{:ignore, :any_reason}
end)

%{status: status} =
conn
|> put_req_header("content-type", "application/json")
|> post("/api/v1/collect", body)

assert status == 202

[discarded_event] = Discovery.get_discarded_discovery_events(1)

assert %DiscardedDiscoveryEvent{payload: ^body, reason: "[:any_reason]"} =
discarded_event
end
end
Expand Down
Loading