Resolve created_by and oncall_schedule n+1's in group member index
requested to merge 21033-controller-groups-groupmemberscontroller-index-executes-more-than-100-sql-queries-p80-108-6 into master
What does this MR do?
- Resolves
user: :oncall_schedules
n+1. - Resolves
:created_by
n+1 inGroups::GroupMembersController#index
- Moves query analysis from serializer to controller spec with rendered views to catalog and analyze our performance/db query count.
- Modifies formatting of
preload_all
to match theincludes
formatting as described in Parameters. Same result, merely removing explicit mapping function from our code.
oncall schedules
What did this do before?
-
app/views/groups/group_members/index.html.haml
calls group_members_list_data_attributes (in a few places). -
Groups::GroupMembersHelper#group_members_list_data_attributes
calls members_data_json. -
Groups::GroupMembersHelper#members_data_json
calls MemberSerializer. -
MemberSerializer
uses the MemberEntity. -
MemberEntity
exposes theuser
via MemberUserEntity. -
ee
version ofMemberUserEntity
exposes oncall_schedules, which uses the OncallScheduleEntity, which is then executed when invoked through the view in the first step causing ourn+1
.
created_by
What did this do before?
-
app/views/groups/group_members/index.html.haml
calls group_members_list_data_attributes (in a few places). -
Groups::GroupMembersHelper#group_members_list_data_attributes
calls members_data_json. -
Groups::GroupMembersHelper#members_data_json
calls MemberSerializer. -
MemberSerializer
uses the MemberEntity. -
MemberEntity
exposes created_by, which is then executed when invoked through the view in the first step causing ourn+1
.
Performance analysis locally on GDK
Caveat This was done on a group where there were a handful of members(10), some of them being invited(45), and Access Requests(2). The numbers and query times will vary based upon size.
Before(after gdk restart and cache warm-up)
- ActiveRecord Query time:
~142ms
- queries(cached/uncached):
119
After(after gdk restart and cache warm-up)
- ActiveRecord Query time:
~80ms
- queries(cached/uncached):
56
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Related to #21033
Edited by Doug Stull