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: move SentryExtraContextProvider singleton to SentryDependencyContainer #3244

Conversation

armcknight
Copy link
Member

Another singleton that was passed in a few inits, that should be just accessed via the dependency container.

#skip-changelog

@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #3244 (db033de) into main (4b1a58e) will increase coverage by 0.030%.
The diff coverage is 93.750%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3244       +/-   ##
=============================================
+ Coverage   89.177%   89.207%   +0.030%     
=============================================
  Files          502       502               
  Lines        54210     54250       +40     
  Branches     19462     19476       +14     
=============================================
+ Hits         48343     48395       +52     
+ Misses        5002      4994        -8     
+ Partials       865       861        -4     
Files Changed Coverage
Sources/Sentry/PrivateSentrySDKOnly.mm 0.000%
Sources/Sentry/SentryExtraContextProvider.m ø
Sources/Sentry/SentryClient.m 100.000%
Sources/Sentry/SentryDependencyContainer.m 100.000%
Tests/SentryTests/SentryClientTests.swift 100.000%

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 4b1a58e...db033de. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Aug 19, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1275.57 ms 1293.06 ms 17.49 ms
Size 22.85 KiB 406.95 KiB 384.10 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
89d72e7 1222.60 ms 1252.78 ms 30.18 ms
24b4df1 1240.98 ms 1253.98 ms 13.00 ms
7c5d161 1224.38 ms 1249.66 ms 25.28 ms
efb0147 1245.26 ms 1266.94 ms 21.68 ms
89b12eb 1236.02 ms 1246.63 ms 10.61 ms
4259afd 1222.12 ms 1249.74 ms 27.62 ms
fdfe96b 1227.90 ms 1242.56 ms 14.66 ms
98a8c16 1206.40 ms 1232.14 ms 25.74 ms
d760c3f 1228.58 ms 1246.22 ms 17.64 ms
7cd187e 1204.78 ms 1235.86 ms 31.08 ms

App size

Revision Plain With Sentry Diff
89d72e7 20.76 KiB 431.87 KiB 411.11 KiB
24b4df1 20.76 KiB 425.77 KiB 405.01 KiB
7c5d161 20.76 KiB 414.44 KiB 393.68 KiB
efb0147 22.84 KiB 403.52 KiB 380.67 KiB
89b12eb 20.76 KiB 432.88 KiB 412.11 KiB
4259afd 20.76 KiB 419.70 KiB 398.94 KiB
fdfe96b 20.76 KiB 419.70 KiB 398.95 KiB
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
d760c3f 22.84 KiB 403.17 KiB 380.33 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB

Previous results on branch: armcknight/ref/centralize-SentryExtraContextProvider-singleton

Startup times

Revision Plain With Sentry Diff
c68ec87 1232.06 ms 1262.98 ms 30.92 ms

App size

Revision Plain With Sentry Diff
c68ec87 22.85 KiB 405.44 KiB 382.59 KiB

@brustolin
Copy link
Contributor

Something broke CI

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 1e66f98 into main Sep 5, 2023
66 checks passed
@armcknight armcknight deleted the armcknight/ref/centralize-SentryExtraContextProvider-singleton branch September 5, 2023 19:47
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