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: rename SentryTimerWrapper to SentryTimerFactory #3120

Merged

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Jun 24, 2023

since it's no longer a subclass after the recent fixes, it's now a factory. Noticed as I made the change in #3119

#skip-changelog

@github-actions
Copy link

github-actions bot commented Jun 24, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1214.96 ms 1221.28 ms 6.32 ms
Size 20.76 KiB 401.60 KiB 380.84 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6e342ac 1220.24 ms 1240.14 ms 19.90 ms
49819af 1263.92 ms 1275.66 ms 11.74 ms
46f5eb8 1212.27 ms 1231.42 ms 19.15 ms
c319795 1205.12 ms 1231.20 ms 26.08 ms
f587451 1271.63 ms 1275.90 ms 4.27 ms
fcde045 1260.71 ms 1275.00 ms 14.29 ms
d3edf46 1213.00 ms 1227.46 ms 14.46 ms
e84bc3f 1196.62 ms 1218.86 ms 22.24 ms
1db04d8 1250.20 ms 1258.12 ms 7.92 ms
8e76be4 1272.67 ms 1286.38 ms 13.71 ms

App size

Revision Plain With Sentry Diff
6e342ac 20.76 KiB 436.66 KiB 415.90 KiB
49819af 20.76 KiB 427.31 KiB 406.55 KiB
46f5eb8 20.76 KiB 432.37 KiB 411.61 KiB
c319795 20.76 KiB 431.99 KiB 411.22 KiB
f587451 20.76 KiB 435.25 KiB 414.49 KiB
fcde045 20.76 KiB 435.26 KiB 414.50 KiB
d3edf46 20.76 KiB 436.65 KiB 415.89 KiB
e84bc3f 20.76 KiB 434.72 KiB 413.96 KiB
1db04d8 20.76 KiB 435.50 KiB 414.74 KiB
8e76be4 20.76 KiB 427.66 KiB 406.89 KiB

Previous results on branch: armcknight/ref/rename-sentry-nstimer-wrapper-to-factory

Startup times

Revision Plain With Sentry Diff
928cca5 1231.31 ms 1247.20 ms 15.89 ms
9999991 1241.84 ms 1264.20 ms 22.36 ms

App size

Revision Plain With Sentry Diff
928cca5 20.76 KiB 401.56 KiB 380.80 KiB
9999991 20.76 KiB 401.53 KiB 380.77 KiB

Comment on lines -1 to -6
#import "SentryNSTimerWrapper.h"

@interface
SentryNSTimerWrapper ()

@end
Copy link
Member Author

Choose a reason for hiding this comment

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

This file never had any declarations, just leftover.

Base automatically changed from armcknight/ref/framestracker to main June 26, 2023 23:52
since it's no longer a subclass after the recent fixes, it's now a factory
@armcknight armcknight force-pushed the armcknight/ref/rename-sentry-nstimer-wrapper-to-factory branch from a7ec7f2 to cca9220 Compare June 27, 2023 00:03
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #3120 (fdde4df) into main (1d11695) will increase coverage by 0.008%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3120       +/-   ##
=============================================
+ Coverage   89.007%   89.015%   +0.008%     
=============================================
  Files          500       500               
  Lines        53926     53902       -24     
  Branches     19296     19287        -9     
=============================================
- Hits         47998     47981       -17     
- Misses        4962      5064      +102     
+ Partials       966       857      -109     
Impacted Files Coverage Δ
Sources/Sentry/SentryHub.m 98.454% <ø> (ø)
Sources/Sentry/SentryMetricProfiler.mm 94.957% <ø> (ø)
Sources/Sentry/SentryNSTimerFactory.m 100.000% <ø> (ø)
Sources/Sentry/SentryDependencyContainer.m 94.392% <100.000%> (ø)
Sources/Sentry/SentryProfiler.mm 84.318% <100.000%> (ø)
Sources/Sentry/SentryTracer.m 96.082% <100.000%> (ø)
...SentryProfilerTests/SentryNSTimerFactoryTest.swift 100.000% <100.000%> (ø)
...SentryProfilerTests/SentryProfilerSwiftTests.swift 97.033% <100.000%> (ø)
...ts/SentryTests/Transaction/SentryTracerTests.swift 99.607% <100.000%> (ø)

... and 27 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 1d11695...fdde4df. Read the comment docs.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@armcknight armcknight merged commit e8b14db into main Jun 27, 2023
@armcknight armcknight deleted the armcknight/ref/rename-sentry-nstimer-wrapper-to-factory branch June 27, 2023 08:41
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