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

impr: Remove not needed log for logging #3934

Merged
merged 3 commits into from
May 3, 2024
Merged

Conversation

philipphofmann
Copy link
Member

📜 Description

Every log message needs to acquire a lock to evaluate whether the logger should log it. #3303 added this logic to fix a data race the thread sanitizer job found. As the SDK only calls the configure method when starting, we don't need to put a synchronized keyword around the code that evaluates the log level. This PR replaces the synchronized keyword by ignoring the thread sanitizer.

💡 Motivation and Context

This came up while investigating failing unit tests for #3915. The raw log messages show a 500ms delay in the SentryHttpTransport between two calls that only log a simple message: SENTRY_LOG_DEBUG(@"No internet connection."); and SENTRY_LOG_DEBUG(@"Finished sending.");

SENTRY_LOG_DEBUG(@"No internet connection.");
[weakSelf finishedSending];
}
}];
}
- (void)finishedSending
{
SENTRY_LOG_DEBUG(@"Finished sending.");
@synchronized(self) {

2024-05-03 10:16:56.674070+0000 xctest[10854:44346] [Sentry] [debug] [SentryHttpTransport:359] No internet connection.
2024-05-03 10:16:57.214727+0000 xctest[10854:44346] [Sentry] [debug] [SentryHttpTransport:367] Finished sending.

I don't think the not needed lock is responsible for this, but removing it should improve overall performance of logging.

💚 How did you test it?

Logging still works.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Every log message needs to acquire a lock to evaluate whether the logger
should log it. #3303 added this logic to fix a data race the thread
sanitizer job found. As the SDK only calls the configure method when
starting, we don't need to put a synchronized keyword around the code
that evaluates the log level. This PR replaces the synchronized keyword
by ignoring the thread sanitizer.
Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.686%. Comparing base (ca507ec) to head (4ffedc0).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3934       +/-   ##
=============================================
- Coverage   90.686%   89.686%   -1.000%     
=============================================
  Files          582       581        -1     
  Lines        45524     45241      -283     
  Branches     16218     15839      -379     
=============================================
- Hits         41284     40575      -709     
- Misses        4170      4596      +426     
  Partials        70        70               
Files Coverage Δ
Sources/Sentry/SentryLog.m 100.000% <100.000%> (ø)

... and 94 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 ca507ec...4ffedc0. Read the comment docs.

@philipphofmann philipphofmann merged commit 32ac934 into main May 3, 2024
66 of 70 checks passed
@philipphofmann philipphofmann deleted the impr/logger-lock branch May 3, 2024 12:50
Copy link

github-actions bot commented May 3, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.00 ms 1244.79 ms 18.79 ms
Size 21.58 KiB 616.72 KiB 595.14 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7fb7afb 1202.18 ms 1219.42 ms 17.24 ms
a9103fe 1221.49 ms 1243.33 ms 21.84 ms
626b91b 1231.67 ms 1247.13 ms 15.46 ms
2cf1b84 6265.25 ms 6277.98 ms 12.73 ms
2b19b82 1226.73 ms 1243.27 ms 16.53 ms
160a2a7 1219.29 ms 1240.96 ms 21.67 ms
a71f5e2 1245.27 ms 1258.00 ms 12.73 ms
4a947cf 1230.73 ms 1239.38 ms 8.65 ms
965db8a 1211.61 ms 1226.60 ms 14.99 ms
5b3518e 1242.27 ms 1257.81 ms 15.55 ms

App size

Revision Plain With Sentry Diff
7fb7afb 20.76 KiB 419.70 KiB 398.94 KiB
a9103fe 20.76 KiB 426.95 KiB 406.19 KiB
626b91b 21.58 KiB 546.20 KiB 524.61 KiB
2cf1b84 20.76 KiB 431.91 KiB 411.15 KiB
2b19b82 21.58 KiB 542.18 KiB 520.60 KiB
160a2a7 21.58 KiB 614.86 KiB 593.28 KiB
a71f5e2 21.58 KiB 424.34 KiB 402.76 KiB
4a947cf 20.76 KiB 399.69 KiB 378.92 KiB
965db8a 22.84 KiB 403.24 KiB 380.39 KiB
5b3518e 21.58 KiB 612.11 KiB 590.53 KiB

philipphofmann added a commit that referenced this pull request May 6, 2024
Every log message needs to acquire a lock to evaluate whether the logger
should log it. #3303 added this logic to fix a data race the thread
sanitizer job found. As the SDK only calls the configure method when
starting, we don't need to put a synchronized keyword around the code
that evaluates the log level. This PR replaces the synchronized keyword
by ignoring the thread sanitizer.
dKasabwala pushed a commit to dKasabwala/sentry-cocoa that referenced this pull request May 6, 2024
Every log message needs to acquire a lock to evaluate whether the logger
should log it. getsentry#3303 added this logic to fix a data race the thread
sanitizer job found. As the SDK only calls the configure method when
starting, we don't need to put a synchronized keyword around the code
that evaluates the log level. This PR replaces the synchronized keyword
by ignoring the thread sanitizer.
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
Every log message needs to acquire a lock to evaluate whether the logger
should log it. getsentry#3303 added this logic to fix a data race the thread
sanitizer job found. As the SDK only calls the configure method when
starting, we don't need to put a synchronized keyword around the code
that evaluates the log level. This PR replaces the synchronized keyword
by ignoring the thread sanitizer.
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

3 participants