-
Notifications
You must be signed in to change notification settings - Fork 158
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
Fix logrus logger hook data race condition #181
Conversation
Codecov Report
@@ Coverage Diff @@
## main #181 +/- ##
==========================================
+ Coverage 88.58% 88.78% +0.20%
==========================================
Files 18 18
Lines 981 981
==========================================
+ Hits 869 871 +2
+ Misses 78 77 -1
+ Partials 34 33 -1
Continue to review full report at Codecov.
|
I don't believe this resolves the noticed issue: #166 (comment) If you merge that branch and run |
0e0f721
to
44e15a0
Compare
@rmfitzpatrick you are correct. Nonetheless it looks like this problem is fixed in logrus v1.8.1 (see v1.8.1 here in contrast to v1.8.0 here). I cleanup a few things and added tests. Can you please review those changes? |
@rmfitzpatrick I forgot to mention that we are using v1.8.0. |
@bjsignalfx would just bumping the logrus version be able to resolve on its own? |
@rmfitzpatrick yes that resolves the data race condition. But, below is an excerpt of the error we are getting now:
|
@rmfitzpatrick Then again, maybe not. I ran the test again and I'm not getting the error I mentioned above. I got the result below.
|
Data race issue #166 (comment) fixed by updating logrus to version v1.8.1. Plus refactoring.