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

Add simple log processor and log exporter interface #403

Merged
merged 7 commits into from
Dec 4, 2020

Conversation

kxyr
Copy link
Contributor

@kxyr kxyr commented Nov 24, 2020

This PR adds the simple log processor implementation and unit tests, as well as a log exporter interface for #337. The simple processor sends Log Records from the SDK directly to an exporter in a batch of one, and its design currently follows the trace specification. The log exporter interface is used for the simple processor unit tests.

Currently, the processor's Shutdown() function does not forcibly abort if the processor shutdown exceeds the max timeout. This functionality should be added in the future, and similarly for traces' simple span processor as well.

--cc @alolita @MarkSeufert

@kxyr kxyr requested a review from a team November 24, 2020 17:00
@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #403 (570db55) into master (956270e) will decrease coverage by 0.06%.
The diff coverage is 84.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
- Coverage   94.77%   94.70%   -0.07%     
==========================================
  Files         175      179       +4     
  Lines        7591     7666      +75     
==========================================
+ Hits         7194     7260      +66     
- Misses        397      406       +9     
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/logs/processor.h 100.00% <ø> (ø)
sdk/test/logs/logger_provider_sdk_test.cc 84.84% <0.00%> (-5.48%) ⬇️
sdk/test/logs/logger_sdk_test.cc 84.61% <0.00%> (-7.06%) ⬇️
sdk/src/logs/simple_log_processor.cc 86.66% <86.66%> (ø)
sdk/test/logs/simple_log_processor_test.cc 96.22% <96.22%> (ø)
sdk/include/opentelemetry/sdk/logs/exporter.h 100.00% <100.00%> (ø)
...lude/opentelemetry/sdk/logs/simple_log_processor.h 100.00% <100.00%> (ø)
sdk/test/common/circular_buffer_test.cc 98.97% <0.00%> (-1.03%) ⬇️
... and 2 more

sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved
sdk/src/logs/simple_log_processor.cc Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/exporter.h Show resolved Hide resolved
sdk/src/logs/simple_log_processor.cc Outdated Show resolved Hide resolved
sdk/src/logs/simple_log_processor.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

The changes as-is look like a great start with two caveats:

  1. We should open an issue around how Processor => Exporter SDK interface, as we'll want to make sure we can implement something highly efficient with predicable performance implications.
  2. We aren't planning to implement any complicated exporters prior to the first issue getting resolved

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM in current state, assuming we have a follow up ticket to discuss

  • Whether there are use-case to support multiple exporters through single processor

If answer is no, we probably need a PR to transfer the ownership of logrecord to (single) exporter. If yes, we would definitely need a PR to either share ownership through std::shared_ptr or implement recordable interface as done for trace api.

@kxyr
Copy link
Contributor Author

kxyr commented Dec 1, 2020

LGTM in current state, assuming we have a follow up ticket to discuss

  • Whether there are use-case to support multiple exporters through single processor

I believe in logging, it is common to append multiple export destinations to a single Logger (AKA logging "sinks" or "appenders"). For example in these logging libraries, there is the ability to add multiple Appenders: Log4cxx, Plog, and Log4J. Would you say an issue should still be opened up for this?

If answer is no, we probably need a PR to transfer the ownership of logrecord to (single) exporter. If yes, we would definitely need a PR to either share ownership through std::shared_ptr or implement recordable interface as done for trace api.

As per this comment, it would be more feasible to first implement shared_ptr approach as a starting point, which is something we could do in a soon to come PR.

As @jsuereth suggested, we can open up an issue about this the performance implications of this and to add a Recordable as a future TODO as well (see #412).

@lalitb
Copy link
Member

lalitb commented Dec 2, 2020

I believe in logging, it is common to append multiple export destinations to a single Logger (AKA logging "sinks" or "appenders"). For example in these logging libraries, there is the ability to add multiple Appenders: Log4cxx, Plog, and Log4J. Would you say an issue should still be opened up for this?

I totally agree on supporting multiple destinations. Just thinking what should be right way to support them:

logger -> processor1 -> memoryExporter
       -> processor2 -> consoleExporter
       -> processor3 -> fileExpoter

or

logger -> processor   -> memoryExporter
                      -> consoleExporter
                      -> fileExporter

or

mix of both.

If we follow the trace SDK spec, my interpretation is it talks about first approach ( eg, it mentions about processors configured).

Our implementation will change based on which approach we use. Again this is nothing to block current PR, but needs more discussion.

@lalitb
Copy link
Member

lalitb commented Dec 2, 2020

Great, thank you! Should this PR be ready to merge then?

Yes, there are some comments by @pyohannes regarding handling timeout, we can briefly discuss with him in today's community meeting before merging.

@lalitb lalitb requested a review from pyohannes December 2, 2020 19:17
@alolita
Copy link
Member

alolita commented Dec 3, 2020

@pyohannes can you please review? We're blocked on getting this merged. ty!

Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Looks good so far. I saw you created an issue for the revisiting the implementation with a Recordable approach in mind - thanks.

Two things, in addition to my comments below:

  • Please fix the formatting so the CI format job passes. Otherwise this will break other CI jobs after merging.
  • Please submit issues for all TODOs and remove the TODOs.

sdk/include/opentelemetry/sdk/logs/exporter.h Show resolved Hide resolved
sdk/src/logs/simple_log_processor.cc Outdated Show resolved Hide resolved
@kxyr
Copy link
Contributor Author

kxyr commented Dec 3, 2020

Looks good so far. I saw you created an issue for the revisiting the implementation with a Recordable approach in mind - thanks.

Two things, in addition to my comments below:

  • Please fix the formatting so the CI format job passes. Otherwise this will break other CI jobs after merging.
  • Please submit issues for all TODOs and remove the TODOs.

@pyohannes thank you, removed TODOs and fixed formatting.

Related issues: #408, #412, #417, #418, and #420.

@lalitb
Copy link
Member

lalitb commented Dec 4, 2020

@xukaren - Can you update this branch with base, so that it should be ready for merge.

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.

8 participants