DRAFT: RFC Search alert refactor
Created by: camdencheek
This is an "RFC PR." I'm making this a PR because attempting the refactor was easier than trying to write up the idea. I want to get people's take on the direction this is going. I'm happy to abandon this or make a replacement PR if this isn't a direction we want to go.
This PR attempts begins a refactor of our search alerts with a few goals in mind:
- The Alert and AlertObserver types should live in
internal/search/alert
- Alerts should be defined as close to the source as possible, not by inspecting errors (and especially error strings) many levels up in the call chain
- Alerts and AlertResolvers should be separate
The motivation for this is twofold:
- I would like to move more things out of
graphqlbackend
, but that is difficult to do sincealertFromError
referenced error types defined ingraphqlbackend
. This is blocks movingalertObserver
and, by proxy,aggregator
. - Generating alerts based on error text is pretty fragile. Since I've been here, I've come across multiple instances of alert definitions that were dead code because the error text searched for no longer exists. This would be less fragile if we generate the alerts either at the same time we create the error, or as close to it as possible with well-typed errors rather than working from error strings.
The solution presented by this PR:
- Separate
internal/search/alert.Alert
andcmd/frontend/graphqlbackend.searchAlertResolver
. This makes the resolver a thin wrapper type around the alert. - Add
internal/search/alert.AlertableError
. This is an error type that can be unwrapped, yielding the original error and an alert that is defined at the timeAlertableError
is created. This allows us to create our alerts at the source. - Convert
alertFromError
so that attempts to unwrap into anAlertableError
, then return the alert if it succeeds. - This allows
alertFromError
to not reference any specific error types ingraphqlbackend
, so it can live ininternal/search/alert
. - This is largely compatible with the framework we already have. The only significant difference is we can't use error types defined in graphqlbackend after the error hits
Observer
Stacked on #20915