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
_events
properties (examples in CE) should default tofalse
inIntegrations::Base::Integration
. - All integrations should have their
_events
properties totrue
per integration, based on theirsupported_events
. Prefer to do this in theIntegrations::Base
modules where possible. - Add a test that asserts that for all integration models, their
supported_events
have a corresponding_events
property set astrue
, but otherwise they are allfalse
. - The exception for
2
and3
will be our notification integrations (ones that mixinIntegrations::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 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
false
unless they were present in the.supported_events
for that type of integration.