Record the global_search error SLI on the zoekt web success path

🤖 AI-authored change.

What does this MR do and why?

Zoekt exact code search never records a successful search, so its global_search error-rate SLI sits at a constant 100% and cannot be alerted on. The recording ran only inside the server_error? branch, so a success recorded nothing and an error recorded both halves of the ratio. This resolver is the only recording site for that population.

The fix moves the call into an ensure on #results, so every exit path records; classifies the error as "nil result or server_error?"; and makes the recording itself unable to change the response. One deliberate behaviour change follows: a failed?-but-not-server_error? result (invalid regex, abusive term) now lands in the denominator as a non-error, which preserves exactly what the old numerator counted while fixing the denominator.

Reviewer focus: ee/app/graphql/resolvers/search/blob/blob_search_resolver.rb — the ensure, the new private record_error_rate, and the rescue StandardError that routes through track_and_raise_for_dev_exception so it is loud in test and dev and silent in production.

References

Unblocks widening runbooks MR 11489's ZoektSearchErrorRateHigh, which today excludes this population on purpose.

Parent epic: &23480

Mechanism, citations and what I did not verify

Why this population had no denominator

Per Labkit::ApplicationSli#increment there is no separate "success" call site; record_error_rate advances the total and, when error: true, the error total too.

SearchController#show runs haml_search_results unless multi_match?(...), and ee/app/controllers/ee/search_controller.rb overrides multi_match? to scope == 'blobs' && search_type == 'zoekt'. So for exact code search haml_search_results — and with it SearchController#record_search_error — never runs; the results come from the GraphQL blobSearch query, i.e. this resolver. lib/api/search.rb uses the same ensure shape on the API path.

What counts as an error

  • nil — the Benchmark.realtime block raised before a result existed. No search happened; a service failure.
  • server_error? — unchanged from before.
  • failed? but not server_error? (invalid regex, abusive term) — now in the denominator as a non-error. This preserves exactly what the old numerator condition counted while fixing the denominator.

record_apdex is untouched: it was already on the success path, so ZoektSearchApdexBurn was never affected.

Why the rescue is not a blanket swallow

record_error_rate now runs from an ensure on every request. A raise there would replace the exception already in flight, including the intentional BaseError for an invalid regex. should_raise_for_dev? in lib/gitlab/error_tracking.rb gates the re-raise on Rails.env.development? || Rails.env.test?, so a genuine labels mismatch fails CI and a production metrics failure cannot turn a good answer into a 500.

The previously-passing "error is not an internal server error" example is inverted: that result now lands in the denominator as a non-error.

Verification

At the first commit (fe11802e), ee/spec/requests/api/graphql/search/blob_search_spec.rb ran 30 examples with 0 failures and RuboCop was clean on the changed spec file. Restoring the numerator-only shape turns four examples red; keeping the ensure but hardcoding error: true turns three red — so the examples pin both that the call happens and that its error: value is right.

What I did not verify

  • The examples added in the second commit (f259dee8) were not run locally. The file's before hook needs a live Zoekt test service and now times out in setup on that box for every example in the file, including untouched ones used as a control. The 30/0 figures above therefore cover fe11802e only; CI on f259dee8 is the authority and its pipeline was still running when this description was written.
  • No production or staging metric was observed. The claim that the ratio is currently a constant 100% for this population is derived from the code path, not from a Prometheus query.
  • The runbook alert widening is not part of this MR and has not been done.

MR acceptance checklist

  • Tested in all supported browsers — n/a, backend only
  • Informed Infrastructure department of a default or new setting change — n/a

🤖 Automated change. Mention @johnmason for feedback, or reply #human to escalate to John.

Edited by John Mason

Merge request reports

Loading
Loading