Skip to content
Snippets Groups Projects

Draft: Add performance tests for sync license

Closed Marcos Rocha requested to merge mc_rocha-performance-specs-sync-policy into master
4 unresolved threads

What does this MR do and why?

This MR adds performance tests for sync license feature.

Related to #433407 (closed)

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Marcos Rocha

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
116 worker.perform(configuration.project_id, configuration.id)
117 end.to issue_same_number_of_queries_as(control)
118 end
119 end
120
121 context 'when the policy did not changed but a user is added to the project' do
122 it 'does not increase the query count', :use_sql_query_cache do
123 control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
124 worker.perform(configuration.project_id, configuration.id)
125 end
126
127 configuration.project.add_developer(create(:user))
128
129 expect do
130 worker.perform(configuration.project_id, configuration.id)
131 end.not_to exceed_query_limit(control)
  • 37 37
    38 38 subject
    39 39 end
    40
    41 context 'with multiple projects in the namespace' do
    42 let_it_be(:worker) { Security::ProcessScanResultPolicyWorker }
    43
    44 it 'does trigger SyncScanResultPoliciesProjectService for each project in group' do
    45 create_list(:project, 10, namespace: namespace)
    46
    47 expect(worker).to receive(:perform_async).and_call_original.exactly(11).times
    • Author Maintainer

      This spec captures one of the root causes of the incident mentioned here:

      Huge number of invocation of Security::ProcessScanResultPolicyWorker

      If we follow the suggestion of using a batch of projects, this will reduce the number of invocations of Security::ProcessScanResultPolicyWorker.

    • @mc_rocha Once we introduce the suggested ID batching, this test will fail, despite it being a performance improvement. It seems to test this loop. Which sorts of performance issues could it detect that we might introduce? :thinking:

      Edited by Dominic Bauer
    • Author Maintainer

      Which sorts of performance issues could it detect that we might introduce? :thinking:

      Great question @bauerdominic!

      Maybe it won't detect new performance issues, but I had it for 2 reasons:

      1. I believe this is a spec we were missing. If we had it before, it could have helped us identify the potential performance risk.

      2. If this spec is merged before the ID batching update, it will be explicit that we are no longer enqueuing a job for each project.

    • @mc_rocha Agreed that the spec is useful! I don't think we disagree here. My concern regarding #433409 (closed) is that is aimed at detecting performance regressions by counting queries, especially regarding the improvements from &9971.

      I think the improvements &9971 will bring can be tested by counting queries, since we will be able to express the assertions in relative terms: If only one policy is changed, we should need less queries than if two policies were changed.

      But since today's implementation deletes and recreates all scan_result_policy_reads and their associations, a change to one or two policies take us the same amount of queries, because we also rebuild all other policies. So we would end up with an already failing test case, and I don't think these are useful (!138401 (comment 1681530094)).

      I believe this is a spec we were missing. If we had it before, it could have helped us identify the potential performance risk.

      I am not sure about this, since the worker behaved as specified in the test case. What happened was that the worker got repeatedly invoked for a group with a large number of contained projects. So we could also have written the test like:

      create_list(:project, 12345, namespace: namespace)
      expect(worker).to receive(:perform_async).and_call_original.exactly(12346).times

      We correctly get 12346 invocations of ProcessScanResultPolicyWorker, so this test wouldn't have saved us.

      WDYT?

    • Author Maintainer

      Thanks for the detailed response @bauerdominic!

      I believe it is a missing spec because the expected behavior was not explicit in the tests. No spec indicated that we are invoking a job for each project in a group.

      I thought that making this behavior explicit in a spec could have helped us because this incident and this another incident was caused by a large number of invocations of a worker.

      In one incident we opted to inline some of the workers invocations. And for the most recent one we are discussing about batching to reduce the number of workers.

      I believe that worker invocation is a point for attention during our reviews, and specs like this one could help anticipate future performance incidents.

      Once we introduce the suggested ID batching, this test will fail, despite it being a performance improvement.

      I believe this is expected since we would be changing the service's behavior. Fixing this test in the future will demonstrate the performance improvement.

    • Please register or sign in to reply
  • 102 enabled: true,
    103 rules: [
    104 { type: 'scan_finding', branches: %w[main], vulnerabilities_allowed: 0,
    105 severity_levels: %w[critical], scanners: %w[container_scanning],
    106 vulnerability_states: %w[detected] }
    107 ],
    108 actions: [
    109 { type: 'require_approval', approvals_required: 1, user_approvers: %w[admin] }
    110 ]
    111 }
    112
    113 policies[:scan_result_policy].push(policy)
    114
    115 expect do
    116 worker.perform(configuration.project_id, configuration.id)
    117 end.to issue_same_number_of_queries_as(control)
  • mentioned in issue #433409 (closed)

  • mentioned in issue #433407 (closed)

  • Author Maintainer

    The relevant spec that could be added, was added by MR !139409 (merged).

  • closed

  • Please register or sign in to reply
    Loading