Skip to content

Refactored ConsoleExporter#1590

Closed
utpilla wants to merge 2 commits into
open-telemetry:masterfrom
utpilla:utpilla/Refactor-ConsoleExporter
Closed

Refactored ConsoleExporter#1590
utpilla wants to merge 2 commits into
open-telemetry:masterfrom
utpilla:utpilla/Refactor-ConsoleExporter

Conversation

@utpilla

@utpilla utpilla commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

Changes

  • Refactored ConsoleExporter generic class to get rid of type specific check
  • Added ConsoleActivityExporter and ConsoleLogRecordExporter classes

@utpilla

utpilla commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

@CodeBlanch It'd be great if you could take a look at this and let me know of what you think. This is in regards to this discussion: #1438 (comment)

@utpilla

utpilla commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

@cijothomas @reyang I would love to get any suggestions for this.

@codecov

codecov Bot commented Nov 19, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1590 (2facdf3) into master (b710c31) will decrease coverage by 0.10%.
The diff coverage is 2.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1590      +/-   ##
==========================================
- Coverage   80.74%   80.64%   -0.11%     
==========================================
  Files         242      244       +2     
  Lines        6544     6555      +11     
==========================================
+ Hits         5284     5286       +2     
- Misses       1260     1269       +9     
Impacted Files Coverage Δ
...emetry.Exporter.Console/ConsoleActivityExporter.cs 0.00% <0.00%> (ø)
.../OpenTelemetry.Exporter.Console/ConsoleExporter.cs 21.42% <0.00%> (+16.51%) ⬆️
...xporter.Console/ConsoleExporterHelperExtensions.cs 0.00% <0.00%> (ø)
...porter.Console/ConsoleExporterLoggingExtensions.cs 0.00% <0.00%> (ø)
...metry.Exporter.Console/ConsoleLogRecordExporter.cs 0.00% <0.00%> (ø)
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs 15.38% <0.00%> (-17.95%) ⬇️
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs 100.00% <100.00%> (ø)

@CodeBlanch

Copy link
Copy Markdown
Member

@utpilla I like it! I think this is definitely a more standard use of generics. Could probably clean it up even further like...

public abstract class ConsoleExporter<T> : BaseExporter<T>
{
   public override ExportResult Export(in Batch<T> batch)
   {
      foreach (var item in batch)
      {
         ExportItem(item);
      }
   }

   protected abstract void ExportItem(T item);

   protected void WriteLine(string message) { /* Existing code goes here*/ }
}

internal class ConsoleActivityExporter : ConsoleExporter<Activity>
{
   protected override void ExportItem(Activity item)
   {
      WriteLine($"Activity.Id:          {activity.Id}");
      /* Etc. */
   }
}

Tweaks: Made ConsoleExporter abstract, switched the action to an abstract method, and made WriteLine protected so super can call it.

@utpilla utpilla closed this Nov 19, 2020
@utpilla

utpilla commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

I have created a new PR #1593 with the suggested changes.

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.

2 participants