OpenTelemetry.Exporter.Console: Print exception attributes according to SemConv #5351
Conversation
Perf. For exporters which do not need the exception string, we don't want the SDK to put pressure on GC heap. |
…com/gregkalapos/opentelemetry-dotnet into ConsoleExporter_ExceptionAttributes
Yes, perf is definitely a valid point. However, I was reading the SemConv spec, which says:
My interpretation of the But this is not yet part of this PR anyways, so I'm happy close this question. |
| @@ -81,23 +83,35 @@ public override ExportResult Export(in Batch<LogRecord> batch) | |||
| this.WriteLine($"{"LogRecord.Body:",-RightPaddingLength}{logRecord.Body}"); | |||
| } | |||
|
|
|||
| if (logRecord.Attributes != null) | |||
| if (logRecord.Attributes != null || logRecord.Exception != null) | |||
There was a problem hiding this comment.
No need to add this check.
There was a problem hiding this comment.
The idea here is to treat exception attributes like any other attributes, since SemConv defines them as attributes.
With this, the output is something like this:
LogRecord.Attributes (Key:Value):
OriginalFormat (a.k.a Body): You divided by 0
exception.type: DivideByZeroException
exception.message: Attempted to divide by zero.
exception.stacktrace: System.DivideByZeroException: Attempted to divide by zero.
at OpenTelemetry.Exporter.Console.Tests.ConsoleLogRecordExporterTest.VerifyExceptionAttributesAreWritten() in /Users/gregkalapos/repos/gregkalapos_opentelemetry-dotnet/test/OpenTelemetry.Exporter.Console.Tests/ConsoleLogRecordExporterTest.cs:line 39
LogRecord.Exception: System.DivideByZeroException: Attempted to divide by zero.
at OpenTelemetry.Exporter.Console.Tests.ConsoleLogRecordExporterTest.VerifyExceptionAttributesAreWritten() in /Users/gregkalapos/repos/gregkalapos_opentelemetry-dotnet/test/OpenTelemetry.Exporter.Console.Tests/ConsoleLogRecordExporterTest.cs:line 3
If we move the logging of exception attributes outside this block, then the LogRecord.Attributes (Key:Value): may not be printed (if no other attribute is present, and I'm not sure we can assume we'll always have any other attribute). Also the indentation of these attributes also matches other attributes' indentation on purpose.
There was a problem hiding this comment.
I'm concerned about the readability of the exception callstack, create a tracking issue with my suggestions #5372.
There was a problem hiding this comment.
I agree with the concerns raised by @reyang here. YAML has better support for multi-line strings. @gregkalapos Would you be interested in working on updating the ConsoleExporter to print the data in yaml format?
There was a problem hiding this comment.
Yeah, I'd be happy to look into that - I also think it's a good idea. Let's handle it in the issue.
What do we do with this PR?
The issue with the readability of the exception callstack is not new, LogRecord.Exception is already printed a few lines below with this output (that's not changed in the PR):
LogRecord.Exception: System.DivideByZeroException: Attempted to divide by zero.
at OpenTelemetry.Exporter.Console.Tests.ConsoleLogRecordExporterTest.VerifyExceptionAttributesAreWritten() in /Users/gregkalapos/repos/gregkalapos_opentelemetry-dotnet/test/OpenTelemetry.Exporter.Console.Tests/ConsoleLogRecordExporterTest.cs:line 39
I see #5372 and this PR orthogonal.
There was a problem hiding this comment.
The issue with the readability of the exception callstack is not new,
I see #5372 and this PR orthogonal.
I don't see them as orthogonal. The issue clearly talks about new things getting added with no well-defined structure. Just because the ConsoleExporter already has a readability issue, doesn't mean we keep making it worse.
What do we do with this PR?
I would say we should close it and invest in making ConsoleExporter better and extensible for newer changes in a well-defined way. I don't think anyone is blocked on ConsoleExporter not printing out the exception information with Semantic Convention attributes. As you previously mentioned, the exception information is already printed to console in some other format.
| } | ||
| } | ||
|
|
||
| if (logRecord.Exception != null) |
There was a problem hiding this comment.
Move this if block outside of the if (logRecord.Attributes != null) check.
There was a problem hiding this comment.
Same reasoning as above: #5351 (comment)
| { | ||
| using var writer = new StringWriter(); | ||
| System.Console.SetOut(writer); | ||
| using var tracerProvider = Sdk.CreateTracerProviderBuilder() |
There was a problem hiding this comment.
Remove traces related code?
| .Build(); | ||
|
|
||
| using ILoggerFactory factory = LoggerFactory | ||
| .Create(builder => builder.AddOpenTelemetry(n => |
There was a problem hiding this comment.
nit: Use more intuitive variable names.
| .Create(builder => builder.AddOpenTelemetry(n => | |
| .Create(builder => builder.AddOpenTelemetry(logging => |
| using ILoggerFactory factory = LoggerFactory | ||
| .Create(builder => builder.AddOpenTelemetry(n => | ||
| { | ||
| n.AddConsoleExporter(); |
There was a problem hiding this comment.
| n.AddConsoleExporter(); | |
| logging.AddConsoleExporter(); |
| .AddConsoleExporter() | ||
| .Build(); | ||
|
|
||
| using ILoggerFactory factory = LoggerFactory |
There was a problem hiding this comment.
Since you have been using `var at other places, you could just use it for LoggerFactory and Logger as well to be consistent.
| })); | ||
|
|
||
| ILogger logger = factory.CreateLogger("Program"); | ||
| try |
There was a problem hiding this comment.
Instead of this setup, you could simply log an exception with a custom message to test this. Something like:
logger.LogError(default(EventId), new Exception("Exception Message"), null);Review feedback
|
Thanks for the review @utpilla. Full agreement on changes in Regarding the suggestions in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5351 +/- ##
==========================================
+ Coverage 83.38% 83.53% +0.15%
==========================================
Files 297 273 -24
Lines 12531 11384 -1147
==========================================
- Hits 10449 9510 -939
+ Misses 2082 1874 -208
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
Follow up from #5342 (comment) - and open-telemetry/semantic-conventions#689
Changes
ConsoleLogRecordExporterprints exception attributes according to the SemConv spec. With this PR,ConsoleLogRecordExporterbehaves similarly toOtlpLogRecordTransformerfromOpenTelemetry.Exporter.OpenTelemetryProtocol.Open questions
Since I'm fairly new to the code-base I wanted to ask this before I do any such changes - let me know if you think it'd make sense to explore this and I'm happy to update the PR - or open a follow up.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes