-
Notifications
You must be signed in to change notification settings - Fork 785
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
OpenTelemetry.Exporter.Console: Print exception attributes according to SemConv #5351
Changes from 4 commits
a04d03b
11a16c5
d9d450a
7affb58
5ab6630
c6cbb5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
using System.Runtime.CompilerServices; | ||
using OpenTelemetry.Internal; | ||
using OpenTelemetry.Logs; | ||
using OpenTelemetry.Resources; | ||
using OpenTelemetry.Trace; | ||
|
||
namespace OpenTelemetry.Exporter; | ||
|
||
|
@@ -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) | ||
{ | ||
this.WriteLine("LogRecord.Attributes (Key:Value):"); | ||
for (int i = 0; i < logRecord.Attributes.Count; i++) | ||
if (logRecord.Attributes != null) | ||
{ | ||
// Special casing {OriginalFormat} | ||
// See https://github.com/open-telemetry/opentelemetry-dotnet/pull/3182 | ||
// for explanation. | ||
var valueToTransform = logRecord.Attributes[i].Key.Equals("{OriginalFormat}") | ||
? new KeyValuePair<string, object>("OriginalFormat (a.k.a Body)", logRecord.Attributes[i].Value) | ||
: logRecord.Attributes[i]; | ||
|
||
if (ConsoleTagTransformer.Instance.TryTransformTag(valueToTransform, out var result)) | ||
for (int i = 0; i < logRecord.Attributes.Count; i++) | ||
{ | ||
this.WriteLine($"{string.Empty,-4}{result}"); | ||
// Special casing {OriginalFormat} | ||
// See https://github.com/open-telemetry/opentelemetry-dotnet/pull/3182 | ||
// for explanation. | ||
var valueToTransform = logRecord.Attributes[i].Key.Equals("{OriginalFormat}") | ||
? new KeyValuePair<string, object>( | ||
"OriginalFormat (a.k.a Body)", | ||
logRecord.Attributes[i].Value) | ||
: logRecord.Attributes[i]; | ||
|
||
if (ConsoleTagTransformer.Instance.TryTransformTag(valueToTransform, out var result)) | ||
{ | ||
this.WriteLine($"{string.Empty,-4}{result}"); | ||
} | ||
} | ||
} | ||
|
||
if (logRecord.Exception != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reasoning as above: #5351 (comment) |
||
{ | ||
this.PrintAttribute(SemanticConventions.AttributeExceptionType, logRecord.Exception.GetType().Name); | ||
this.PrintAttribute(SemanticConventions.AttributeExceptionMessage, logRecord.Exception.Message); | ||
this.PrintAttribute(SemanticConventions.AttributeExceptionStacktrace, logRecord.Exception.ToInvariantString()); | ||
} | ||
} | ||
|
||
if (logRecord.EventId != default) | ||
|
@@ -163,4 +177,16 @@ protected override void Dispose(bool disposing) | |
|
||
base.Dispose(disposing); | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
private void PrintAttribute(string key, string value) | ||
{ | ||
var valueToTransform = | ||
new KeyValuePair<string, object>(key, value); | ||
|
||
if (ConsoleTagTransformer.Instance.TryTransformTag(valueToTransform, out var result)) | ||
{ | ||
this.WriteLine($"{string.Empty,-4}{result}"); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,57 @@ | ||||||
// Copyright The OpenTelemetry Authors | ||||||
// SPDX-License-Identifier: Apache-2.0 | ||||||
|
||||||
using Microsoft.Extensions.Logging; | ||||||
using OpenTelemetry.Logs; | ||||||
using OpenTelemetry.Resources; | ||||||
using OpenTelemetry.Trace; | ||||||
using Xunit; | ||||||
|
||||||
namespace OpenTelemetry.Exporter.Console.Tests; | ||||||
|
||||||
public class ConsoleLogRecordExporterTest | ||||||
{ | ||||||
[Fact] | ||||||
public void VerifyExceptionAttributesAreWritten() | ||||||
{ | ||||||
using var writer = new StringWriter(); | ||||||
System.Console.SetOut(writer); | ||||||
using var tracerProvider = Sdk.CreateTracerProviderBuilder() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove traces related code? |
||||||
.AddSource("*") | ||||||
.ConfigureResource(resource => | ||||||
resource.AddService( | ||||||
serviceName: "Test_VerifyExceptionAttributesAreWritten", | ||||||
serviceVersion: "1.0.0")) | ||||||
.AddConsoleExporter() | ||||||
.Build(); | ||||||
|
||||||
using ILoggerFactory factory = LoggerFactory | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you have been using `var at other places, you could just use it for LoggerFactory and Logger as well to be consistent. |
||||||
.Create(builder => builder.AddOpenTelemetry(n => | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Use more intuitive variable names.
Suggested change
|
||||||
{ | ||||||
n.AddConsoleExporter(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
})); | ||||||
|
||||||
ILogger logger = factory.CreateLogger("Program"); | ||||||
try | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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); |
||||||
{ | ||||||
int num1 = 5, num2 = 0, division_res = 0; | ||||||
division_res = num1 / num2; | ||||||
} | ||||||
catch (Exception ex) | ||||||
{ | ||||||
logger.LogError(ex, "You divided by 0"); | ||||||
} | ||||||
|
||||||
writer.Flush(); | ||||||
var consoleLog = writer.ToString(); | ||||||
|
||||||
Assert.Contains("exception.type", consoleLog); | ||||||
Assert.Contains("DivideByZeroException", consoleLog); | ||||||
|
||||||
Assert.Contains("exception.message", consoleLog); | ||||||
Assert.Contains("Attempted to divide by zero.", consoleLog); | ||||||
|
||||||
Assert.Contains("exception.stacktrace", consoleLog); | ||||||
Assert.Contains("System.DivideByZeroException: Attempted to divide by zero.", consoleLog); | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ConsoleExporte
r to print the data inyaml
format?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):I see #5372 and this PR orthogonal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.