Skip to content

Resolve created_by and oncall_schedule n+1's in group member index

What does this MR do?

  • Resolves user: :oncall_schedules n+1.
  • Resolves :created_by n+1 in Groups::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 the includes formatting as described in Parameters. Same result, merely removing explicit mapping function from our code.

oncall schedules

What did this do before?

  1. app/views/groups/group_members/index.html.haml calls group_members_list_data_attributes (in a few places).
  2. Groups::GroupMembersHelper#group_members_list_data_attributes calls members_data_json.
  3. Groups::GroupMembersHelper#members_data_json calls MemberSerializer.
  4. MemberSerializer uses the MemberEntity.
  5. MemberEntity exposes the user via MemberUserEntity.
  6. ee version of MemberUserEntity exposes oncall_schedules, which uses the OncallScheduleEntity, which is then executed when invoked through the view in the first step causing our n+1.

created_by

What did this do before?

  1. app/views/groups/group_members/index.html.haml calls group_members_list_data_attributes (in a few places).
  2. Groups::GroupMembersHelper#group_members_list_data_attributes calls members_data_json.
  3. Groups::GroupMembersHelper#members_data_json calls MemberSerializer.
  4. MemberSerializer uses the MemberEntity.
  5. MemberEntity exposes created_by, which is then executed when invoked through the view in the first step causing our n+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

Availability and Testing

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

Merge request reports