Add approve_merge_request user experience SLI

Marc Shawrequested to merge
601335-approve-merge-request-ux-sli into master

Instruments the Approve MR journey as a User Experience SLI. The web UI's Approve button posts to POST /projects/:id/merge_requests/:merge_request_iid/approve, so instrumenting that one endpoint covers UI and API approvals alike.

Three things worth a reviewer's attention:

  • The SLI uses sync_slow (5s apdex threshold), not the async_fast (15s) named in the issue. The journey turned out to be entirely synchronous, and the endpoint is already declared urgency: :low, so 15s would never signal anything.
  • A rejected approval is marked as an error explicitly. Grape unwinds with throw, which Labkit cannot observe, so without that the 401 would land in the SLI as a success.
  • When publish_review=true, the endpoint also publishes the user's draft notes before approving. A checkpoint marks where that publishing ends and the approval itself starts, so the two can be told apart in the same journey.

Rebuilt from scratch on current master, replacing !238481 (closed) (now closed).

Detailed context for AI agents

Changes

  1. config/user_experience_slis/approve_merge_request.yml - new SLI definition.
  2. lib/api/merge_request_approvals.rb - wraps the body of POST .../approve in Labkit::UserExperienceSli.start(:approve_merge_request).
  3. spec/requests/api/merge_request_approvals_spec.rb - five examples covering start/complete, the error path, the not-started path, and the publish-step checkpoint in both directions.

Why the endpoint, and not a controller or GraphQL mutation

MergeRequestPresenter#api_approve_path (app/presenters/merge_request_presenter.rb:218) is exposed on MergeRequestPollCachedWidgetEntity as api_approve_path, and MRWidgetService#approveMergeRequest posts to it. So the web UI Approve button and direct REST callers converge on this single endpoint. There is no separate approve controller action and no mergeRequestApprove GraphQL mutation, so no other entry point needs instrumenting.

The consequence is that the SLI blends human UI approvals with bot and API approvals. That is deliberate - both are the same system-side journey - but it means the ID has no _ui suffix, unlike submit_mr_review_ui and ui_button_rebase.

Why sync_slow rather than the async_fast named in the issue

The issue describes the journey as ending when "approval rules re-evaluation completes (merge-check state reflects the new approval)" and suggests async_fast, which implies the journey continues into Sidekiq.

It does not. MergeRequests::ApprovalService#execute does everything the user waits on inside the request:

  • save_approval persists the Approval record,
  • update_reviewer_state flips the reviewer to approved,
  • reset_approvals_cache (EE: merge_request.reset_approval_cache!) invalidates the approval state cache,
  • trigger_merge_request_merge_status_updated and trigger_merge_request_approval_state_updated push the new state to GraphQL subscribers,
  • present_approval serialises the recomputed approval state into the response.

MergeRequests::ApprovedEvent is published afterwards, but every subscriber is a side effect that the user is not waiting on: CreateApprovalEventWorker, CreateApprovalNoteWorker, ResolveTodosAfterApprovalWorker, ExecuteApprovalHooksWorker, StreamApprovalAuditEventWorker, ApprovalMetricsEventWorker, ProcessAutoMergeFromEventWorker. None of them update the approval or merge-check state the widget renders.

So there is nothing to resume in a worker, the journey is one synchronous request, and the apdex threshold should match the request budget. The endpoint is declared urgency: :low, which is a 5s target; sync_slow is also 5s. async_fast would set the bar at 15s, which a request already budgeted at 5s would essentially never breach - the SLI would read ~100% forever and never signal a regression. immediate_web_merge and ui_button_rebase both use sync_slow for the same reason.

sync_slow is therefore a deliberate decision, not an oversight - the async_fast in the issue is the template default and does not match this journey.

Error attribution: Grape throws, Labkit rescues

Labkit::UserExperienceSli::Experience#completable is:

def completable(**extra, &)
  yield self
rescue StandardError => e
  error!(e)
  raise
ensure
  complete(**extra)
end

Grape's error! does throw :error, ... (grape-2.4.0/lib/grape/dsl/inside_route.rb:173), and unauthorized! routes through render_api_error_with_reason! to it. A throw is not a StandardError, so:

  • the ensure fires and the experience completes, meaning the request is not lost from the denominator, but
  • the rescue never fires, so error! is never called, so the failure increments gitlab_user_experience_total{error="false"} and counts as an apdex success.

Marking the experience before unauthorized! fixes that. The spec proves it: with the experience.error! line removed, complete_user_experience(:approve_merge_request, error: true) fails with expected total='error: true' counter to increase by 1, but increased by 0.

Why the experience starts after the lookup and SHA check

find_merge_request_with_access (404) and check_sha_param! (409) both throw the same way. Starting the experience after them keeps "merge request does not exist" and "stale SHA" out of the SLI entirely, rather than recording them as fast successes. Requests that get past both are real approval attempts, and every exit from the block is then attributed correctly - unauthorized! explicitly, anything raising StandardError via Labkit's own rescue.

Only the 409 path has a not_to start_user_experience example, which is deliberate. start sits below both guards, so moving it above either one breaks that single example; a matching 404 example would not add coverage that the suite does not already have.

The publish_review checkpoint

