Skip to content

Add Ostream log exporter and tests#13

Closed
kxyr wants to merge 8 commits intomasterfrom
logs-ostream-exporter
Closed

Add Ostream log exporter and tests#13
kxyr wants to merge 8 commits intomasterfrom
logs-ostream-exporter

Conversation

@kxyr
Copy link
Copy Markdown

@kxyr kxyr commented Dec 2, 2020

This PR implements an exporter to Ostream for the logging pipeline, as part of Issue # 337.

This PR adds:

  • LogRecord change from a unique_ptr to a shared_ptr when passed from Logger SDK --> processor --> exporter, as proposed as a solution in # 412. This change applies to many test files, processor interfaces and implementations, and across the exporter interface and implementations.
  • A OStream Log Exporter implementation. The OStreamLogExporter takes the batch of LogRecords from the processor, and sends each field to an OStream. The default configuration is to send the data to std::cout.
  • Unit tests for the OStream exporter to cout, cerr, and clog, and an integration test for the full logging pipeline from Logger API --> LoggerSDK --> Simple Log Processor --> OStream Log Exporter

Note that all changes in log_record.h, logger.h, and noop.h in this PR can be ignored, as they are covered by # 422. However, including them here allows them to pass the CI and for this PR to be merged without # 422.

cc @MarkSeufert @alolita

@kxyr kxyr force-pushed the logs-ostream-exporter branch 4 times, most recently from fc3705a to c857b13 Compare December 7, 2020 21:43
@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 7, 2020

Codecov Report

Merging #13 (644a625) into master (c5c82fd) will decrease coverage by 0.03%.
The diff coverage is 91.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   94.53%   94.49%   -0.04%     
==========================================
  Files         179      182       +3     
  Lines        7702     7888     +186     
==========================================
+ Hits         7281     7454     +173     
- Misses        421      434      +13     
Impacted Files Coverage Δ
api/include/opentelemetry/logs/logger.h 100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/logs/exporter.h 100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/logs/processor.h 100.00% <ø> (ø)
...lude/opentelemetry/sdk/logs/simple_log_processor.h 100.00% <ø> (ø)
sdk/test/logs/logger_provider_sdk_test.cc 84.84% <0.00%> (ø)
sdk/test/logs/simple_log_processor_test.cc 96.22% <ø> (ø)
sdk/test/logs/logger_sdk_test.cc 84.61% <50.00%> (ø)
...ude/opentelemetry/exporters/ostream/log_exporter.h 64.70% <64.70%> (ø)
api/include/opentelemetry/logs/log_record.h 100.00% <100.00%> (ø)
exporters/ostream/src/log_exporter.cc 100.00% <100.00%> (ø)
... and 9 more

@kxyr kxyr marked this pull request as ready for review December 7, 2020 22:25
@kxyr kxyr requested a review from alolita December 7, 2020 22:26
@kxyr kxyr force-pushed the logs-ostream-exporter branch 3 times, most recently from c4850ee to 0dfced0 Compare December 8, 2020 05:20
Copy link
Copy Markdown

@alolita alolita left a comment

Choose a reason for hiding this comment

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

Lgtm mostly. Pl add inline comments esp for functions
e.g. in exporters/ostream/src/log_exporter.cc

Comment thread exporters/ostream/include/opentelemetry/exporters/ostream/log_exporter.h Outdated
Comment thread exporters/ostream/src/log_exporter.cc Outdated
Comment thread exporters/ostream/src/log_exporter.cc
Comment thread exporters/ostream/include/opentelemetry/exporters/ostream/log_exporter.h Outdated
@kxyr kxyr force-pushed the logs-ostream-exporter branch 4 times, most recently from 303810c to 542be20 Compare December 8, 2020 21:43
@kxyr kxyr force-pushed the logs-ostream-exporter branch from 5b69bd1 to 644a625 Compare December 8, 2020 22:53
@kxyr kxyr closed this Dec 9, 2020
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.

4 participants