Add batch log processor implementation with test coverage by kxyr · Pull Request #434 · open-telemetry/opentelemetry-cpp

kxyr · 2020-12-09T22:18:36Z

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 LogRecords, 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

codecov · 2020-12-09T22:27:37Z

Codecov Report

Merging #434 (5a507bb) into master (b80dccb) will increase coverage by 0.06%.
The diff coverage is 97.32%.

@@            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

Impacted Files	Coverage Δ
sdk/src/logs/batch_log_processor.cc	`93.82% <93.82%> (ø)`
sdk/test/logs/batch_log_processor_test.cc	`100.00% <100.00%> (ø)`

lalitb

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.

Replace while loop with cv.wait with predicate Remove while loop around cv.notify Replace operator= with .store() for clarity

jsuereth

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.

kxyr · 2020-12-18T17:57:07Z

@jsuereth, please let me know what you think of these changes! :)
@maxgolov would love a final stamp as well!

kxyr · 2020-12-19T01:55:51Z

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.

jsuereth

I think keeping this the same as the Trace batch processor (for now) is definitely the right call. Threading logic looks good.

…to logs-sdk3

maxgolov · 2020-12-21T23:34:52Z

Logged a separate issue to make this code readable, and/or avoiding the while looks on waiting for variable update:
#477

…etry#434)

…and-patch-dependencies Update dependency rules_go to v0.56.1

kxyr requested a review from a team December 9, 2020 22:18

lalitb reviewed Dec 10, 2020

View reviewed changes

kxyr mentioned this pull request Dec 10, 2020

Add a Recordable interface, implementation, and tests for logs #438

Merged

maxgolov reviewed Dec 11, 2020

View reviewed changes

Comment thread sdk/src/logs/batch_log_processor.cc Outdated

maxgolov reviewed Dec 11, 2020

View reviewed changes

Comment thread sdk/src/logs/batch_log_processor.cc Outdated

MarkSeufert mentioned this pull request Dec 13, 2020

Added Logging SDK Context Injection and Logger Limit #435

Closed

kxyr force-pushed the logs-sdk3 branch 2 times, most recently from 5ba22ce to f642a60 Compare December 14, 2020 20:10

kxyr changed the base branch from master to pr-merge December 14, 2020 20:16

kxyr changed the base branch from pr-merge to master December 14, 2020 20:16

kxyr force-pushed the logs-sdk3 branch 2 times, most recently from a55d098 to 13bc179 Compare December 14, 2020 20:31

Karen Xu added 3 commits December 14, 2020 15:33

Add batch log processor implementation with test coverage

04c434e

Rename

175280b

Remove error printing

977e3fc

kxyr force-pushed the logs-sdk3 branch from 13bc179 to a688bb4 Compare December 14, 2020 20:34

Update with Recordable

30f9731

kxyr force-pushed the logs-sdk3 branch from a688bb4 to 30f9731 Compare December 14, 2020 20:39

Karen Xu added 2 commits December 15, 2020 11:45

Add lock to Shutdown(),

14033b5

Replace while loop with cv.wait with predicate Remove while loop around cv.notify Replace operator= with .store() for clarity

Atomic exchange for shutdown

227ee38

kxyr force-pushed the logs-sdk3 branch from 4935060 to 227ee38 Compare December 15, 2020 20:27

jsuereth requested changes Dec 16, 2020

View reviewed changes

Comment thread sdk/src/logs/batch_log_processor.cc

Comment thread sdk/src/logs/batch_log_processor.cc Outdated

Comment thread sdk/test/logs/batch_log_processor_test.cc Outdated

ThomsonTan reviewed Dec 17, 2020

View reviewed changes

Comment thread sdk/include/opentelemetry/sdk/logs/batch_log_processor.h Outdated

Karen Xu added 2 commits December 16, 2020 19:48

Revert to using lock for Shutdown(), virtual override dtor, no if

b3a1f2f

Remove thread sleep, simplify DoBackgroundWork(), reorg

49039a1

kxyr force-pushed the logs-sdk3 branch from cb8ddd5 to 49039a1 Compare December 17, 2020 03:59

Rename typo (scheduled_delay_millis) to match spec

bc3a1af

kxyr force-pushed the logs-sdk3 branch from 1f4a21e to bc3a1af Compare December 17, 2020 04:25

ThomsonTan reviewed Dec 18, 2020

View reviewed changes

Comment thread sdk/src/logs/batch_log_processor.cc Outdated

Revert to batch span processor implementation

1204bdc

Merge branch 'master' into logs-sdk3

09812d9

jsuereth approved these changes Dec 19, 2020

View reviewed changes

Merge branch 'master' into logs-sdk3

4e5fee4

ThomsonTan reviewed Dec 20, 2020

View reviewed changes

Comment thread sdk/test/logs/batch_log_processor_test.cc

ThomsonTan approved these changes Dec 20, 2020

View reviewed changes

Karen Xu added 3 commits December 20, 2020 16:31

Fix tests

24e7463

Merge branch 'logs-sdk3' of github.com:open-o11y/opentelemetry-cpp in…

a44c938

…to logs-sdk3

Format

dfb2bba

lalitb requested a review from maxgolov December 21, 2020 19:21

Merge branch 'master' into logs-sdk3

5a507bb

maxgolov reviewed Dec 21, 2020

View reviewed changes

Comment thread sdk/src/logs/batch_log_processor.cc

maxgolov reviewed Dec 21, 2020

View reviewed changes

Comment thread sdk/src/logs/batch_log_processor.cc

maxgolov approved these changes Dec 21, 2020

View reviewed changes

maxgolov added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Dec 21, 2020

lalitb merged commit 5e946f9 into open-telemetry:master Dec 22, 2020

kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 22, 2020

Add batch log processor implementation with test coverage (open-telem…

53726ca

…etry#434)

GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Mar 6, 2026

Merge pull request open-telemetry#434 from malkia/renovate/all-minor-…

e8b2a65

…and-patch-dependencies Update dependency rules_go to v0.56.1

Uh oh!

Conversation

kxyr commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsuereth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kxyr commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kxyr commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsuereth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxgolov commented Dec 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kxyr commented Dec 9, 2020 •

edited

Loading

codecov Bot commented Dec 9, 2020 •

edited

Loading

kxyr commented Dec 18, 2020 •

edited

Loading

kxyr commented Dec 19, 2020 •

edited

Loading