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

feat: Disable SIGTERM reporting by default #4025

Merged
merged 12 commits into from
Jun 3, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

We added support for SIGTERM reporting in the last release and enabled it by default. For some users, SIGTERM events were verbose and not actionable. Therefore, we disable it per default.

💡 Motivation and Context

Fixes GH-4013

💚 How did you test it?

Options via unit test. The signal handling installation manually with log messages cause this code isn't testable at all at the moment, and I don't think we should change that by adding a simple boolean check.

📝 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

Copy link

github-actions bot commented May 31, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 918f61a

Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.093%. Comparing base (7d72ee1) to head (918f61a).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4025       +/-   ##
=============================================
+ Coverage   91.061%   91.093%   +0.031%     
=============================================
  Files          607       607               
  Lines        47425     47449       +24     
  Branches     17008     17036       +28     
=============================================
+ Hits         43186     43223       +37     
+ Misses        4169      4156       -13     
  Partials        70        70               
Files Coverage Δ
Sources/Sentry/SentryCrashIntegration.m 98.734% <100.000%> (+0.104%) ⬆️
Sources/Sentry/SentryOptions.m 99.220% <100.000%> (+0.014%) ⬆️
...ash/Recording/Monitors/SentryCrashMonitor_Signal.c 58.928% <100.000%> (+3.588%) ⬆️
...entryCrash/Recording/Tools/SentryCrashSignalInfo.c 100.000% <ø> (ø)
Tests/SentryTests/SentryOptionsTest.m 98.190% <100.000%> (+0.002%) ⬆️

... and 9 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 7d72ee1...918f61a. Read the comment docs.

We added support for SIGTERM reporting in the last release and enabled
it by default. For some users, SIGTERM events were verbose and not
actionable. Therefore, we disable it per default.

Fixes GH-4013
@philipphofmann philipphofmann force-pushed the feat/disable-sigterm-reporting branch from fa756c6 to ede45b3 Compare May 31, 2024 08:23
Copy link

github-actions bot commented May 31, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.83 ms 1261.37 ms 21.55 ms
Size 21.58 KiB 651.07 KiB 629.49 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
83acf3e 1240.06 ms 1261.38 ms 21.31 ms
65290e0 1204.29 ms 1217.00 ms 12.71 ms
8919322 1262.67 ms 1269.90 ms 7.22 ms
4be2b3c 1220.47 ms 1223.88 ms 3.41 ms
4154aff 1225.25 ms 1244.77 ms 19.52 ms
39b1c35 1236.35 ms 1239.90 ms 3.55 ms
d253cdf 1231.61 ms 1259.52 ms 27.91 ms
01a28a9 1245.43 ms 1255.98 ms 10.55 ms
c00eafe 1253.04 ms 1256.41 ms 3.37 ms
2de284c 1234.92 ms 1254.45 ms 19.53 ms

App size

Revision Plain With Sentry Diff
83acf3e 21.58 KiB 539.87 KiB 518.29 KiB
65290e0 21.58 KiB 418.76 KiB 397.18 KiB
8919322 22.84 KiB 403.18 KiB 380.34 KiB
4be2b3c 20.76 KiB 393.37 KiB 372.61 KiB
4154aff 21.58 KiB 424.30 KiB 402.72 KiB
39b1c35 22.85 KiB 408.88 KiB 386.03 KiB
d253cdf 20.76 KiB 427.66 KiB 406.90 KiB
01a28a9 22.85 KiB 405.39 KiB 382.55 KiB
c00eafe 20.76 KiB 432.88 KiB 412.12 KiB
2de284c 21.58 KiB 542.39 KiB 520.80 KiB

Previous results on branch: feat/disable-sigterm-reporting

Startup times

Revision Plain With Sentry Diff
8b1b364 1221.98 ms 1247.61 ms 25.63 ms
79adb1d 1233.24 ms 1253.27 ms 20.02 ms

App size

Revision Plain With Sentry Diff
8b1b364 21.58 KiB 651.07 KiB 629.49 KiB
79adb1d 21.58 KiB 651.08 KiB 629.50 KiB

@philipphofmann philipphofmann merged commit 5eaadc5 into main Jun 3, 2024
67 of 70 checks passed
@philipphofmann philipphofmann deleted the feat/disable-sigterm-reporting branch June 3, 2024 08:27
@eric
Copy link

eric commented Jun 4, 2024

If this is false is a SIGTERM shutdown considered graceful or is it a watchdog event?

@brustolin
Copy link
Contributor

Hello @eric, if enableSigtermReporting is false, no issue related to SIGTERM will be created.

@eric
Copy link

eric commented Jun 5, 2024

So a SIGTERM will not create a watchdog event?

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.

Add the ability to disable SIGTERM reporting
3 participants