Backend: Add cached denormalized count of jobs - GraphQL

Problem

The example query below blows up with an Internal Server Error.

This is due to the count field on the CiJobConnection type. That field works fine for projects with low job records but when they are larger the query blows up with an Internal server error.

This prevents rollout of the new jobs table filtered search. [jobs_table_vue] Rollout Plan (#327500 - closed)

{
  project(fullPath: "gitlab-org/gitlab") {
    jobs {
      count
    }
  }
}

Potential Solution

Suggestion from @alexkalderimis in the #f_graphql channel for a solution.

We probably need a cached denormalized count of jobs (like we have for some other records), if throwing indices at the problem does not solve things. For example we do this with the open MR count on projects

Or add a limited count field: #360672 (comment 941308255)

added Category:Continuous Integration GraphQL backend devopsverify grouppipeline execution typebug labels

marked this issue as related to #356007 (closed)

marked this issue as related to #327500 (closed)

added bugperformance label

Labelling this severity1 as-per the handbook severity guides https://about.gitlab.com/handbook/engineering/quality/issue-triage/#severity. On large enough projects (like GitLab) this definitely falls into the 9000ms+ category.

cc @marknuzzo

@samdbeckham @pburdette added to the next milestone as we don't look to have room with other VerifyP1 and VerifyP2 issues in the current milestone. let's move the rollout issues there and block them on this issue too.

@jheimbuck_gl issues have been updated! Thanks

Thanks everyone - I updated the labeling so this will show up in our needs weight board to be weighted next week too.

added severity1 label

changed milestone to %15.1

added VerifyP2 label

changed the description

added priority1 label

changed title from Add cached denormalized count of jobs - GraphQL to Backend: Add cached denormalized count of jobs - GraphQL

added needs weight label

added sectionops label

added [deprecated] Accepting merge requests label

mentioned in issue #356007 (closed)

mentioned in issue gitlab-org/ci-cd/pipeline-execution#98 (closed)

I see that the count attribute is defined by Types::CountableConnectionType. Maybe we could define a limited count method as well:

diff --git a/app/graphql/types/countable_connection_type.rb b/app/graphql/types/countable_connection_type.rb
index 0f24964daa6..5b01663cfc0 100644
--- a/app/graphql/types/countable_connection_type.rb
+++ b/app/graphql/types/countable_connection_type.rb
@@ -6,6 +6,9 @@ class CountableConnectionType < GraphQL::Types::Relay::BaseConnection
     field :count, GraphQL::Types::Int, null: false,
           description: 'Total count of collection.'
 
+    field :count_with_limit, GraphQL::Types::Int, null: false,
+          description: 'Total count of collection.'
+
     def count
       # rubocop: disable CodeReuse/ActiveRecord
       relation = object.items
@@ -20,5 +23,20 @@ def count
         relation.size
       end
     end
+
+    def count_with_limit
+      # rubocop: disable CodeReuse/ActiveRecord
+      relation = object.items
+
+      # sometimes relation is an Array
+      relation = relation.reorder(nil) if relation.respond_to?(:reorder)
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      if relation.try(:group_values)&.present?
+        relation.size.keys.size
+      else
+        relation.page.total_count_with_limit(:all, limit: 1000)
+      end
+    end
   end
 end

@pburdette wdyt?

@mbobin Given it's defined on countable_connection_type that would be a smooth refactor for us. I guess my only concern would be just having access to count and others not knowing it's potential to throw an internal server error. Maybe we just change the docs on the field? Could be as simple as that?

EDIT: But yes countWithLimit would work for us.

Any network call has the potential to throw an internal server error, but this one is more likely because it's easier to have database timeouts on count queries.

Maybe we just change the docs on the field? Could be as simple as that?

Maybe we should not expose counts in this case since it'll most likely time out.

Any network call has the potential to throw an internal server error, but this one is more likely because it's easier to have database timeouts on count queries

Right, my point exactly.

Maybe we should not expose counts in this case since it'll most likely time out

That's fine with me. It works fine on small pipelines with low counts. But I guess we don't have to solve this immediately on this issue, we could create a new one.

@pburdette @mbobin could you make sure you're happy with the proposal and move this to workflowready for development if it is ready for that please?

cc @marknuzzo

@jheimbuck_gl yep the proposals look good. Moved to workflowready for development

set weight to 2

removed needs weight label

changed the description

added workflowplanning breakdown label

added Stretch label

added workflowready for development label and removed workflowplanning breakdown label

added SLOMissed label

added workflowin dev label and removed workflowready for development label

assigned to @mbobin

mentioned in merge request !89642 (merged)

Thanks for working on this @mbobin! We've removed the Seeking community contributions label to avoid having multiple people working on the same issue.

removed [deprecated] Accepting merge requests label

mentioned in issue verify-stage#206 (closed)

mentioned in issue gitlab-org/ci-cd/pipeline-execution#101 (closed)

added workflowin review label and removed workflowin dev label

mentioned in commit 94daf8cf

added workflowverification label and removed workflowin review label

@pburdette @samdbeckham !89642 (merged) was merged a couple of hours ago.

Excellent news. Thanks @mbobin

@pburdette is there any frontend work to be done to start using this or can we proceed with the rollout?

@samdbeckham

is there any frontend work to be done to start using this or can we proceed with the rollout?

Yes we will need to refactor to use the new count.

@samdbeckham actually maybe not. I just took a loot at the MR @mbobin opened and it looks like there is a default count value of 1000 for the code so we might not need any changes after all.

@pburdette yeah, but if the count is greater than 1000 it will return a capped count of 1001, so it might need some UX changes to show 1001 as 1000+.

@mbobin ahh yeah good point!!!

MR opened here !90833 (merged)

closed

added workflowproduction label and removed workflowverification label

mentioned in issue #327500 (closed)