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

feat: Add request start date to network breadcrumb #4008

Merged
merged 4 commits into from
May 24, 2024

Conversation

brustolin
Copy link
Contributor

📜 Description

💡 Motivation and Context

Needed for session replay breadcrumb (#4002).

💚 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

github-actions bot commented May 23, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against db6e85a

Copy link

github-actions bot commented May 23, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.866%. Comparing base (cf97209) to head (db6e85a).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4008       +/-   ##
=============================================
+ Coverage   90.844%   90.866%   +0.021%     
=============================================
  Files          593       593               
  Lines        46519     46564       +45     
  Branches     16633     16642        +9     
=============================================
+ Hits         42260     42311       +51     
+ Misses        4076      4072        -4     
+ Partials       183       181        -2     
Files Coverage Δ
Sources/Sentry/SentryNetworkTracker.m 96.451% <100.000%> (+0.058%) ⬆️
...erformance/Network/SentryNetworkTrackerTests.swift 98.493% <100.000%> (+0.004%) ⬆️

... and 8 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 cf97209...db6e85a. Read the comment docs.

Copy link

github-actions bot commented May 23, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

github-actions bot commented May 23, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1228.08 ms 1246.90 ms 18.82 ms
Size 21.58 KiB 632.04 KiB 610.46 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
dd4145f 1233.48 ms 1254.35 ms 20.88 ms
3cba0e8 1250.86 ms 1258.39 ms 7.53 ms
69d8759 1235.71 ms 1241.34 ms 5.63 ms
dcec216 1238.94 ms 1261.06 ms 22.12 ms
aa45f36 1226.39 ms 1253.22 ms 26.84 ms
7bb0873 1215.65 ms 1235.00 ms 19.35 ms
7fe37ab 1228.92 ms 1243.86 ms 14.94 ms
cd39d58 1203.87 ms 1239.88 ms 36.01 ms
11b2ffa 1204.86 ms 1218.16 ms 13.31 ms
c021422 1237.12 ms 1263.18 ms 26.06 ms

App size

Revision Plain With Sentry Diff
dd4145f 21.58 KiB 540.09 KiB 518.51 KiB
3cba0e8 22.84 KiB 403.19 KiB 380.34 KiB
69d8759 20.76 KiB 393.05 KiB 372.29 KiB
dcec216 20.76 KiB 432.88 KiB 412.11 KiB
aa45f36 21.58 KiB 616.73 KiB 595.15 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
7fe37ab 21.58 KiB 542.28 KiB 520.70 KiB
cd39d58 20.76 KiB 435.26 KiB 414.50 KiB
11b2ffa 22.85 KiB 412.67 KiB 389.82 KiB
c021422 20.76 KiB 435.64 KiB 414.88 KiB

Previous results on branch: feat/network-breadcrumb-start

Startup times

Revision Plain With Sentry Diff
e930c26 1220.94 ms 1238.76 ms 17.82 ms

App size

Revision Plain With Sentry Diff
e930c26 21.58 KiB 632.13 KiB 610.55 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, with two suggestions. Thanks for putting this in an extra PR.

Copy link

github-actions bot commented May 24, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

@brustolin brustolin merged commit 4ae9b7a into main May 24, 2024
67 of 70 checks passed
@brustolin brustolin deleted the feat/network-breadcrumb-start branch May 24, 2024 14:23
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.

None yet

2 participants