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

Ignore Shutdown in progress when closing ShutdownHookIntegration #2521

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Feb 6, 2023

📜 Description

Ignore "Shutdown in progress" IllegalStateException when removing the shutdown hook. This was also present in older versions of the SDK (https://github.com/getsentry/sentry-java/pull/550/files).

💡 Motivation and Context

Came up again recently in #2465 and also happened in tests before #1574 and #1663

💚 How did you test it?

Unit Test

📝 Checklist

  • I reviewed the submitted code.
  • [x 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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

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

Generated by 🚫 dangerJS against 9dafcf0

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 340.31 ms 383.34 ms 43.03 ms
Size 1.73 MiB 2.33 MiB 623.06 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c1399c1 345.06 ms 385.49 ms 40.43 ms
f6a135d 263.96 ms 383.59 ms 119.63 ms
5fa24ec 326.29 ms 384.53 ms 58.24 ms
14c083a 350.82 ms 388.86 ms 38.04 ms

App size

Revision Plain With Sentry Diff
c1399c1 1.73 MiB 2.33 MiB 620.61 KiB
f6a135d 1.73 MiB 2.33 MiB 623.10 KiB
5fa24ec 1.73 MiB 2.33 MiB 620.61 KiB
14c083a 1.73 MiB 2.33 MiB 620.61 KiB

Previous results on branch: feat/ignore-shutdonw-in-progress

Startup times

Revision Plain With Sentry Diff
7115bd5 361.45 ms 386.82 ms 25.37 ms

App size

Revision Plain With Sentry Diff
7115bd5 1.73 MiB 2.33 MiB 623.07 KiB

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Base: 80.18% // Head: 80.18% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (9dafcf0) compared to base (3e773a7).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2521      +/-   ##
============================================
- Coverage     80.18%   80.18%   -0.01%     
- Complexity     3942     3943       +1     
============================================
  Files           323      323              
  Lines         14891    14896       +5     
  Branches       1964     1965       +1     
============================================
+ Hits          11940    11944       +4     
  Misses         2178     2178              
- Partials        773      774       +1     
Impacted Files Coverage Δ
...c/main/java/io/sentry/ShutdownHookIntegration.java 95.45% <83.33%> (-4.55%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adinauer adinauer merged commit 754021c into main Feb 7, 2023
@adinauer adinauer deleted the feat/ignore-shutdonw-in-progress branch February 7, 2023 15:43
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