[Ingest Management] Remove/replace Boom errors in services

[jfsiii]

This is the second phase of work from #66688 which ensured consistent logging & return values

We can replace Boom with custom errors like

class ServiceXItemNotFoundError extends Error {}
...
throw new ServiceXItemNotFoundError(`${packageName} not found`);

The legacy SO client is also a source of Boom errors. Instead of having our services transparently return the result (success or error) of the SO call, we should look for errors there and throw an error.

e.g. throw new AgentEventNotFoundError(id) if soClient.find<AgentEventSOAttributes>(id) returns a Boom 404.

Tracking

• setup
• agent_config
• enrollment_api_key
• agent
• epm
• datasource
• data_streams
• install_script
• output
• settings
• app

original description

See https://github.com//issues/57458

I've seen this sentiment (deprecate Boom, adopt NP http errors) in Slack and elsewhere.

I can look for something more official, but I think we should avoid new Boom usage and replace the existing ones.

[elasticmachine]

Pinging @elastic/ingest-management (Team:Ingest Management)

[neptunian]

This refers to returning a Boom error in the API handlers. We can still use Boom we just need to handle them which we are doing.

https://github.com/elastic/kibana/blob/master/src/core/MIGRATION_EXAMPLES.md#http-routes

All exceptions thrown by handlers result in 500 errors. If you need a specific HTTP error code, catch any exceptions in your handler and construct the appropriate response using the provided response factory. While you can continue using the boom module internally in your plugin, the framework does not have native support for converting Boom exceptions into HTTP responses.

[jfsiii]

That link refers to routes, which are "above" services.

Maybe someone from @elastic/kibana-platform can clarify and offer some guidance?

My understanding is using Boom in services means their consumers must also use it which is undesirable and preventable.

[pgayvallet]

The idea behind getting rid of boom errors is that these errors were often thrown directly from the service layer and only 'handled' implicitly from the hapi server, making it difficult to track possible error types returned from an endpoint, while also being poor isolation between the service and routing layers. This approach should be avoided at all costs in KP, in favor of explicit errors thrown from the route handlers.

I.E instead of having myService.doSomethingRisky throw boom's 404 and 500, the suggested approach would be:

try {
      const something = await myService.doSomethingRisky()
      return response.ok({
        body: something,
      });
    } catch (e) {
        if(isMyNotFoundError(e)) {
          return response.notFound();
        }
        return response.internalError();
    }

For the migration guide:

If your plugin still relies on throwing Boom errors from routes, you can use the router.handleLegacyErrors as a temporary solution until error migration is complete:

You can use handleLegacyErrors atm, but this API was meant to be a temporary measure to avoid slowing down plugin migrations. Mid term, this legacy 'boom' wrapper will be deprecated and removed, meaning that all plugins should properly handle errors explicitly in their route handlers.

[jen-huang]

(Removing release note labels as this is not a release-specific chore/tech debt issue.)

[ph]

Isn't this discussion also happening in #66688 If so we could only keep @skh issue open?

[jfsiii]

Reopening this to track the the second phase of work from #66688