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 warning that spammed logs when called from display link callbacks #3284

Merged
merged 3 commits into from
Sep 18, 2023

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Sep 15, 2023

This method is now called from every display link callback and if there's no profiler running it emits large amounts of logs.

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #3284 (23b23d2) into main (881a955) will increase coverage by 0.013%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3284       +/-   ##
=============================================
+ Coverage   89.223%   89.236%   +0.013%     
=============================================
  Files          501       502        +1     
  Lines        53949     54266      +317     
  Branches     19170     19489      +319     
=============================================
+ Hits         48135     48425      +290     
- Misses        4958      4982       +24     
- Partials       856       859        +3     
Files Changed Coverage
Sources/Sentry/SentryProfiler.mm ø

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 881a955...23b23d2. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Sep 15, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1201.69 ms 1221.28 ms 19.59 ms
Size 22.85 KiB 407.63 KiB 384.78 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
be2977c 1243.94 ms 1258.59 ms 14.65 ms
e71cf92 1201.69 ms 1226.52 ms 24.83 ms
c0c1496 1201.19 ms 1228.36 ms 27.17 ms
b9d59f7 1222.23 ms 1227.16 ms 4.93 ms
bd2afa6 1192.31 ms 1210.37 ms 18.05 ms
8636ef0 1239.96 ms 1253.14 ms 13.18 ms
aeec206 1229.27 ms 1253.70 ms 24.43 ms
4be2b3c 1220.47 ms 1223.88 ms 3.41 ms
7c37d8e 1256.00 ms 1259.36 ms 3.36 ms
d760c3f 1200.95 ms 1233.96 ms 33.00 ms

App size

Revision Plain With Sentry Diff
be2977c 22.85 KiB 407.67 KiB 384.82 KiB
e71cf92 20.76 KiB 419.85 KiB 399.10 KiB
c0c1496 22.85 KiB 407.45 KiB 384.60 KiB
b9d59f7 22.85 KiB 405.77 KiB 382.92 KiB
bd2afa6 20.76 KiB 420.55 KiB 399.79 KiB
8636ef0 22.84 KiB 403.18 KiB 380.34 KiB
aeec206 20.76 KiB 434.88 KiB 414.12 KiB
4be2b3c 20.76 KiB 393.37 KiB 372.61 KiB
7c37d8e 20.76 KiB 426.86 KiB 406.09 KiB
d760c3f 22.84 KiB 403.17 KiB 380.33 KiB

Previous results on branch: armcknight/fix/remove-logspam

Startup times

Revision Plain With Sentry Diff
7d786c5 1230.45 ms 1260.94 ms 30.49 ms
9dfc6c3 1263.70 ms 1280.39 ms 16.69 ms

App size

Revision Plain With Sentry Diff
7d786c5 22.85 KiB 407.41 KiB 384.56 KiB
9dfc6c3 22.85 KiB 407.41 KiB 384.56 KiB

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 db31083 into main Sep 18, 2023
28 of 29 checks passed
@armcknight armcknight deleted the armcknight/fix/remove-logspam branch September 18, 2023 18:38
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- remove warning that spammed logs when called from display link callbacks ([#3284](https://github.com/getsentry/sentry-cocoa/pull/3284))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 9cab488

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