Extend ConsoleExporter to add support for Logs

[utpilla]

Changes

• Extend ConsoleExporter to support LogRecords
• Removed displayAsJson as a configuration option and removed dependency on System.Text.Json
• Added net452 as a Target framework for ConsoleExporter
• Addressing Generalize ConsoleExporter #1373

[codecov]

Codecov Report

Merging #1438 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1438      +/-   ##
==========================================
- Coverage   81.70%   81.68%   -0.02%     
==========================================
  Files         229      229              
  Lines        6095     6095              
==========================================
- Hits         4980     4979       -1     
- Misses       1115     1116       +1     

Impacted Files	Coverage Δ
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	73.52% <0.00%> (-2.95%)	⬇️

[reyang]

Please add an entry to the CHANGELOG.md.

[reyang]

LGTM.

[reyang]

nit: the output is a bit different from the Activity one (e.g. here we have the LogRecord.Something).
Might be nice to make them consistent. Not blocking though.

[utpilla]

It's LogRecord.propertyName right now, what you suggest it be? Should we use "Activity" instead of "LogRecord"?

[reyang]

Would you help to update the example to use the console exporter?

opentelemetry-dotnet/docs/logs/getting-started/Program.cs

Line 33 in 46094ad

.AddInMemoryExporter()); // TODO: change to console output

We can use it to drive the output format discussion.

[CodeBlanch]

nit: Would prob be more efficient to check the type in ctor and then set a field instead of checking every batch.

[CodeBlanch]

So now I can do ConsoleExporter<some_custom_type> and it will emit nothing? I don't see a lot of value in having the generic, TBH. Why don't we just have ConsoleLogExporter : BaseExporter<LogRecord> & ConsoleActivityExporter : BaseExporter<Activity>? There's really not that much code being duplicated.

[CodeBlanch]

OR how about something like...

public class ConsoleExporter<T> : BaseExporter<T>
{
   public ConsoleExporter(Action<T> writeToConsole) { ... }

   public override ExportResult Export(in Batch<T> batch)
   {
      foreach (var item in batch) {
         this.writeToConsole(item);
     }
   }
}

public class ConsoleLogExporter : ConsoleExporter<LogRecord>
{
   public ConsoleLogExporter () : base(WriteLogToConsole) { }

   private static void WriteLogToConsole(LogRecord logRecord) {
      var rightPaddingLength = 30;

      Console.WriteLine($"{"LogRecord.TraceId:".PadRight(rightPaddingLength)}{logRecord.TraceId}");
      Console.WriteLine($"{"LogRecord.SpanId:".PadRight(rightPaddingLength)}{logRecord.SpanId}");
      Console.WriteLine($"{"LogRecord.Timestamp:".PadRight(rightPaddingLength)}{logRecord.Timestamp:yyyy-MM-ddTHH:mm:ss.fffffffZ}");
      Console.WriteLine($"{"LogRecord.EventId:".PadRight(rightPaddingLength)}{logRecord.EventId}");
      Console.WriteLine($"{"LogRecord.CategoryName:".PadRight(rightPaddingLength)}{logRecord.CategoryName}");
      Console.WriteLine($"{"LogRecord.LogLevel:".PadRight(rightPaddingLength)}{logRecord.LogLevel}");
      Console.WriteLine($"{"LogRecord.TraceFlags:".PadRight(rightPaddingLength)}{logRecord.TraceFlags}");
      Console.WriteLine($"{"LogRecord.State:".PadRight(rightPaddingLength)}{logRecord.State}");
      if (logRecord.Exception is { })
      {
         Console.WriteLine($"{"LogRecord.Exception:".PadRight(rightPaddingLength)}{logRecord.Exception?.Message}");
      }

      Console.WriteLine();
   }
}

[utpilla]

I don't think you can add an Exporter of a type anything other than Activity or LogRecord. To add an exporter for traces, you have to use the TracerProviderBuilder AddProcessor(BaseProcessor processor) method of TracerProviderBuilder class. Here, both SimpleExportProcessor and BatchExportProcessor will enforce you to only use Activity as the type.

Similarly, to add an exporter to Logs, you have to use the extension method OpenTelemetryLoggerOptions AddProcessor(BaseProcessor processor). Here, both SimpleExportProcessor and BatchExportProcessor will enforce you to only use LogRecord as the type.

[CodeBlanch]

That's a good point. I still feel like it is misusing generics and we could improve it though. If it was internal, no prob, but it is part of the public API so we'll have to live with it forever.

@cijothomas @reyang Any thoughts?

[reyang]

No strong opinion from my side.

I agree that having a switch-case (or if-else) in generic method (e.g. if T is Activity do this, if T is LogRecord do that) is not very idiomatic / performant.

[charzhao]

why remove output as json format, I think this is the useful function here, recently i want to put the log to splunks failed without josn output here.