Add :ignore behavior for discovery events

[balanza]

A received discovery event is discarded even if valid and correctly mapped to a command. This can happen if it does not make sense for the state of the SapSystem aggregate at that moment.

Namely, this happens with the RegisterApplicationInstance command. When the relative database instance has not been registered yet, the command cannot be processed.

This scenario is not considered an error anymore; instead, the command is just ignored and the publishing client receives a 202 Accepted response.

---

Despite being applied to discovery events, the behavior is available to any other command that's been enriched (example)

[balanza]

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

[arbulu89]

Hey @balanza ,
What do you think of the next option?
Simply return {:error, :database_not_registered} in the enrichment code, as before. The same goes for the discovery. And in the FallbackController, instead of returning 404, simply return 202. I mean, move the logic to the FallbackController in this scenario. I think, internally, for the enrichment protocol and discovery, this can continue being an error, and you decide how to handle this error somewhere else.

Otherwise, I see this new aggregate_dispatch_results really confusing an error prone. For example, ignored messages coming with error ones, are kind of ignored, and only the error message is logged.

[arbulu89]

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

[balanza]

Should be an error or a warning?

[arbulu89]

Yes, maybe an warning is more appropriate

[arbulu89]

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

[balanza]

@arbulu89 I'm trying to avoid the if :database_not_registered then thing flooding into the dispatcher logic. The introduction of :ignore serves to allow each event processing pipeline to decide whether is an error or not, depending on the case.

I see the implementation I'm proposing is maybe too convoluted.

As an alternative, I'm keen to just return 202 Accepted on every discarded event, as there is nothing the Agent can do with the error. But I see it can be a big stretch for now.

[arbulu89]

I'm trying to avoid the if :database_not_registered then thing flooding into the dispatcher logic. The introduction of :ignore serves to allow each event processing pipeline to decide whether is an error or not, depending on the case.

What I propose doesn't even require to do almost any single change in the enrichment or discovery code.
You could replace the {:error, :database_not_registered} by {:error, :associated_database_not_found} in enrich_register_application_instance.ex, and in the fallback controller include this:

def call(conn, {:error, :associated_database_not_found}) do
    conn
      |> put_status(:accepted)
      |> json(%{})
end

It simply defines what type of return to use in the controller in case of we don't have the happy path

[nelsonkopliku]

I don't mind the ignore semantics. However it is true that returning a different error, perhaps a more specific one, allowing us to catch it in the outer layer and act accordingly (the fallback controller in this case returning a 202) is simpler and requires less changes.

[balanza]

@nelsonkopliku @arbulu89 Thanks for the review, I buy your point and I'll go for a simpler solution. I open a new PR.