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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

OpenTelemetry.Exporter.Console: Print exception attributes according to SemConv #5351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a04d03b
Print exception attributes according to SemConv
gregkalapos Feb 12, 2024
11a16c5
Merge branch 'main' into ConsoleExporter_ExceptionAttributes
gregkalapos Feb 14, 2024
d9d450a
Update ConsoleLogRecordExporter.cs
gregkalapos Feb 14, 2024
7affb58
Merge branch 'ConsoleExporter_ExceptionAttributes' of https://github.…
gregkalapos Feb 14, 2024
5ab6630
Update ConsoleLogRecordExporterTest.cs
gregkalapos Feb 19, 2024
c6cbb5f
Update ConsoleLogRecordExporterTest.cs
gregkalapos Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 37 additions & 11 deletions src/OpenTelemetry.Exporter.Console/ConsoleLogRecordExporter.cs
View file
Open in desktop
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;

Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Member Author

@gregkalapos gregkalapos Feb 19, 2024 •
edited
Loading

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:

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.

Copy link
Member

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.

Copy link
Contributor

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 ConsoleExporter to print the data in yaml format?

Copy link
Member Author

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):

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this if block outside of the if (logRecord.Attributes != null) check.

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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}");
}
}
}
1 change: 1 addition & 0 deletions src/OpenTelemetry.Exporter.Console/OpenTelemetry.Exporter.Console.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
<Compile Include="$(RepoRoot)\src\Shared\DiagnosticDefinitions.cs" Link="Includes\DiagnosticDefinitions.cs" RequiresExposedExperimentalFeatures="true" />
<Compile Include="$(RepoRoot)\src\Shared\ExceptionExtensions.cs" Link="Includes\ExceptionExtensions.cs" RequiresExposedExperimentalFeatures="true" />
<Compile Include="$(RepoRoot)\src\Shared\SpanAttributeConstants.cs" Link="Includes\SpanAttributeConstants.cs" RequiresExposedExperimentalFeatures="true" />
<Compile Include="$(RepoRoot)\src\Shared\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" RequiresExposedExperimentalFeatures="true" />
</ItemGroup>

</Project>
41 changes: 41 additions & 0 deletions test/OpenTelemetry.Exporter.Console.Tests/ConsoleLogRecordExporterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using Microsoft.Extensions.Logging;
using OpenTelemetry.Logs;
using Xunit;

namespace OpenTelemetry.Exporter.Console.Tests;

public class ConsoleLogRecordExporterTest
{
[Fact]
public void VerifyExceptionAttributesAreWritten()
{
var originalConsoleOut = System.Console.Out;
using var writer = new StringWriter();
System.Console.SetOut(writer);

using var factory = LoggerFactory
.Create(builder => builder.AddOpenTelemetry(logging =>
{
logging.AddConsoleExporter();
}));

var logger = factory.CreateLogger("Program");
logger.LogError(default, new Exception("Exception Message"), null);

writer.Flush();
var consoleLog = writer.ToString();
System.Console.SetOut(originalConsoleOut);

Assert.Contains("exception.type", consoleLog);
Assert.Contains("Exception", consoleLog);

Assert.Contains("exception.message", consoleLog);
Assert.Contains("Exception Message", consoleLog);

Assert.Contains("exception.stacktrace", consoleLog);
Assert.Contains("Exception: Exception Message", consoleLog);
}
}