-
Notifications
You must be signed in to change notification settings - Fork 452
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
Use Arc to store reference to LoggerProvider from Logger #1446
Conversation
I wanted to get draft PR out, but I still want to validate that the change doesn't cause any subtle breakage. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1446 +/- ##
=====================================
Coverage 60.8% 60.8%
=====================================
Files 146 146
Lines 19230 19230
=====================================
Hits 11702 11702
Misses 7528 7528 ☔ View full report in Codecov by Sentry. |
As discussed in the community meeting today, we'll first add test coverage, targeted towards shutdown scenarios to have more confidence in this. Also will try this for span as well, where the benefits are even higher. |
…metry-rust into bvoleti/use-arc-in-tracer
Ran the stress tests on my WSL system and didn't get a huge boost: On this branch:
On main:
The interesting thing is, I ran smaller tests on my m2 macbook pro and the difference was much larger. I'll put those numbers up later today. |
From my m2 MBP on the current branch:
vs main
|
Looks like the git history is messed up, will fix it tomorrow. |
@bIgBV It could be the compiler optimizations for Mac and WSL that we are seeing the difference. Can you confirm if you tested this in release mode: <path-to-otel-rust>/stress$ cargo run --bin logs --release |
@lalitb I can confirm that the stress test was done in release mode on both systems. |
Closing this PR in favor of #1455 |
Fixes #
Design discussion issue (if applicable) #1209
Changes
Switch from using a
Weak
reference toLoggerProvider
from within theLogger
to using anArc
.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes