Skip to content

Commit

Permalink
perf: Prevent Sentry from auto-generating spans for requests to Sentry (
Browse files Browse the repository at this point in the history
#28613)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## 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.

<img width="1789" alt="Screenshot 2024-11-21 at 8 16 04 AM"
src="https://github.com/user-attachments/assets/b3f58f43-fa17-4731-9333-d2bcf271a8cc">

>
https://metamask.sentry.io/performance/summary/spans/?project=273505&query=&statsPeriod=90d&transaction=%2Fpopup.html

<img width="1789" alt="Screenshot 2024-11-21 at 8 17 20 AM"
src="https://github.com/user-attachments/assets/74b4b590-045e-45f2-a18d-feafbe54d212">

>
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.

<img width="1789" alt="Screenshot 2024-11-21 at 8 18 37 AM"
src="https://github.com/user-attachments/assets/42be31ce-9721-42c5-9b2a-de168d22bd6d">

>
https://metamask.sentry.io/performance/summary/spans/http.client:8ee04019c21406cc/?project=273505&query=&sort=-span.duration&statsPeriod=90d&transaction=%2Fpopup.html

<img width="1789" alt="Screenshot 2024-11-21 at 8 18 43 AM"
src="https://github.com/user-attachments/assets/33bbd0f6-e620-4f21-8ba9-09eef903f444">

>
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 MetaMask/MetaMask-planning#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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

Note the three `http.client` spans for 'sentry.io':

<img width="1217" alt="Screenshot 2024-11-21 at 8 37 55 AM"
src="https://github.com/user-attachments/assets/6fedfe60-c356-4dec-a91d-998dce5080f5">

### **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.

<img width="1214" alt="Screenshot 2024-11-21 at 7 49 39 AM"
src="https://github.com/user-attachments/assets/10e64131-22ec-4872-982d-17404ac4b120">

## **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.
  • Loading branch information
MajorLift authored Nov 27, 2024
1 parent f249b8c commit 7a9793f
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion app/scripts/lib/setupSentry.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 7a9793f

Please sign in to comment.