Compute merge request metrics before the post-merge transaction

Marc Shawrequested to merge
628868-metrics-outside-merge-transaction into master

What does this MR do and why?

MergeWorker holds a DB transaction open while it computes merge request metrics. MergeRequests::PostMergeService#create_event opens Event.transaction, and inside it EE's MergeRequestMetricsService#merge loads every commit from Gitaly, runs DiffStats and parses all diff files. That is what the PatroniLongRunningTransactionDetected alerts for MergeWorker point at, and a 30 minute transaction_timeout is about to be enforced in production.

Behind the merge_request_metrics_outside_merge_transaction flag, the metrics are now computed before the transaction opens and the transaction only writes them. The values written are unchanged.

Detailed context for AI agents

Root cause and how it was found. A merge was run inline on GDK with a tracer subscribed to sql.active_record (every BEGIN/COMMIT) and prepended on Gitlab::GitalyClient.call (every Gitaly RPC). The trace showed MergeRequests::PostMergeService#create_event opening Event.transaction, and inside it MergeRequestMetricsService#merge. The EE override (ee/app/services/ee/merge_request_metrics_service.rb) calls Analytics::MergeRequestMetricsCalculator#productivity_data and #line_counts_data there. Those load every commit of the MR from Gitaly (ListCommitsByOid, batches of 250) only to read first and last commit dates, call Gitaly DiffStats for lines_count, load and parse every merge_request_diff_files row (external diffs included) to sum added and removed lines, and run the related_notes UNION query. While that runs the Postgres session is idle in transaction, holding the events insert and the merge_request_metrics row lock.

Reproduced on every merge path tested: merge commit, fast-forward with automatic rebase (CreateRefService), squash, merge_requests_merge_data_dual_write flag on, merge closing an issue, 300 commits, large diff. With Gitaly slowed by 4 seconds per RPC, pg_stat_activity showed exactly the alert signature (idle in transaction, last query the diff files SELECT).

What changed. CE gains a no-op MergeRequestMetricsService#prepare_merge_data. EE overrides it to compute and memoize the calculator output (calculated_merge_data), and EE #merge now reads the memoized hash. PostMergeService#create_event calls prepare_merge_data before Event.transaction when the flag is enabled for the project. The CE hook is needed because CE opens the transaction, so EE has no other point to run before it.

Flag semantics. merge_request_metrics_outside_merge_transaction, gitlab_com_derisk, default off, project actor. Off: prepare_merge_data is a no-op and the calculator runs inside the transaction on first call from merge, identical to before. On: the calculator runs before the transaction and merge only writes.

Failure mode. If the calculator raises, the merge fails at the same point in PostMergeService as before, after mark_as_merged. Before, the raise happened inside the transaction; now just before it opens. Metrics values are identical either way.

Alternatives rejected.

  • Move the metrics update to an async worker: changes visibility semantics for analytics and is bigger than needed.
  • Drop Event.transaction: it exists so the Event and the metrics update stay in sync.
  • Do the work lazily in EE with no CE hook: impossible, the transaction is opened by the CE caller.

Out of scope. A second, smaller occurrence: ApprovalRules::FinalizeService reads the CODEOWNERS blob from Gitaly inside its transaction via ApprovalWrappedCodeOwnerRule#section_optional?. Only when code owner rules exist. Left for a follow-up.

Verification.

  • bundle exec rspec spec/services/merge_requests/post_merge_service_spec.rb ee/spec/services/ee/merge_request_metrics_service_spec.rb: 31 examples, 0 failures.
  • New specs: PostMergeService orders prepare_merge_data, then Event.transaction, then merge with the flag on, and only merge with the flag off. EE spec: after prepare_merge_data the calculator is not instantiated again and the metrics still land.
  • Manual GDK check with the tracer: flag on, ListCommitsByOid and DiffStats no longer occur inside any transaction and commits_count, added_lines, removed_lines, first_commit_at are still written. Flag off, both RPCs still run inside the transaction.
References, screenshots and how to validate locally

References

Screenshots or screen recordings

Not applicable, backend only.

How to set up and validate locally

  1. In a Rails console enable the flag for a project: 
    project = Project.find_by_full_path("your-namespace/your-project")
Feature.enable(:merge_request_metrics_outside_merge_transaction, project)
  2. Merge a merge request in that project.
  3. Check merge_request.metrics (commits_count, added_lines, removed_lines, first_commit_at) is populated.
  4. Optionally subscribe to sql.active_record with ActiveSupport::Notifications around the merge to confirm BEGIN/COMMIT no longer wraps the Gitaly calls.
Edited by Marc Shaw

Merge request reports