Skip to content

Integration hooks loading more frequently than necessary

This issue came out of this discussion &7290 (comment 1175265005).

Problem

Nearly all hook event properties are default set to true for all integrations regardless of whether the integration uses the event hooks or not.

Only integrations that specifically allow people to check and uncheck the hook event options in the integration form can have false values for some events (limited to integrations that mixin Integrations::Base::ChatNotification only).

The integration models themselves implement a .supported_events method that specifies whether they support the events, but that is checked later at the point where we have loaded the record and attempted to execute it.

We are currently loading integrations 10 million times unnecessarily per day on GitLab.com #382999 (comment 2195740529) - most likely all on the DB primary.

We're additionally building data payloads, which ultimately are for nothing as the Project#has_active_integrations? check is much less helpful in terms of a performance optimisation made before attempting to build data payloads or execute integration hooks, as projects with any enabled integrations will generally report true even when it's not.

This will lead to many unnecessary PostgreSQL queries.

Proposal

Initialise new records with correct defaults:

  1. All _events properties (examples in CE) should default to false in Integrations::Base::Integration.
  2. All integrations should have their _events properties to true per integration, based on their supported_events. Prefer to do this in the Integrations::Base modules where possible.
  3. Add a test that asserts that for all integration models, their supported_events have a corresponding _events property set as true, but otherwise they are all false.
  4. Update integrations development guide with necessary new information https://docs.gitlab.com/development/integrations/
  5. The exception for 2 and 3 will be our notification integrations (ones that mixin Integrations::Base::ChatNotification) which present a form with checkboxes for people to set the _events properties themselves. We could have these _events properties all be the new default false (that will be specified in Integrations::Base::Integration) which will mean that the checkboxes in their forms will present as unchecked by default - which will mean people don't need to uncheck ones they don't want as they currently need to do.

Migrate existing records:

  1. Add a data migration to correct existing data. For every type of integration, we would ensure that all of its event hooks were false unless they were present in the .supported_events for that type of integration.

Note: When an existing integration adds a new supported_events they would need to add a data migration to set old records's _events attribute to true in order for the older integrations to be triggered. This would be necessary for all integrations besides out notification integrations (ones that mixin Integrations::Base::ChatNotification) which present a form with checkboxes for people to set the _events properties themselves. We need to work out how best to alert teams of this requirement - one method might be to have a Danger bot comment appear on the MR whenever there is a change to supported_events (?) but this part of the issue needs to be refined/considered further.

Edited by Luke Duncalfe