From 7a9793f5701f507764a524c13b6fc6b99f36f652 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 27 Nov 2024 12:19:12 -0500 Subject: [PATCH] perf: Prevent Sentry from auto-generating spans for requests to Sentry (#28613) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Motivation The Sentry browser tracing integration intercepts all outgoing fetch requests, and measures their duration in automatically-generated `http.client` spans. Currently, our Sentry configuration does not exclude POST requests to Sentry DSN endpoints from this process. Unfortunately, these requests appear to have been taking up not just a non-negligible, but an outright dominating footprint in our Sentry performance quota and budget. Based on usage data over the past 90 days in our `metamask` Sentry project, we can make the following observations (the following data only covers requests sent to `sentry.io`, and doesn't include requests sent to hostnames under that domain e.g. `*.ingest.us.sentry.io`): - In both the `/home.html` and `/popup.html` traces, the auto-generated spans for sentry POST requests collectively rank in 3rd place by the "Time Spent" metric, only coming in behind the `ui.long-task` and `pageload` spans, and actually being ahead of `navigation` as well as all other legitimate requests to resource API endpoints. Screenshot 2024-11-21 at 8 16 04 AM > https://metamask.sentry.io/performance/summary/spans/?project=273505&query=&statsPeriod=90d&transaction=%2Fpopup.html Screenshot 2024-11-21 at 8 17 20 AM > https://metamask.sentry.io/performance/summary/spans/?project=273505&query=&statsPeriod=90d&transaction=%2Fhome.html - While the average duration of the Sentry spans is around 0.5s, there are instances where they last for much longer (30s - 1m). - An argument could be made that we should leave the Sentry spans to measure these edge cases. However, I think it would be a safe assumption that drastic delays like these will be accompanied by and captured through similar delays in other outgoing network requests. Unless we have reason to target delayed communications that only affect Sentry, filtering out these spans wouldn't result in meaningful information being lost. Screenshot 2024-11-21 at 8 18 37 AM > https://metamask.sentry.io/performance/summary/spans/http.client:8ee04019c21406cc/?project=273505&query=&sort=-span.duration&statsPeriod=90d&transaction=%2Fpopup.html Screenshot 2024-11-21 at 8 18 43 AM > https://metamask.sentry.io/performance/summary/spans/http.client:8ee04019c21406cc/?project=273505&query=&sort=-span.duration&statsPeriod=90d&transaction=%2Fhome.html ## **Description** To fix this, we need to filter out communications with Sentry itself from the monitoring data that is logged to the Sentry dashboard. This commit achieves this by configuring Sentry's browser tracing integration with its `shouldCreateSpanForRequest` option. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28613?quickstart=1) ## **Related issues** - Closes https://github.com/MetaMask/MetaMask-planning/issues/3636 ## **Manual testing steps** 1. Initialize the MetaMask extension with Sentry enabled. 2. [Optional] Take actions that are measured by custom spans (e.g. toggle between account overview tabs, load the account list). This isn't necessary, as communications with Sentry will already have taken place in the first step. 3. Verify in the Sentry dashboard that no recent `http.client` spans for 'sentry.io' have been logged. ## **Screenshots/Recordings** ### **Before** Note the three `http.client` spans for 'sentry.io': Screenshot 2024-11-21 at 8 37 55 AM ### **After** No auto-generated `http.client` spans show up in the Sentry dashboard for POST requests to the Sentry DSN endpoint, even though the requests are still going out. `http.client` spans for other URLs are still being created and measured. Screenshot 2024-11-21 at 7 49 39 AM ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/lib/setupSentry.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index e268d25b2810..72c246826ab5 100644 --- a/app/scripts/lib/setupSentry.js +++ b/app/scripts/lib/setupSentry.js @@ -86,7 +86,12 @@ function getClientOptions() { integrations: [ Sentry.dedupeIntegration(), Sentry.extraErrorDataIntegration(), - Sentry.browserTracingIntegration(), + Sentry.browserTracingIntegration({ + shouldCreateSpanForRequest: (url) => { + // Do not create spans for outgoing requests to a 'sentry.io' domain. + return !url.match(/^https?:\/\/([\w\d.@-]+\.)?sentry\.io(\/|$)/u); + }, + }), filterEvents({ getMetaMetricsEnabled, log }), ], release: RELEASE,