From f984fbd47e311f05c036248cb92db1fbc0aec802 Mon Sep 17 00:00:00 2001 From: Princi Vershwal Date: Thu, 22 Aug 2024 18:26:10 +0530 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20Improved=20the=20performance=20o?= =?UTF-8?q?f=20the=20/members/events/=20aggregated=5Fclick=5Fevent=20endpo?= =?UTF-8?q?int=20(#20790)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ref https://linear.app/tryghost/issue/ONC-216/improve-the-performance-of-the-membersevents-aggregated-click-event --- .github/workflows/ci.yml | 7 ++ .../core/server/models/base/plugins/crud.js | 14 +++ .../core/server/models/member-click-event.js | 7 +- .../__snapshots__/activity-feed.test.js.snap | 96 ++++++++++++++++++ .../test/e2e-api/admin/activity-feed.test.js | 42 ++++++++ .../lib/repositories/EventRepository.js | 98 ++++++++++++++++--- 6 files changed, 247 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec4707ae62..a5c1ef91bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -843,6 +843,13 @@ jobs: run: | ghost update -f -d $V4_DIR --archive $(pwd)/ghost/core/ghost.tgz + - name: Save Ghost CLI Debug Logs + if: failure() + uses: actions/upload-artifact@v3 + with: + name: ghost-cli-debug-logs + path: /home/runner/.ghost/logs/ + - name: Clean Install run: | DIR=$(mktemp -d) diff --git a/ghost/core/core/server/models/base/plugins/crud.js b/ghost/core/core/server/models/base/plugins/crud.js index 46ab3b1b34..be67979ff2 100644 --- a/ghost/core/core/server/models/base/plugins/crud.js +++ b/ghost/core/core/server/models/base/plugins/crud.js @@ -120,6 +120,20 @@ module.exports = function (Bookshelf) { }); } + if (Array.isArray(options.cte)) { + options.cte.forEach((cte) => { + itemCollection.query((qb) => { + qb.with(cte.name, qb.client.raw(cte.query)); + }); + }); + } + + if (options.from) { + itemCollection.query((qb) => { + qb.from(options.from); + }); + } + //option param to skip distinct from count query, distinct adds a lot of latency and in this case the result set will always be unique. if (unfilteredOptions.useBasicCount) { options.useBasicCount = unfilteredOptions.useBasicCount; diff --git a/ghost/core/core/server/models/member-click-event.js b/ghost/core/core/server/models/member-click-event.js index 25728a1d56..ff25586d80 100644 --- a/ghost/core/core/server/models/member-click-event.js +++ b/ghost/core/core/server/models/member-click-event.js @@ -21,7 +21,10 @@ const MemberClickEvent = ghostBookshelf.Model.extend({ return expansions; }, - filterRelations() { + filterRelations(options) { + if (options && options.filterRelations === false) { + return {}; + } return { link: { // Mongo-knex doesn't support belongsTo relations @@ -47,7 +50,7 @@ const MemberClickEvent = ghostBookshelf.Model.extend({ permittedOptions(methodName) { let options = ghostBookshelf.Model.permittedOptions.call(this, methodName); const validOptions = { - findPage: ['selectRaw', 'whereRaw'] + findPage: ['selectRaw', 'whereRaw', 'cte', 'from', 'useCTE', 'filterRelations'] }; if (validOptions[methodName]) { diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/activity-feed.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/activity-feed.test.js.snap index 02842b5783..558a7444a1 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/activity-feed.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/activity-feed.test.js.snap @@ -23567,6 +23567,102 @@ Object { } `; +exports[`Activity Feed API Returns aggregated_click events in activity feed 1: [body] 1`] = ` +Object { + "events": Array [ + Object { + "data": Any, + "type": "aggregated_click_event", + }, + Object { + "data": Any, + "type": "aggregated_click_event", + }, + Object { + "data": Any, + "type": "aggregated_click_event", + }, + Object { + "data": Any, + "type": "aggregated_click_event", + }, + Object { + "data": Any, + "type": "aggregated_click_event", + }, + Object { + "data": Any, + "type": "aggregated_click_event", + }, + Object { + "data": Any, + "type": "aggregated_click_event", + }, + Object { + "data": Any, + "type": "aggregated_click_event", + }, + ], + "meta": Object { + "pagination": Object { + "limit": 10, + "next": null, + "page": null, + "pages": 1, + "prev": null, + "total": 8, + }, + }, +} +`; + +exports[`Activity Feed API Returns aggregated_click events in activity feed 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "2443", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Activity Feed API Returns aggregated_click events in activity feed with post_id filter 1: [body] 1`] = ` +Object { + "events": Array [ + Object { + "data": Any, + "type": "aggregated_click_event", + }, + ], + "meta": Object { + "pagination": Object { + "limit": 10, + "next": null, + "page": null, + "pages": 1, + "prev": null, + "total": 1, + }, + }, +} +`; + +exports[`Activity Feed API Returns aggregated_click events in activity feed with post_id filter 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "391", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Activity Feed API Returns click events in activity feed 1: [body] 1`] = ` Object { "events": Array [ diff --git a/ghost/core/test/e2e-api/admin/activity-feed.test.js b/ghost/core/test/e2e-api/admin/activity-feed.test.js index 7824001a0c..897a895b6e 100644 --- a/ghost/core/test/e2e-api/admin/activity-feed.test.js +++ b/ghost/core/test/e2e-api/admin/activity-feed.test.js @@ -319,6 +319,48 @@ describe('Activity Feed API', function () { }); }); + it('Returns aggregated_click events in activity feed', async function () { + // Check activity feed + await agent + .get(`/members/events?filter=type:aggregated_click_event`) + .expectStatus(200) + .matchHeaderSnapshot({ + etag: anyEtag, + 'content-version': anyContentVersion + }) + .matchBodySnapshot({ + events: new Array(8).fill({ + type: 'aggregated_click_event', + data: anyObject + }) + }) + .expect(({body}) => { + assert(body.events.find(e => e.type === 'aggregated_click_event'), 'Expected a aggregated_click event'); + assert(!body.events.find(e => e.type !== 'aggregated_click_event'), 'Expected only aggregated_click events'); + }); + }); + + it('Returns aggregated_click events in activity feed with post_id filter', async function () { + const postId = fixtureManager.get('posts', 0).id; + await agent + .get(`/members/events?filter=type:aggregated_click_event${encodeURIComponent(`+data.post_id:'${postId}'`)}`) + .expectStatus(200) + .matchHeaderSnapshot({ + etag: anyEtag, + 'content-version': anyContentVersion + }) + .matchBodySnapshot({ + events: new Array(1).fill({ + type: 'aggregated_click_event', + data: anyObject + }) + }) + .expect(({body}) => { + assert(body.events.find(e => e.type === 'aggregated_click_event'), 'Expected a aggregated_click event'); + assert(!body.events.find(e => e.type !== 'aggregated_click_event'), 'Expected only aggregated_click events'); + }); + }); + it('Returns signup events in activity feed', async function () { // Check activity feed await agent diff --git a/ghost/members-api/lib/repositories/EventRepository.js b/ghost/members-api/lib/repositories/EventRepository.js index 7bf1f8fc10..3fd41fc739 100644 --- a/ghost/members-api/lib/repositories/EventRepository.js +++ b/ghost/members-api/lib/repositories/EventRepository.js @@ -1,7 +1,7 @@ const errors = require('@tryghost/errors'); const nql = require('@tryghost/nql'); const mingo = require('mingo'); -const {replaceFilters, expandFilters, splitFilter, getUsedKeys, chainTransformers, mapKeys} = require('@tryghost/mongo-utils'); +const {replaceFilters, expandFilters, splitFilter, getUsedKeys, chainTransformers, mapKeys, rejectStatements} = require('@tryghost/mongo-utils'); /** * This mongo transformer ignores the provided filter option and replaces the filter with a custom filter that was provided to the transformer. Allowing us to set a mongo filter instead of a string based NQL filter. @@ -489,23 +489,66 @@ module.exports = class EventRepository { * This groups click events per member for the same post, and only returns the first actual event, and includes the total clicks per event (for the same member and post) */ async getAggregatedClickEvents(options = {}, filter) { - // This counts all clicks for a member for the same post - const postClickQuery = `SELECT count(distinct A.redirect_id) - FROM members_click_events A - LEFT JOIN redirects A_r on A_r.id = A.redirect_id - LEFT JOIN redirects B_r on B_r.id = members_click_events.redirect_id - WHERE A.member_id = members_click_events.member_id AND A_r.post_id = B_r.post_id`; + let postId = ''; - // Counts all clicks for the same member, for the same post, but only preceding events. This should be zero to include the event (so we only include the first events) - const postClickQueryPreceding = `SELECT count(distinct A.redirect_id) - FROM members_click_events A - LEFT JOIN redirects A_r on A_r.id = A.redirect_id - LEFT JOIN redirects B_r on B_r.id = members_click_events.redirect_id - WHERE A.member_id = members_click_events.member_id AND A_r.post_id = B_r.post_id AND (A.created_at < members_click_events.created_at OR (A.created_at = members_click_events.created_at AND A.id < members_click_events.id))`; + if (filter && filter.$and) { + // Case when there is an $and condition + postId = filter.$and.find(condition => condition['data.post_id'])?.['data.post_id']; + } else { + // Case when there's no $and condition, directly look for data.post_id + postId = filter ? filter['data.post_id'] : ''; + } + //Remove type filter as we don't need it in the query + const [typeFilter, otherFilter] = this.getNQLSubset(options.filter); // eslint-disable-line + + filter = this.removePostIdFilter(otherFilter); //Remove post_id filter as we don't need it in the query + + let postClicksQuery = postId && postId !== '' ? `SELECT + mce.id, + mce.member_id, + mce.redirect_id, + mce.created_at + FROM + members_click_events mce + INNER JOIN + redirects r ON mce.redirect_id = r.id + WHERE + r.post_id = '${postId}' + ` + : `SELECT + mce.id, + mce.member_id, + mce.redirect_id, + mce.created_at + FROM + members_click_events mce + INNER JOIN + redirects r ON mce.redirect_id = r.id + `; + + const firstClicksQuery = ` + SELECT + id, + member_id, + redirect_id, + created_at, + ROW_NUMBER() OVER (PARTITION BY member_id ORDER BY created_at, id) AS rn + FROM + PostClicks + `; + + const mainQuery = `SELECT COUNT(DISTINCT redirect_id) + FROM PostClicks AS inner_mce + WHERE inner_mce.member_id = FirstClicks.member_id + AND inner_mce.redirect_id IN ( + SELECT redirect_id + FROM PostClicks + )`; options = { ...options, withRelated: ['member'], + filterRelations: false, filter: 'custom:true', useBasicCount: true, mongoTransformer: chainTransformers( @@ -519,11 +562,22 @@ module.exports = class EventRepository { 'data.post_id': 'post_id' }) ), + useCTE: true, // We need to use MIN to make pagination work correctly // Note: we cannot do `count(distinct redirect_id) as count__clicks`, because we don't want the created_at filter to affect that count // For pagination to work correctly, we also need to return the id of the first event (or the minimum id if multiple events happend at the same time, but should be the first). Just MIN(id) won't work because that value changes if filter created_at < x is applied. - selectRaw: `id, member_id, created_at, (${postClickQuery}) as count__clicks`, - whereRaw: `(${postClickQueryPreceding}) = 0` + selectRaw: `id, member_id, created_at, (${mainQuery}) as count__clicks`, + whereRaw: `rn = 1 ORDER BY created_at DESC, id DESC`, + cte: [{ + name: `PostClicks`, + query: postClicksQuery + }, + { + name: `FirstClicks`, + query: firstClicksQuery + }], + from: 'FirstClicks', + order: '' }; const {data: models, meta} = await this._MemberLinkClickEvent.findPage(options); @@ -856,6 +910,20 @@ module.exports = class EventRepository { } } + removePostIdFilter(filter) { + if (!filter) { + return filter; + } + + try { + return rejectStatements(filter, key => key === 'data.post_id'); + } catch (e) { + throw new errors.IncorrectUsageError({ + message: e.message + }); + } + } + async getMRR() { const results = await this._MemberPaidSubscriptionEvent.findAll({ aggregateMRRDeltas: true