Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Too many navigation breadcrumbs for Session Replay #4480

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

brustolin
Copy link
Contributor

📜 Description

We create a navigation breadcrumb for every view controller that appears, but some view controllers are child view controllers and they dont represent a navigation.

Filtering out this child breadcrumbs in the session replay

💡 Motivation and Context

Fixes https://github.com/getsentry/team-replay/issues/467

💚 How did you test it?

Unit tests

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.425%. Comparing base (a90f228) to head (6427720).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4480       +/-   ##
=============================================
+ Coverage   91.410%   91.425%   +0.015%     
=============================================
  Files          615       616        +1     
  Lines        68597     68647       +50     
  Branches     24623     24598       -25     
=============================================
+ Hits         62705     62761       +56     
+ Misses        5800      5794        -6     
  Partials        92        92               
Files with missing lines Coverage Δ
SentryTestUtils/BreadcrumbExtension.swift 100.000% <100.000%> (ø)
...tegrations/SessionReplay/SentrySessionReplay.swift 97.570% <100.000%> (+1.826%) ⬆️
...tions/SessionReplay/SentrySessionReplayTests.swift 98.870% <100.000%> (+0.116%) ⬆️

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a90f228...6427720. Read the comment docs.

Copy link

github-actions bot commented Oct 29, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1231.39 ms 1244.94 ms 13.55 ms
Size 21.90 KiB 724.68 KiB 702.78 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e84bc3f 1196.62 ms 1218.86 ms 22.24 ms
cc31630 1235.22 ms 1252.51 ms 17.29 ms
f576153 1210.02 ms 1228.94 ms 18.92 ms
becc941 1221.90 ms 1240.37 ms 18.47 ms
3cd0156 1216.37 ms 1232.84 ms 16.47 ms
18d491a 1229.78 ms 1252.67 ms 22.90 ms
f8833c4 1229.69 ms 1236.45 ms 6.76 ms
69d2789 1235.73 ms 1254.00 ms 18.27 ms
070db34 1228.64 ms 1240.24 ms 11.61 ms
3a31fc9 1237.35 ms 1249.02 ms 11.67 ms

App size

Revision Plain With Sentry Diff
e84bc3f 20.76 KiB 434.72 KiB 413.96 KiB
cc31630 21.58 KiB 694.58 KiB 672.99 KiB
f576153 20.76 KiB 425.77 KiB 405.01 KiB
becc941 21.58 KiB 419.82 KiB 398.24 KiB
3cd0156 21.58 KiB 706.47 KiB 684.89 KiB
18d491a 21.58 KiB 544.87 KiB 523.29 KiB
f8833c4 21.58 KiB 422.66 KiB 401.08 KiB
69d2789 21.58 KiB 548.09 KiB 526.51 KiB
070db34 21.58 KiB 574.17 KiB 552.59 KiB
3a31fc9 20.76 KiB 414.45 KiB 393.69 KiB

Previous results on branch: fix/sr-filter-navigation-breadcrunb

Startup times

Revision Plain With Sentry Diff
ca6c892 1240.41 ms 1253.92 ms 13.51 ms
40c6acc 1235.47 ms 1251.53 ms 16.06 ms

App size

Revision Plain With Sentry Diff
ca6c892 21.90 KiB 709.56 KiB 687.66 KiB
40c6acc 21.90 KiB 709.49 KiB 687.59 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

@brustolin brustolin enabled auto-merge (squash) October 31, 2024 07:45
@brustolin brustolin merged commit e92acb3 into main Oct 31, 2024
38 of 39 checks passed
@brustolin brustolin deleted the fix/sr-filter-navigation-breadcrunb branch October 31, 2024 07:45
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Too many navigation breadcrumbs for Session Replay ([#4480](https://github.com/getsentry/sentry-cocoa/pull/4480))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 6427720

Sign up for free to 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