-
Notifications
You must be signed in to change notification settings - Fork 411
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
Add batch log processor implementation with test coverage #434
Conversation
Codecov Report
@@ Coverage Diff @@
## master #434 +/- ##
==========================================
+ Coverage 94.37% 94.43% +0.06%
==========================================
Files 185 187 +2
Lines 8047 8234 +187
==========================================
+ Hits 7594 7776 +182
- Misses 453 458 +5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. Few minor changes would be needed in this PR ( as mentioned in comments) once Recordable
interface PR is reviewed and merged. As I understand, @xukaren is working on that PR. We can hold this PR for merging till then.
5ba22ce
to
f642a60
Compare
a55d098
to
13bc179
Compare
Replace while loop with cv.wait with predicate Remove while loop around cv.notify Replace operator= with .store() for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the concurrency portions of this may need some help.
Given the use of locks + condition for the core of this class, may make sense to just use a real lock/mutex instead of Atomics for now.
I have reverted this log processor to the same one as the batch span processor. This way there won't be any concurrency issues if this PR is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping this the same as the Trace batch processor (for now) is definitely the right call. Threading logic looks good.
Logged a separate issue to make this code readable, and/or avoiding the while looks on waiting for variable update: |
This PR adds a batch log processor for the Logging Prototype, based off the batch span processor, and following the simple log processor. The batch log processor has a main thread that is responsible for batching the
LogRecord
s, and a worker thread that sends these records to an exporter. Note:std::cerr
messages are included here as a placeholder before error diagnostics is available.cc @MarkSeufert @alolita