perf: Prevent Sentry from auto-generating spans for requests to Sentry #28613
Conversation
…Integration` and filter spans for requests to 'sentry.io'
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
MajorLift
added
team-tiger
Builds ready [9142f93]
Page Load Metrics (1967 ± 79 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
app/scripts/lib/setupSentry.js
Outdated
| Sentry.browserTracingIntegration({ | ||
| shouldCreateSpanForRequest: (url) => { | ||
| // Do not create spans for outgoing requests to a 'sentry.io' domain. | ||
| return !url.match(/sentry\.io\/?$/u); | ||
| }, | ||
| }), |
There was a problem hiding this comment.
For comparison, this is the code example from the docs:
shouldCreateSpanForRequestThis function can be used to filter out unwanted spans such as XHRs running health checks or something similar. If this function isn't specified, spans will be created for all requests.
Sentry.browserTracingIntegration({ shouldCreateSpanForRequest: (url) => { // Do not create spans for outgoing requests to a `/health/` endpoint return !url.match(/\/health\/?$/); }, }),
There was a problem hiding this comment.
This regex is incorrect. Because it has the $, it will only match URLs that end in sentry.io/
There was a problem hiding this comment.
You're right! This could be fixed by making the regex more specific:
| Sentry.browserTracingIntegration({ | |
| shouldCreateSpanForRequest: (url) => { | |
| // Do not create spans for outgoing requests to a 'sentry.io' domain. | |
| return !url.match(/sentry\.io\/?$/u); | |
| }, | |
| }), | |
| Sentry.browserTracingIntegration({ | |
| shouldCreateSpanForRequest: (url) => { | |
| // Do not create spans for outgoing requests to a 'sentry.io' domain. | |
| return !url.match( | |
| /^https?:\/\/(\w+@)?(\w+\.ingest\.\w{2,3}\.)?sentry\.io(\/api)?/u, | |
| ); | |
| }, | |
| }), |
But I guess a simpler, more permissive regex would be better performance-wise (less backtracking and potentially exponential-time greedy matching).
| Sentry.browserTracingIntegration({ | |
| shouldCreateSpanForRequest: (url) => { | |
| // Do not create spans for outgoing requests to a 'sentry.io' domain. | |
| return !url.match(/sentry\.io\/?$/u); | |
| }, | |
| }), | |
| Sentry.browserTracingIntegration({ | |
| shouldCreateSpanForRequest: (url) => { | |
| // Do not create spans for outgoing requests to a 'sentry.io' domain. | |
| return !url.match(/sentry\.io/u); | |
| }, | |
| }), |
What do you think? The only possible problem I can think of from taking the second approach would be false positives being filtered out, but that's probably not an issue with the Sentry domain.
There was a problem hiding this comment.
I guess the question is... is there some attack possible if someone makes a URL like badplace.com/sentry.io? I guess it would hide this information from us, but we do already have the privacy-snapshot.json for this.
There was a problem hiding this comment.
That's a good point. Maybe we could strike a middle ground by just checking that we're in the right domain?
| Sentry.browserTracingIntegration({ | |
| shouldCreateSpanForRequest: (url) => { | |
| // Do not create spans for outgoing requests to a 'sentry.io' domain. | |
| return !url.match(/sentry\.io\/?$/u); | |
| }, | |
| }), | |
| 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 | |
| ); | |
| }, | |
| }), |
(Not sure if the $ in capture group at the end is correct syntax)
Edit: Looks like it is. https://ingest.us.sentry.ioapi is rejected by https://regex101.com/
Or we could just go more specific, since the security concern seems like a higher priority than performance gains here.
There was a problem hiding this comment.
Yeah I think this one works
Builds ready [0552f10]
Page Load Metrics (2053 ± 102 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
MajorLift
changed the title
shouldCreateSpanForRequest option for Sentry browserTracingIntegration
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
MajorLift
deleted the
jongsun/perf/trace/241121-filter-fetch-request-spans
branch
metamaskbot
added
the
release-12.9.0
Motivation
The Sentry browser tracing integration intercepts all outgoing fetch requests, and measures their duration in automatically-generated
http.clientspans.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
metamaskSentry project, we can make the following observations (the following data only covers requests sent tosentry.io, and doesn't include requests sent to hostnames under that domain e.g.*.ingest.us.sentry.io):/home.htmland/popup.htmltraces, the auto-generated spans for sentry POST requests collectively rank in 3rd place by the "Time Spent" metric, only coming in behind theui.long-taskandpageloadspans, and actually being ahead ofnavigationas well as all other legitimate requests to resource API endpoints.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
shouldCreateSpanForRequestoption.Related issues
Manual testing steps
http.clientspans for 'sentry.io' have been logged.Screenshots/Recordings
Before
Note the three
http.clientspans for 'sentry.io':After
No auto-generated
http.clientspans show up in the Sentry dashboard for POST requests to the Sentry DSN endpoint, even though the requests are still going out.http.clientspans for other URLs are still being created and measured.Pre-merge author checklist
Pre-merge reviewer checklist