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

Create timer in TransactionPerformanceCollector lazily #2478

Merged
merged 3 commits into from
Jan 17, 2023

Conversation

adinauer
Copy link
Member

📜 Description

Timer was created even if performance wasn't enabled. Also Timer prevented shutdown.

💡 Motivation and Context

Fixes #2477

💚 How did you test it?

📝 Checklist

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

🔮 Next steps

  • @stefanosiano can you please help test if this still works correctly

@adinauer adinauer changed the title Create timer in TransactionPerformanceCollector lazily Create timer in TransactionPerformanceCollector lazily Jan 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 17, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 322.08 ms 380.45 ms 58.37 ms
Size 1.73 MiB 2.33 MiB 619.47 KiB

Previous results on branch: fix/create-performance-collector-timer-lazily

Startup times

Revision Plain With Sentry Diff
bec6aae 307.14 ms 344.22 ms 37.08 ms

App size

Revision Plain With Sentry Diff
bec6aae 1.73 MiB 2.33 MiB 619.47 KiB

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

jeez, here we are again with the Timer stuff :D can we write an integration test that checks for shutdown of a backend app somehow?

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 80.14% // Head: 80.13% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (adb859b) compared to base (734e4a5).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2478      +/-   ##
============================================
- Coverage     80.14%   80.13%   -0.01%     
- Complexity     3872     3873       +1     
============================================
  Files           312      312              
  Lines         14669    14672       +3     
  Branches       1941     1943       +2     
============================================
+ Hits          11756    11758       +2     
  Misses         2153     2153              
- Partials        760      761       +1     
Impacted Files Coverage Δ
...ava/io/sentry/TransactionPerformanceCollector.java 89.79% <83.33%> (-1.51%) ⬇️

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 34431f8 into main Jan 17, 2023
@adinauer adinauer deleted the fix/create-performance-collector-timer-lazily branch January 17, 2023 10:27
@adinauer
Copy link
Member Author

can we write an integration test that checks for shutdown of a backend app somehow?

Yeah we should. Created #2479 so we don't forget.

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.

Sentry 6.12.0 causes Spring Boot Web application cannot be stopped
3 participants