Skip to content

DRAFT: RFC Search alert refactor

Warren Gifford requested to merge backend-dry-run/cc/alert-refactor into main

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:

  1. The Alert and AlertObserver types should live in internal/search/alert
  2. 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
  3. Alerts and AlertResolvers should be separate

The motivation for this is twofold:

  1. I would like to move more things out of graphqlbackend, but that is difficult to do since alertFromError referenced error types defined in graphqlbackend. This is blocks moving alertObserver and, by proxy, aggregator.
  2. 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 and cmd/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 time AlertableError is created. This allows us to create our alerts at the source.
  • Convert alertFromError so that attempts to unwrap into an AlertableError, then return the alert if it succeeds.
  • This allows alertFromError to not reference any specific error types in graphqlbackend, so it can live in internal/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

Merge request reports

Loading