Skip to content

feat(replay): Add more default block filters - #7233

Merged
mydea merged 12 commits into
getsentry:developfrom
lucas-zimerman:fix/blockall-newtypes
Feb 22, 2023
Merged

feat(replay): Add more default block filters#7233
mydea merged 12 commits into
getsentry:developfrom
lucas-zimerman:fix/blockall-newtypes

Conversation

@lucas-zimerman

@lucas-zimermanlucas-zimerman commented Feb 20, 2023

Copy link
Copy Markdown
Contributor

This adds the following block filters:

  • link[rel="icon"] (Browser icon)
  • link[rel="apple-touch-icon"]' (Browser icon)
  • base[href="/"]

The link icons could be removed since they're not visible from the Sentry Replay UI.
The base href is removing the localhost references that causes CSP errors at Sentry as seen here #7184 and here #7183 (comment)

The base[href="/"] filter makes sense for apps that are using Sentry/Javascript since the tools usually adds a localhost reference and that causes the Sentry Website to have CSP errors.

Additionally, I have validated it locally with the Replay Sample/Demo but I am unsure how should I add tests for those items.
Before submitting a pull request, please take a look at our

Maybe in the future, consider adding ways for sibling SDKs to add additional block parameters so that the main Replay doesn't include filters from platform specific code.
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).
    (Yarn test is not passing locally because the machine is running out of memory and there's a test that seems to be time sensitive)

@github-actions

Copy link
Copy Markdown
Contributor

Replay SDK metrics 🚀

Plain+Sentry+Replay
RevisionValueValueDiffRatioValueDiffRatio
LCPThis PR b04a6d492.09 ms131.97 ms+39.88 ms+43.31 %165.95 ms+73.86 ms+80.21 %
Previous 12e34d4120.79 ms144.24 ms+23.45 ms+19.42 %149.36 ms+28.57 ms+23.65 %
CLSThis PR b04a6d40.06 ms0.06 ms-0.00 ms-0.41 %0.06 ms-0.00 ms-0.25 %
Previous 12e34d40.06 ms0.06 ms-0.00 ms-0.10 %0.06 ms+0.00 ms+0.09 %
CPUThis PR b04a6d423.24 %23.59 %+0.35 pp+1.50 %31.96 %+8.73 pp+37.55 %
Previous 12e34d425.72 %25.96 %+0.24 pp+0.93 %31.49 %+5.77 pp+22.42 %
JS heap avgThis PR b04a6d41.94 MB2.05 MB+115.42 kB+5.96 %2.87 MB+932.01 kB+48.14 %
Previous 12e34d41.94 MB2.06 MB+116.91 kB+6.03 %2.87 MB+930.12 kB+47.96 %
JS heap maxThis PR b04a6d42.3 MB2.58 MB+278.5 kB+12.10 %3.36 MB+1.05 MB+45.76 %
Previous 12e34d42.32 MB2.58 MB+259.14 kB+11.17 %3.36 MB+1.04 MB+44.66 %
netTxThis PR b04a6d40 B0 B0 Bn/a2.21 kB+2.21 kBn/a
Previous 12e34d40 B0 B0 Bn/a2.26 kB+2.26 kBn/a
netRxThis PR b04a6d40 B0 B0 Bn/a41 B+41 Bn/a
Previous 12e34d40 B0 B0 Bn/a41 B+41 Bn/a
netCountThis PR b04a6d4000n/a1+1n/a
Previous 12e34d4000n/a1+1n/a
netTimeThis PR b04a6d40.00 ms0.00 ms0.00 msn/a127.27 ms+127.27 msn/a
Previous 12e34d40.00 ms0.00 ms0.00 msn/a109.67 ms+109.67 msn/a

Previous results on branch: develop

RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
12e34d4+28.57 ms+0.00 ms+5.77 pp+930.12 kB+1.04 MB+2.26 kB+41 B+1+109.67 ms
c46c56c+65.45 ms-0.00 ms+5.38 pp+930.26 kB+1.07 MB+2.21 kB+41 B+1+91.29 ms
7f4c4ec+56.64 ms-0.00 ms+5.57 pp+927.42 kB+1.06 MB+2.21 kB+41 B+1+110.83 ms
00d2360+55.18 ms+0.00 ms+2.23 pp+934.14 kB+1.05 MB+2.22 kB+41 B+1+71.65 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Mon, 20 Feb 2023 15:38:22 GMT

Comment threadpackages/replay/src/util/getPrivacyOptions.ts Outdated

@mydeamydea left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice, thanks a lot! 🚀

@mydea

Copy link
Copy Markdown
Member

Ah, some tests are failing - can you take a look at fixing them? if not or if unsure where to look, let me know, then I'll see when I get to it!

@lucas-zimerman

lucas-zimerman commented Feb 21, 2023

Copy link
Copy Markdown
ContributorAuthor

Ah, some tests are failing - can you take a look at fixing them? if not or if unsure where to look, let me know, then I'll see when I get to it!

Not entirely sure if tests will pass (my machine keeps dying during tests 😅) but I fixed some of them (I believe if there's any issue it may be related to the outdated snapshot) If tests fail I'll fix it tomorrow : )

@mydeamydea changed the title fix(replay): Additional block filtersfeat(replay): Additional block filtersFeb 22, 2023
@lucas-zimerman

Copy link
Copy Markdown
ContributorAuthor

@mydea Tests are now fixed : )

@mydeamydea changed the title feat(replay): Additional block filtersfeat(replay): Add more default block filtersFeb 22, 2023
@mydea
mydea merged commit e60cd02 into getsentry:developFeb 22, 2023
@mydea

Copy link
Copy Markdown
Member

Thanks a lot! I promoted this to a feat, as it's actually an improvement. Will go out in the next release - thanks for the contribution!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@lucas-zimerman@mydea