From 3d39b35f46bf8482a9b48caac06e8caba46933d9 Mon Sep 17 00:00:00 2001 From: Joe Grigg Date: Thu, 25 Jan 2024 15:16:48 +0000 Subject: [PATCH] Added Sentry instrumentation for get helpers (#19576) no issue - To help debug ABORTED_GET_HELPER errors, this PR adds Sentry instrumentation to the get helpers - It also adds the homepage, any pages/posts, the tag page, and the author page to the list of transactions that will send to Sentry --- ghost/core/core/frontend/helpers/get.js | 60 ++++++++++++++-------- ghost/core/core/shared/sentry.js | 25 +++++++++ ghost/core/test/unit/shared/sentry.test.js | 26 ++++++++++ 3 files changed, 89 insertions(+), 22 deletions(-) diff --git a/ghost/core/core/frontend/helpers/get.js b/ghost/core/core/frontend/helpers/get.js index e682f3bd3a..2f01045448 100644 --- a/ghost/core/core/frontend/helpers/get.js +++ b/ghost/core/core/frontend/helpers/get.js @@ -7,6 +7,7 @@ const {hbs} = require('../services/handlebars'); const logging = require('@tryghost/logging'); const errors = require('@tryghost/errors'); const tpl = require('@tryghost/tpl'); +const Sentry = require('@sentry/node'); const _ = require('lodash'); const jsonpath = require('jsonpath'); @@ -206,32 +207,47 @@ module.exports = async function get(resource, options) { // Parse the options we're going to pass to the API apiOptions = parseOptions(ghostGlobals, this, apiOptions); + let apiOptionsString = Object.entries(apiOptions) + .map(([key, value]) => ` ${key}="${value}"`) + .join(''); apiOptions.context = {member: data.member}; - try { - const response = await makeAPICall(resource, controllerName, action, apiOptions); + const spanName = `{{#get "${resource}"${apiOptionsString}}} ${data.member ? 'member' : 'public'}`; + const result = await Sentry.startSpan({ + op: 'frontend.helpers.get', + name: spanName, + tags: { + resource, + ...apiOptions, + context: data.member ? 'member' : 'public' + } + }, async (span) => { + const response = await makeAPICall(resource, controllerName, action, apiOptions); - // prepare data properties for use with handlebars - if (response[resource] && response[resource].length) { - response[resource].forEach(prepareContextResource); - } - - // used for logging details of slow requests - returnedRowsCount = response[resource] && response[resource].length; - - // block params allows the theme developer to name the data using something like - // `{{#get "posts" as |result pageInfo|}}` - const blockParams = [response[resource]]; - if (response.meta && response.meta.pagination) { - response.pagination = response.meta.pagination; - blockParams.push(response.meta.pagination); - } - - // Call the main template function - return options.fn(response, { - data: data, - blockParams: blockParams + // prepare data properties for use with handlebars + if (response[resource] && response[resource].length) { + response[resource].forEach(prepareContextResource); + } + + // used for logging details of slow requests + returnedRowsCount = response[resource] && response[resource].length; + span?.setTag('returnedRows', returnedRowsCount); + + // block params allows the theme developer to name the data using something like + // `{{#get "posts" as |result pageInfo|}}` + const blockParams = [response[resource]]; + if (response.meta && response.meta.pagination) { + response.pagination = response.meta.pagination; + blockParams.push(response.meta.pagination); + } + + // Call the main template function + return options.fn(response, { + data: data, + blockParams: blockParams + }); }); + return result; } catch (error) { logging.error(error); data.error = error.message; diff --git a/ghost/core/core/shared/sentry.js b/ghost/core/core/shared/sentry.js index abf2ad27eb..8f86b16996 100644 --- a/ghost/core/core/shared/sentry.js +++ b/ghost/core/core/shared/sentry.js @@ -57,6 +57,31 @@ const beforeSend = function (event, hint) { } }; +const ALLOWED_HTTP_TRANSACTIONS = [ + '/ghost/api', // any Ghost API call + '/members/api', // any Members API call + '/:slug', // any frontend post/page + '/author', // any frontend author page + '/tag' // any frontend tag page +].map((path) => { + // Sentry names HTTP transactions like: " " i.e. "GET /ghost/api/content/settings" + // Match any of the paths above with any HTTP method, and also the homepage "GET /" + return new RegExp(`^(GET|POST|PUT|DELETE)\\s(?${path}\\/.+|\\/$)`); +}); + +const beforeSendTransaction = function (event) { + // Drop transactions that are not in the allowed list + for (const transaction of ALLOWED_HTTP_TRANSACTIONS) { + const match = event.transaction.match(transaction); + + if (match?.groups?.path) { + return event; + } + } + + return null; +}; + if (sentryConfig && !sentryConfig.disabled) { const Sentry = require('@sentry/node'); const version = require('@tryghost/version').full; diff --git a/ghost/core/test/unit/shared/sentry.test.js b/ghost/core/test/unit/shared/sentry.test.js index 05047685a0..8b69e6d774 100644 --- a/ghost/core/test/unit/shared/sentry.test.js +++ b/ghost/core/test/unit/shared/sentry.test.js @@ -155,4 +155,30 @@ describe('UNIT: sentry', function () { assert.deepEqual(result, expected); }); }); + + describe('beforeSendTransaction', function () { + it('filters transactions based on an allow list', function () { + sentry = require('../../../core/shared/sentry'); + + const beforeSendTransaction = sentry. beforeSendTransaction; + + const allowedTransactions = [ + {transaction: 'GET /ghost/api/settings'}, + {transaction: 'PUT /members/api/member'}, + {transaction: 'POST /ghost/api/tiers'}, + {transaction: 'DELETE /members/api/member'}, + {transaction: 'GET /'}, + {transaction: 'GET /:slug/options(edit)?/'}, + {transaction: 'GET /author/:slug'}, + {transaction: 'GET /tag/:slug'} + ]; + + allowedTransactions.forEach((transaction) => { + assert.equal(beforeSendTransaction(transaction), transaction); + }); + + assert.equal(beforeSendTransaction({transaction: 'GET /foo/bar'}), null); + assert.equal(beforeSendTransaction({transaction: 'Some other transaction'}), null); + }); + }); });