Merge branch 'backstage/gb/improve-jobs-controller-performance' into …

…'master' Improve performance of jobs controller show Closes #60708 See merge request gitlab-org/gitlab-ce!28093
NewSD · May 28, 2019 · 029d68d · 029d68d
2 parents c2b7c6e + a66cbb6
commit 029d68d
Show file tree

Hide file tree

Showing 21 changed files with 378 additions and 373 deletions.
diff --git a/app/assets/javascripts/jobs/components/job_app.vue b/app/assets/javascripts/jobs/components/job_app.vue
@@ -86,6 +86,7 @@ export default {
       'isScrollTopDisabled',
       'isScrolledToBottomBeforeReceivingTrace',
       'hasError',
+      'selectedStage',
     ]),
     ...mapGetters([
       'headerTime',
@@ -121,7 +122,13 @@ export default {
     // fetch the stages for the dropdown on the sidebar
     job(newVal, oldVal) {
       if (_.isEmpty(oldVal) && !_.isEmpty(newVal.pipeline)) {
-        this.fetchStages();
+        const stages = this.job.pipeline.details.stages || [];
+
+        const defaultStage = stages.find(stage => stage && stage.name === this.selectedStage);
+
+        if (defaultStage) {
+          this.fetchJobsForStage(defaultStage);
+        }
       }
 
       if (newVal.archived) {
@@ -160,7 +167,7 @@ export default {
       'setJobEndpoint',
       'setTraceOptions',
       'fetchJob',
-      'fetchStages',
+      'fetchJobsForStage',
       'hideSidebar',
       'showSidebar',
       'toggleSidebar',
@@ -269,7 +276,6 @@ export default {
           :class="{ 'sticky-top border-bottom-0': hasTrace }"
         >
           <icon name="lock" class="align-text-bottom" />
-
           {{ __('This job is archived. Only the complete pipeline can be retried.') }}
         </div>
         <!-- job log -->

diff --git a/app/assets/javascripts/jobs/components/sidebar.vue b/app/assets/javascripts/jobs/components/sidebar.vue
@@ -34,7 +34,7 @@ export default {
     },
   },
   computed: {
-    ...mapState(['job', 'stages', 'jobs', 'selectedStage', 'isLoadingStages']),
+    ...mapState(['job', 'stages', 'jobs', 'selectedStage']),
     coverage() {
       return `${this.job.coverage}%`;
     },
@@ -208,7 +208,6 @@ export default {
         />
 
         <stages-dropdown
-          v-if="!isLoadingStages"
           :stages="stages"
           :pipeline="job.pipeline"
           :selected-stage="selectedStage"

diff --git a/app/assets/javascripts/jobs/store/actions.js b/app/assets/javascripts/jobs/store/actions.js
@@ -178,38 +178,14 @@ export const receiveTraceError = ({ commit }) => {
   flash(__('An error occurred while fetching the job log.'));
 };
 
-/**
- * Stages dropdown on sidebar
- */
-export const requestStages = ({ commit }) => commit(types.REQUEST_STAGES);
-export const fetchStages = ({ state, dispatch }) => {
-  dispatch('requestStages');
-
-  axios
-    .get(`${state.job.pipeline.path}.json`)
-    .then(({ data }) => {
-      // Set selected stage
-      dispatch('receiveStagesSuccess', data.details.stages);
-      const selectedStage = data.details.stages.find(stage => stage.name === state.selectedStage);
-      dispatch('fetchJobsForStage', selectedStage);
-    })
-    .catch(() => dispatch('receiveStagesError'));
-};
-export const receiveStagesSuccess = ({ commit }, data) =>
-  commit(types.RECEIVE_STAGES_SUCCESS, data);
-export const receiveStagesError = ({ commit }) => {
-  commit(types.RECEIVE_STAGES_ERROR);
-  flash(__('An error occurred while fetching stages.'));
-};
-
 /**
  * Jobs list on sidebar - depend on stages dropdown
  */
 export const requestJobsForStage = ({ commit }, stage) =>
   commit(types.REQUEST_JOBS_FOR_STAGE, stage);
 
 // On stage click, set selected stage + fetch job
-export const fetchJobsForStage = ({ dispatch }, stage) => {
+export const fetchJobsForStage = ({ dispatch }, stage = {}) => {
   dispatch('requestJobsForStage', stage);
 
   axios

diff --git a/app/assets/javascripts/jobs/store/mutation_types.js b/app/assets/javascripts/jobs/store/mutation_types.js
@@ -24,10 +24,6 @@ export const STOP_POLLING_TRACE = 'STOP_POLLING_TRACE';
 export const RECEIVE_TRACE_SUCCESS = 'RECEIVE_TRACE_SUCCESS';
 export const RECEIVE_TRACE_ERROR = 'RECEIVE_TRACE_ERROR';
 
-export const REQUEST_STAGES = 'REQUEST_STAGES';
-export const RECEIVE_STAGES_SUCCESS = 'RECEIVE_STAGES_SUCCESS';
-export const RECEIVE_STAGES_ERROR = 'RECEIVE_STAGES_ERROR';
-
 export const SET_SELECTED_STAGE = 'SET_SELECTED_STAGE';
 export const REQUEST_JOBS_FOR_STAGE = 'REQUEST_JOBS_FOR_STAGE';
 export const RECEIVE_JOBS_FOR_STAGE_SUCCESS = 'RECEIVE_JOBS_FOR_STAGE_SUCCESS';

diff --git a/app/assets/javascripts/jobs/store/mutations.js b/app/assets/javascripts/jobs/store/mutations.js
@@ -65,6 +65,11 @@ export default {
     state.isLoading = false;
     state.job = job;
 
+    state.stages =
+      job.pipeline && job.pipeline.details && job.pipeline.details.stages
+        ? job.pipeline.details.stages
+        : [];
+
     /**
      * We only update it on the first request
      * The dropdown can be changed by the user
@@ -101,19 +106,7 @@ export default {
     state.isScrolledToBottomBeforeReceivingTrace = toggle;
   },
 
-  [types.REQUEST_STAGES](state) {
-    state.isLoadingStages = true;
-  },
-  [types.RECEIVE_STAGES_SUCCESS](state, stages) {
-    state.isLoadingStages = false;
-    state.stages = stages;
-  },
-  [types.RECEIVE_STAGES_ERROR](state) {
-    state.isLoadingStages = false;
-    state.stages = [];
-  },
-
-  [types.REQUEST_JOBS_FOR_STAGE](state, stage) {
+  [types.REQUEST_JOBS_FOR_STAGE](state, stage = {}) {
     state.isLoadingJobs = true;
     state.selectedStage = stage.name;
   },

diff --git a/app/assets/javascripts/jobs/store/state.js b/app/assets/javascripts/jobs/store/state.js
@@ -25,7 +25,6 @@ export default () => ({
   traceState: null,
 
   // sidebar dropdown & list of jobs
-  isLoadingStages: false,
   isLoadingJobs: false,
   selectedStage: '',
   stages: [],

diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb
@@ -8,16 +8,18 @@ class BuildDetailsEntity < JobEntity
   expose :stuck?, as: :stuck
   expose :user, using: UserEntity
   expose :runner, using: RunnerEntity
+  expose :metadata, using: BuildMetadataEntity
   expose :pipeline, using: PipelineEntity
 
   expose :deployment_status, if: -> (*) { build.starts_environment? } do
     expose :deployment_status, as: :status
-
-    expose :persisted_environment, as: :environment, with: EnvironmentEntity
+    expose :persisted_environment, as: :environment do |build, options|
+      options.merge(deployment_details: false).yield_self do |opts|
+        EnvironmentEntity.represent(build.persisted_environment, opts)
+      end
+    end
   end
 
-  expose :metadata, using: BuildMetadataEntity
-
   expose :artifact, if: -> (*) { can?(current_user, :read_build, build) } do
     expose :download_path, if: -> (*) { build.artifacts? } do |build|
       download_project_job_artifacts_path(project, build)

diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb
@@ -20,16 +20,39 @@ class DeploymentEntity < Grape::Entity
   expose :created_at
   expose :tag
   expose :last?
-
   expose :user, using: UserEntity
-  expose :commit, using: CommitEntity
-  expose :deployable, using: JobEntity
-  expose :manual_actions, using: JobEntity, if: -> (*) { can_create_deployment? }
-  expose :scheduled_actions, using: JobEntity, if: -> (*) { can_create_deployment? }
+
+  expose :deployable do |deployment, opts|
+    deployment.deployable.yield_self do |deployable|
+      if include_details?
+        JobEntity.represent(deployable, opts)
+      elsif can_read_deployables?
+        { name: deployable.name,
+          build_path: project_job_path(deployable.project, deployable) }
+      end
+    end
+  end
+
+  expose :commit, using: CommitEntity, if: -> (*) { include_details? }
+  expose :manual_actions, using: JobEntity, if: -> (*) { include_details? && can_create_deployment? }
+  expose :scheduled_actions, using: JobEntity, if: -> (*) { include_details? && can_create_deployment? }
 
   private
 
+  def include_details?
+    options.fetch(:deployment_details, true)
+  end
+
   def can_create_deployment?
     can?(request.current_user, :create_deployment, request.project)
   end
+
+  def can_read_deployables?
+    ##
+    # We intentionally do not check `:read_build, deployment.deployable`
+    # because it triggers a policy evaluation that involves multiple
+    # Gitaly calls that might not be cached.
+    #
+    can?(request.current_user, :read_build, request.project)
+  end
 end
diff --git a/app/serializers/pipeline_details_entity.rb b/app/serializers/pipeline_details_entity.rb
@@ -1,8 +1,11 @@
 # frozen_string_literal: true
 
 class PipelineDetailsEntity < PipelineEntity
+  expose :flags do
+    expose :latest?, as: :latest
+  end
+
   expose :details do
-    expose :ordered_stages, as: :stages, using: StageEntity
     expose :artifacts, using: BuildArtifactEntity
     expose :manual_actions, using: BuildActionEntity
     expose :scheduled_actions, using: BuildActionEntity

diff --git a/app/serializers/pipeline_entity.rb b/app/serializers/pipeline_entity.rb
@@ -20,7 +20,6 @@ class PipelineEntity < Grape::Entity
   end
 
   expose :flags do
-    expose :latest?, as: :latest
     expose :stuck?, as: :stuck
     expose :auto_devops_source?, as: :auto_devops
     expose :merge_request_event?, as: :merge_request
@@ -34,6 +33,7 @@ class PipelineEntity < Grape::Entity
 
   expose :details do
     expose :detailed_status, as: :status, with: DetailedStatusEntity
+    expose :ordered_stages, as: :stages, using: StageEntity
     expose :duration
     expose :finished_at
   end

diff --git a/changelogs/unreleased/backstage-gb-improve-jobs-controller-performance.yml b/changelogs/unreleased/backstage-gb-improve-jobs-controller-performance.yml
@@ -0,0 +1,5 @@
+---
+title: Improve performance of jobs controller
+merge_request: 28093
+author:
+type: performance
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
@@ -877,9 +877,6 @@ msgstr ""
 msgid "An error occurred while fetching sidebar data"
 msgstr ""
 
-msgid "An error occurred while fetching stages."
-msgstr ""
-
 msgid "An error occurred while fetching the board lists. Please try again."
 msgstr ""