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

Added SimpleExporter and BatchExporter for Activity and LogRecord #1622

Merged

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Nov 25, 2020

Changes

  • Modified SimpleExportProcessor and BatchExportProcessor to abstract classes
  • Added SimpleActivityExportProcessor, SimpleLogRecordExportProcessor, BatchActivityExportProcessor, BatchLogRecordExportProcessor
  • Added the check for Activity.Recorded in SimpleActivityExportProcessor and BatchActivityExportProcessor

@utpilla
Copy link
Contributor Author

utpilla commented Nov 25, 2020

@cijothomas @CodeBlanch I have refactored SimpleExportProcessor and BatchExportProcessor to have type specific implementations. This is in reference to #1574 (comment)

I added these four files:

  • SimpleActivityExportProcessor.cs
  • SimpleLogRecordExportProcessor.cs
  • BatchActivityExportProcessor.cs
  • BatchLogRecordExportProcessor.cs

I have updated the SimpleExportProcessor and BatchExportProcessor classes to abstract. The check for Activity.Recorded is in SimpleActivityExportProcessor.cs and BatchActivityExportProcessor.cs.

I would love to know your views on this.

@ejsmith
Copy link
Contributor

ejsmith commented Nov 25, 2020

I'd prefer if the base implementations were just called ActivityExportProcessor and LogRecordExportProcessor instead of using the Simple prefix. In what way are they simple? They are just exporters and then the other ones are the same except that they export in batches.

@utpilla
Copy link
Contributor Author

utpilla commented Nov 25, 2020

I'd prefer if the base implementations were just called ActivityExportProcessor and LogRecordExportProcessor instead of using the Simple prefix. In what way are they simple? They are just exporters and then the other ones are the same except that they export in batches.

@ejsmith We need to have two implementations for Activity. one that implements SimpleExportProcessor and the other that implements BatchExportProcessor. The difference between the two is what you've mentioned but we need to have two different implementations so we need two different class names. I added the prefix Simple and Batch to easily distinguish between them. The same goes for LogRecord.

@CodeBlanch
Copy link
Member

Don't want to approve yet because it's a draft but approach LGTM.

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #1622 (a49d96e) into master (c188edb) will decrease coverage by 0.02%.
The diff coverage is 59.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1622      +/-   ##
==========================================
- Coverage   82.12%   82.09%   -0.03%     
==========================================
  Files         245      249       +4     
  Lines        6706     6730      +24     
==========================================
+ Hits         5507     5525      +18     
- Misses       1199     1205       +6     
Impacted Files Coverage Δ
...xporter.Console/ConsoleExporterHelperExtensions.cs 0.00% <0.00%> (ø)
...porter.Console/ConsoleExporterLoggingExtensions.cs 0.00% <0.00%> (ø)
...rter.InMemory/InMemoryExporterLoggingExtensions.cs 0.00% <0.00%> (ø)
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs 15.38% <0.00%> (ø)
....Exporter.Zipkin/ZipkinExporterHelperExtensions.cs 15.38% <0.00%> (ø)
src/OpenTelemetry/BatchExportProcessor.cs 93.05% <ø> (ø)
src/OpenTelemetry/BatchLogRecordExportProcessor.cs 0.00% <0.00%> (ø)
src/OpenTelemetry/SimpleExportProcessor.cs 70.00% <ø> (ø)
...nTelemetryProtocol/OtlpExporterHelperExtensions.cs 92.30% <50.00%> (ø)
...orter.InMemory/InMemoryExporterHelperExtensions.cs 60.00% <100.00%> (ø)
... and 9 more

@utpilla utpilla marked this pull request as ready for review December 2, 2020 23:08
@utpilla utpilla requested a review from a team December 2, 2020 23:08
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@cijothomas
Copy link
Member

@utpilla Please resolve the conflicts.

@cijothomas cijothomas merged commit e2a908e into open-telemetry:master Jan 6, 2021
@utpilla utpilla deleted the utpilla/Refactor-BaseExportProcessor branch January 27, 2021 22:33
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