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

ref: Send frames for all spans #3478

Merged
merged 1 commit into from Dec 5, 2023
Merged

Conversation

philipphofmann
Copy link
Member

Initially, we wanted to send frame data only for spans started on the main thread, but we decided that the frame data makes sense for all spans. This PR fixes that.

Initially, we wanted to send frame data only for spans started on the
main thread, but we decided that the frame data makes sense for all
spans. This PR fixes that.
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Merging #3478 (47ae280) into main (ef5821b) will decrease coverage by 0.035%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3478       +/-   ##
=============================================
- Coverage   89.025%   88.991%   -0.035%     
=============================================
  Files          525       525               
  Lines        56898     56891        -7     
  Branches     20452     20455        +3     
=============================================
- Hits         50654     50628       -26     
- Misses        5322      5343       +21     
+ Partials       922       920        -2     
Files Coverage Δ
Sources/Sentry/SentrySpan.m 98.395% <100.000%> (ø)
...ests/SentryTests/Transaction/SentrySpanTests.swift 99.404% <100.000%> (-0.002%) ⬇️

... and 19 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 ef5821b...47ae280. Read the comment docs.

Copy link

github-actions bot commented Dec 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1210.27 ms 1227.16 ms 16.89 ms
Size 21.58 KiB 414.92 KiB 393.34 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8f397a7 1251.82 ms 1268.34 ms 16.52 ms
53a8885 1241.02 ms 1270.66 ms 29.64 ms
5f8ee7a 1253.96 ms 1264.98 ms 11.02 ms
154f795 1225.53 ms 1231.04 ms 5.51 ms
e998fd0 1241.49 ms 1262.63 ms 21.14 ms
05644fe 1244.04 ms 1244.46 ms 0.42 ms
c0ff306 1219.24 ms 1243.96 ms 24.72 ms
7192d9e 1221.86 ms 1248.28 ms 26.42 ms
ef5821b 1253.18 ms 1265.46 ms 12.28 ms
b16d18c 1245.60 ms 1250.94 ms 5.34 ms

App size

Revision Plain With Sentry Diff
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
53a8885 20.76 KiB 434.65 KiB 413.89 KiB
5f8ee7a 22.85 KiB 411.93 KiB 389.08 KiB
154f795 20.76 KiB 435.25 KiB 414.49 KiB
e998fd0 21.58 KiB 414.59 KiB 393.01 KiB
05644fe 22.85 KiB 408.88 KiB 386.03 KiB
c0ff306 20.76 KiB 434.65 KiB 413.89 KiB
7192d9e 20.76 KiB 431.71 KiB 410.95 KiB
ef5821b 21.58 KiB 414.96 KiB 393.37 KiB
b16d18c 20.76 KiB 437.12 KiB 416.36 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, with one threading question

} else {
_data[SPAN_DATA_THREAD_NAME] = [SentryDependencyContainer.sharedInstance.threadInspector
getThreadName:currentThread];
}

#if SENTRY_HAS_UIKIT
_framesTracker = framesTracker;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need any kind of synchronization now that it's multithreaded?

Copy link
Member

Choose a reason for hiding this comment

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

@philipphofmann please look into this and open a subsequent PR if necessary.

@philipphofmann philipphofmann merged commit 20f4f5d into main Dec 5, 2023
68 of 70 checks passed
@philipphofmann philipphofmann deleted the ref/send-frames-for-all-spans branch December 5, 2023 22:31
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