A reviewer asked whether the publish_review=true case should be accounted for. When that flag is set, the approve endpoint also runs DraftNotes::PublishService to publish all of the user's pending draft notes before it approves the merge request. That is extra work inside the same 5s apdex budget, and without any instrumentation a slow draft-publish and a slow approval look identical in the SLI.

The alternative considered was giving publish_review its own experience id, making it a second journey. That was rejected: the user clicked one button and waited for one outcome, so two ids would fragment a single action into two unrelated measurements. Instead a checkpoint is recorded right after DraftNotes::PublishService runs and before the approval itself:

# Separates draft publishing from the approval itself, so a slow review
# submission is distinguishable from a slow approval in the same journey.
experience.checkpoint(checkpoint_action: 'notes_published')

A checkpoint does not change the apdex calculation - it only records @checkpoint_time and increments gitlab_user_experience_checkpoint_total{checkpoint="intermediate"}. It gives a second timestamp inside the same journey, so checkpoint - start is everything the draft publish did and end - checkpoint is the approval.

The checkpoint_action: 'notes_published' value deliberately matches the one already used at app/controllers/projects/merge_requests/drafts_controller.rb:73, so both call sites land under the same label in dashboards. One difference from that controller: there the same checkpoint is gated on result[:async_notifications], because in that context it means "the rest of the experience is being handed off to a worker that will complete it". That meaning does not apply here - this call simply marks "publishing has finished, approval is starting", so it always fires when publish_review is true, regardless of what PublishService returns.

Measured live on a local GDK, approving with one pending draft note:

start:            0s
notes_published:  3.503s
end:              3.691s

Draft publishing took about 3.5s of the 3.691s total; the approval itself took about 0.19s. That is the split that was previously invisible - without the checkpoint this would show up as a single 3.691s approval. A second run of the same path measured 0.26s end to end, so there is real variance on GDK; the point is that the checkpoint attributes the time correctly, not that publishing is always this slow.

Why the spec does not use Labkit's own counter matcher

The obvious way to test this would be Labkit's checkpoint_user_experience matcher against gitlab_user_experience_checkpoint_total{checkpoint="intermediate"}. That does not work on this endpoint: the Labkit Sidekiq client middleware checkpoints the same experience once for every job the approval enqueues, and approving a merge request enqueues 8 jobs. So:

  • a negated assertion fails outright - expected checkpoint='intermediate' counter not to increase, but increased by 8
  • a positive assertion is vacuous - it passes on the 8 auto-checkpoints from the middleware alone, even with the production checkpoint line deleted

The spec therefore wraps Labkit::UserExperienceSli.get to capture the experience instance, wraps its checkpoint method, and collects every checkpoint_action passed to it. It then asserts on the label 'notes_published' specifically rather than on the counter's raw increment. Two examples: one asserts the label is present when publish_review: true, one asserts it is absent on a plain approve.

This was mutation-tested by deleting the production checkpoint line: the positive example then fails, and the failure output lists the 8 "sidekiq_job_scheduled" actions captured instead, confirming the assertion exercises the new line and not the middleware's checkpoints.

One tradeoff, accepted knowingly: stubbing Labkit::UserExperienceSli.get couples the spec to a Labkit internal. It is the only seam that distinguishes this checkpoint from the middleware's, and the alternative - asserting on the emitted log line - is more brittle.

What is deliberately not covered

  • POST .../unapprove and PUT .../reset_approvals are separate journeys and are not instrumented here.
  • render_api_error on the publish_review failure path is called without a bang, and no such method exists (only render_api_error! and render_api_error_with_reason! in lib/api/helpers.rb). It raises NoMethodError and Grape turns that into a 500, which is why the existing spec asserting :internal_server_error still passes. Pre-existing on master and out of scope here. Incidentally it is a StandardError, so Labkit attributes that path correctly on its own.
  • No changelog trailer: this is internal observability with no user-facing behaviour change.
  • No feature flag: the instrumentation is a counter increment and a log line on a path that already runs.

Feature category

code_review_workflow, which does not match the endpoint's own feature_category :source_code_management. Raised in review and settled by the Source Code and Code Review PMs: Source Code owns the configuration of approval rules, Code Review owns the approval experience inside a merge request. This SLI measures the latter.

Verification

  • bundle exec rspec spec/requests/api/merge_request_approvals_spec.rb - 53 examples, 0 failures, 4 pending (pre-existing granular-token pendings).
  • bundle exec rspec ee/spec/requests/api/merge_request_approvals_spec.rb - 60 examples, 0 failures, 3 pending.
  • Mutation checks: removing experience.error! makes the error-path example fail as described above; removing the notes_published checkpoint makes the publish-step example fail.
  • bundle exec rubocop clean on all three files; lefthook pre-commit and pre-push (rubocop, danger, permissions-verify, openapi_docs, unused-code-linter) all passed.

Relationship to the previous attempt

!238481 (closed) was opened in May 2026 and went stale - branched off a May commit, three milestones behind, and pinned to gitlab-labkit 2.0.0 while master is now on 4.6.0. It wrapped the whole endpoint body including the lookup, used async_fast, did not attribute errors, and only tested the happy path. This branch is a fresh implementation off current master rather than a rebase.

References

Edited by Marc Shaw

Merge request reports