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:
- All
_eventsproperties (examples in CE) should default tofalseinIntegrations::Base::Integration. - All integrations should have their
_eventsproperties totrueper integration, based on theirsupported_events. Prefer to do this in theIntegrations::Basemodules where possible. - Add a test that asserts that for all integration models, their
supported_eventshave a corresponding_eventsproperty set astrue, but otherwise they are allfalse. - Update integrations development guide with necessary new information https://docs.gitlab.com/development/integrations/
- The exception for
2and3will be our notification integrations (ones that mixinIntegrations::Base::ChatNotification) which present a form with checkboxes for people to set the_eventsproperties themselves. We could have these_eventsproperties all be the new defaultfalse(that will be specified inIntegrations::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:
- Add a data migration to correct existing data. For every type of integration, we would ensure that all of its event hooks were
falseunless they were present in the.supported_eventsfor 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.