Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimised getLastSeenEventTimestamp function #20134

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// For information on writing migrations, see https://www.notion.so/ghost/Database-migrations-eb5b78c435d741d2b34a582d57c24253

const {createAddColumnMigration} = require('../../utils');

module.exports = createAddColumnMigration('emails', 'latest_event_timestamp', {
type: 'dateTime', nullable: true
});
3 changes: 2 additions & 1 deletion ghost/core/core/server/data/schema/schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,8 @@ module.exports = {
created_at: {type: 'dateTime', nullable: false},
created_by: {type: 'string', maxlength: 24, nullable: false},
updated_at: {type: 'dateTime', nullable: true},
updated_by: {type: 'string', maxlength: 24, nullable: true}
updated_by: {type: 'string', maxlength: 24, nullable: true},
latest_event_timestamp: {type: 'dateTime', nullable: true}
},
email_batches: {
id: {type: 'string', maxlength: 24, nullable: false, primary: true},
Expand Down
3 changes: 2 additions & 1 deletion ghost/core/core/server/models/email.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const Email = ghostBookshelf.Model.extend({
delivered_count: 0,
opened_count: 0,
failed_count: 0,
source_type: 'html'
source_type: 'html',
latest_event_timestamp: null
};
},

Expand Down
49 changes: 27 additions & 22 deletions ghost/core/core/server/services/email-analytics/lib/queries.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const _ = require('lodash');
const debug = require('@tryghost/debug')('services:email-analytics');
const db = require('../../../data/db');

Expand All @@ -14,30 +13,36 @@ module.exports = {
async getLastSeenEventTimestamp() {
const startDate = new Date();

// three separate queries is much faster than using max/greatest (with coalesce to handle nulls) across columns
let {maxDeliveredAt} = await db.knex('email_recipients').select(db.knex.raw('MAX(delivered_at) as maxDeliveredAt')).first() || {};
let {maxOpenedAt} = await db.knex('email_recipients').select(db.knex.raw('MAX(opened_at) as maxOpenedAt')).first() || {};
let {maxFailedAt} = await db.knex('email_recipients').select(db.knex.raw('MAX(failed_at) as maxFailedAt')).first() || {};

if (maxDeliveredAt && !(maxDeliveredAt instanceof Date)) {
// SQLite returns a string instead of a Date
maxDeliveredAt = new Date(maxDeliveredAt);
}

if (maxOpenedAt && !(maxOpenedAt instanceof Date)) {
// SQLite returns a string instead of a Date
maxOpenedAt = new Date(maxOpenedAt);
// First, try to fetch the max latest_event_timestamp
let {maxTimestamp} = await db.knex('emails').select(db.knex.raw('MAX(latest_event_timestamp) as maxTimestamp')).first() || {};

if (maxTimestamp) {
if (!(maxTimestamp instanceof Date)) {
maxTimestamp = new Date(maxTimestamp);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth explaining why we need to convert to a Date here?

// SQLite returns a string instead of a Date

} else {
let {maxDeliveredAt} = await db.knex('email_recipients').select(db.knex.raw('MAX(delivered_at) as maxDeliveredAt')).first() || {};
let {maxOpenedAt} = await db.knex('email_recipients').select(db.knex.raw('MAX(opened_at) as maxOpenedAt')).first() || {};
let {maxFailedAt} = await db.knex('email_recipients').select(db.knex.raw('MAX(failed_at) as maxFailedAt')).first() || {};

if (maxDeliveredAt && !(maxDeliveredAt instanceof Date)) {
// SQLite returns a string instead of a Date
maxDeliveredAt = new Date(maxDeliveredAt);
}

if (maxOpenedAt && !(maxOpenedAt instanceof Date)) {
// SQLite returns a string instead of a Date
maxOpenedAt = new Date(maxOpenedAt);
}

if (maxFailedAt && !(maxFailedAt instanceof Date)) {
// SQLite returns a string instead of a Date
maxFailedAt = new Date(maxFailedAt);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing we're keeping the old logic to smoothen the rollout, as existing emails won't have the latest_event_timestamp field. Shall we make a note to clean this up in a couple of weeks from now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also return early here, to separate the new logic from the old one, e.g.

if (maxTimestamp) {
    if (!(maxTimestamp instanceof Date)) {
        maxTimestamp = new Date(maxTimestamp);
     }
              
     return maxTimestamp;
}


// Legacy logic

...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes correct, I'll add a note in the comments.


if (maxFailedAt && !(maxFailedAt instanceof Date)) {
// SQLite returns a string instead of a Date
maxFailedAt = new Date(maxFailedAt);
}

const lastSeenEventTimestamp = _.max([maxDeliveredAt, maxOpenedAt, maxFailedAt]);
debug(`getLastSeenEventTimestamp: finished in ${Date.now() - startDate}ms`);

return lastSeenEventTimestamp;
return maxTimestamp;
},

async aggregateEmailStats(emailId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22699,7 +22699,7 @@ exports[`Activity Feed API Can filter events by post id 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": "17559",
"content-length": "17739",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand Down Expand Up @@ -23852,7 +23852,7 @@ exports[`Activity Feed API Returns email delivered events in activity feed 2: [h
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": "1051",
"content-length": "1081",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand Down Expand Up @@ -23886,7 +23886,7 @@ exports[`Activity Feed API Returns email opened events in activity feed 2: [head
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": "1048",
"content-length": "1078",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand Down Expand Up @@ -23944,7 +23944,7 @@ exports[`Activity Feed API Returns email sent events in activity feed 2: [header
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": "3860",
"content-length": "3980",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand Down
13 changes: 9 additions & 4 deletions ghost/core/test/e2e-api/admin/__snapshots__/emails.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,7 @@ Object {
"from": null,
"html": "<p>Look! I'm an email</p>",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"latest_event_timestamp": null,
"newsletter_id": null,
"opened_count": 1,
"plaintext": "Waba-daba-dab-da",
Expand Down Expand Up @@ -526,6 +527,7 @@ Object {
"from": null,
"html": "<p>What's that? Another email!</p>",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"latest_event_timestamp": null,
"newsletter_id": null,
"opened_count": 0,
"plaintext": "yes this is an email",
Expand Down Expand Up @@ -560,7 +562,7 @@ exports[`Emails API Can browse emails 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": "1405",
"content-length": "1465",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand Down Expand Up @@ -698,6 +700,7 @@ Object {
"from": null,
"html": "<p>Look! I'm an email</p>",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"latest_event_timestamp": null,
"newsletter_id": null,
"opened_count": 1,
"plaintext": "Waba-daba-dab-da",
Expand All @@ -722,7 +725,7 @@ exports[`Emails API Can read an email 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": "642",
"content-length": "672",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand All @@ -745,6 +748,7 @@ Object {
"from": null,
"html": "<p>What's that? Another email!</p>",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"latest_event_timestamp": null,
"newsletter_id": null,
"opened_count": 0,
"plaintext": "yes this is an email",
Expand All @@ -769,7 +773,7 @@ exports[`Emails API Can retry a failed email 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": "688",
"content-length": "718",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand All @@ -792,6 +796,7 @@ Object {
"from": "[email protected]",
"html": "<p style=\\"margin: 0 0 1.5em 0; line-height: 1.6em;\\">Hey Jamie, Hey Jamie,</p><a href=\\"http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=preview\\">Unsubscribe</a>",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"latest_event_timestamp": null,
"newsletter_id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"opened_count": 0,
"plaintext": "Hey Jamie, Hey Jamie
Expand All @@ -817,7 +822,7 @@ exports[`Emails API Does default replacements on the HTML body of an old email 2
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": "923",
"content-length": "953",
"content-type": "application/json; charset=utf-8",
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
Expand Down
62 changes: 62 additions & 0 deletions ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -3217,6 +3217,68 @@ Object {
}
`;

exports[`Members API Can filter by conversion attribution 1: [body] 1`] = `
Object {
"members": Array [
Object {
"avatar_image": null,
"comped": false,
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"email": "[email protected]",
"email_count": 0,
"email_open_rate": null,
"email_opened_count": 0,
"email_suppression": Object {
"info": null,
"suppressed": false,
},
"geolocation": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"labels": Any<Array>,
"last_seen_at": null,
"name": "Mr Egg",
"newsletters": Array [
Object {
"description": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"name": "Daily newsletter",
"status": "active",
},
],
"note": null,
"status": "free",
"subscribed": true,
"subscriptions": Any<Array>,
"updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
},
],
"meta": Object {
"pagination": Object {
"limit": 15,
"next": null,
"page": 1,
"pages": 1,
"prev": null,
"total": 1,
},
},
}
`;

exports[`Members API Can filter by conversion attribution 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": "830",
"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[`Members API Can filter by paid status 1: [body] 1`] = `
Object {
"members": Array [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1135,4 +1135,107 @@ describe('EmailEventStorage', function () {
const result = await emailAnalytics.fetchLatest();
assert.equal(result, 0);
});

it('populates latest_event_timestamp in email when handleDelivered fires', async function () {
const emailBatch = fixtureManager.get('email_batches', 0);
const emailId = emailBatch.email_id;

const emailRecipient = fixtureManager.get('email_recipients', 0);
assert(emailRecipient.batch_id === emailBatch.id);
const providerId = emailBatch.provider_id;
const timestamp = new Date(2000, 0, 1);

events = [{
event: 'delivered',
recipient: emailRecipient.member_email,
'user-variables': {
'email-id': emailId
},
message: {
headers: {
'message-id': providerId
}
},
// unix timestamp
timestamp: Math.round(timestamp.getTime() / 1000)
}];

const result = await emailAnalytics.fetchLatest();
assert.equal(result, 1);

await DomainEvents.allSettled();

const updatedEmail = await models.Email.findOne({
id: emailId
}, {require: true});

assert.equal(updatedEmail.get('latest_event_timestamp').toUTCString(), timestamp.toUTCString());
});

it('populates latest_event_timestamp in email when handleOpened fires', async function () {
const emailBatch = fixtureManager.get('email_batches', 0);
const emailId = emailBatch.email_id;

const emailRecipient = fixtureManager.get('email_recipients', 0);
assert(emailRecipient.batch_id === emailBatch.id);
const providerId = emailBatch.provider_id;
const timestamp = new Date(2000, 0, 1);

events = [{
event: 'opened',
recipient: emailRecipient.member_email,
'user-variables': {
'email-id': emailId
},
message: {
headers: {
'message-id': providerId
}
},
// unix timestamp
timestamp: Math.round(timestamp.getTime() / 1000)
}];

const result = await emailAnalytics.fetchLatest();
assert.equal(result, 1);

await DomainEvents.allSettled();

const updatedEmail = await models.Email.findOne({
id: emailId
}, {require: true});

assert.equal(updatedEmail.get('latest_event_timestamp').toUTCString(), timestamp.toUTCString());
});

it('populates latest_event_timestamp in email when handlePermanentFailed fires', async function () {
const emailBatch = fixtureManager.get('email_batches', 0);
const emailId = emailBatch.email_id;

const emailRecipient = fixtureManager.get('email_recipients', 0);
assert(emailRecipient.batch_id === emailBatch.id);
const timestamp = new Date(2000, 0, 1);

events = [{
event: 'failed',
severity: 'permanent',
recipient: emailRecipient.member_email,
'user-variables': {
'email-id': emailId
},
// unix timestamp
timestamp: Math.round(timestamp.getTime() / 1000)
}];

const result = await emailAnalytics.fetchLatest();
assert.equal(result, 1);

await DomainEvents.allSettled();

const updatedEmail = await models.Email.findOne({
id: emailId
}, {require: true});

assert.equal(updatedEmail.get('latest_event_timestamp').toUTCString(), timestamp.toUTCString());
});
});
2 changes: 1 addition & 1 deletion ghost/core/test/unit/server/data/schema/integrity.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const validateRouteSettings = require('../../../../../core/server/services/route
*/
describe('DB version integrity', function () {
// Only these variables should need updating
const currentSchemaHash = 'ccf3893bc3f8930f0d1188e646abda6d';
const currentSchemaHash = '7495ca1ca1127247a2638a19c0c59b8e';
const currentFixturesHash = 'a489d615989eab1023d4b8af0ecee7fd';
const currentSettingsHash = '5c957ceb48c4878767d7d3db484c592d';
const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01';
Expand Down