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: Remove SentrySystemEventBreadcrumbs observers with the most specific detail possible #2489

Merged
merged 9 commits into from
Dec 6, 2022

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Dec 5, 2022

📜 Description

Same as we did in the other files. Maybe this fixes #2486, although I am not sure. Doesn't hurt either way.

💡 Motivation and Context

Could maybe fix #2486.

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1224.62 ms 1239.62 ms 15.00 ms
Size 20.75 KiB 404.84 KiB 384.08 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
a9e77dc 1231.94 ms 1254.85 ms 22.91 ms
010583c 1198.23 ms 1225.52 ms 27.29 ms
88ac2c2 1223.04 ms 1243.12 ms 20.08 ms
bbe81cb 1257.25 ms 1272.24 ms 14.99 ms
57e140e 1223.94 ms 1252.50 ms 28.56 ms
6ef7dc2 1204.14 ms 1253.76 ms 49.62 ms
323cdfe 1263.20 ms 1272.14 ms 8.94 ms
5ed4bee 1226.02 ms 1247.50 ms 21.48 ms
3936dd2 1214.98 ms 1242.24 ms 27.26 ms
d446105 1237.06 ms 1261.34 ms 24.28 ms

App size

Revision Plain With Sentry Diff
a9e77dc 20.75 KiB 379.12 KiB 358.36 KiB
010583c 20.75 KiB 383.61 KiB 362.85 KiB
88ac2c2 20.75 KiB 373.61 KiB 352.86 KiB
bbe81cb 20.75 KiB 381.81 KiB 361.06 KiB
57e140e 20.75 KiB 383.61 KiB 362.85 KiB
6ef7dc2 20.75 KiB 383.39 KiB 362.64 KiB
323cdfe 20.75 KiB 383.50 KiB 362.75 KiB
5ed4bee 20.75 KiB 404.63 KiB 383.88 KiB
3936dd2 20.75 KiB 383.29 KiB 362.53 KiB
d446105 20.75 KiB 383.37 KiB 362.62 KiB

Previous results on branch: fix/SentrySystemEventBreadcrumbs-observers

Startup times

Revision Plain With Sentry Diff
4e6cc98 1220.40 ms 1250.84 ms 30.44 ms
27c2055 1215.96 ms 1238.96 ms 23.00 ms
a5be394 1223.38 ms 1249.92 ms 26.54 ms
b112ed8 1208.61 ms 1240.58 ms 31.97 ms

App size

Revision Plain With Sentry Diff
4e6cc98 20.75 KiB 383.70 KiB 362.95 KiB
27c2055 20.75 KiB 383.71 KiB 362.95 KiB
a5be394 20.75 KiB 383.78 KiB 363.03 KiB
b112ed8 20.75 KiB 383.78 KiB 363.03 KiB

@kevinrenskers
Copy link
Contributor Author

I ran brew upgrade today and now I got a bunch of code formatting changes, that were then overwritten again by CI. How can we make sure that we all + CI are on the same version? I think the problem is SwiftLint: I am running version 0.50.1 but the version in Brewfile.lock.json is 0.49.1.

Then again the version of clang-format is locked to 15.0.4 yet the Format Code workflow installs 15.0.6, so I don't understand how that works 🤔

@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #2489 (7eeb4e4) into 8.0.0 (57e140e) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 7eeb4e4 differs from pull request most recent head 74af8b2. Consider uploading reports for the commit 74af8b2 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##           8.0.0   #2489    +/-   ##
======================================
  Coverage   0.00%   0.00%            
======================================
  Files        239     242     +3     
  Lines      22235   22431   +196     
  Branches    9133    9189    +56     
======================================
- Misses     21965   22426   +461     
+ Partials     270       5   -265     
Impacted Files Coverage Δ
...s/Sentry/SentryAutoBreadcrumbTrackingIntegration.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryNSNotificationCenterWrapper.m 0.00% <0.00%> (ø)
Sources/Sentry/SentrySystemEventBreadcrumbs.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryDsn.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryHub.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryUser.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryTime.mm 0.00% <0.00%> (ø)
Sources/Sentry/SentryClient.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryTracer.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryBaggage.m 0.00% <0.00%> (ø)
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57e140e...74af8b2. 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

@kevinrenskers kevinrenskers merged commit fded9f8 into 8.0.0 Dec 6, 2022
@kevinrenskers kevinrenskers deleted the fix/SentrySystemEventBreadcrumbs-observers branch December 6, 2022 10:58
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.

Crash when changing timeZone
5 participants