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

Jump to bottom

Use Utf8JsonWriter in EventSourceLogger #1227

Merged
merged 3 commits into from
Mar 7, 2019
Merged

Use Utf8JsonWriter in EventSourceLogger #1227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6acb53d
Use Utf8JsonWriter in EventSourceLogger
pakrym Mar 7, 2019
ee8fb1a
ref
pakrym Mar 7, 2019
8e03e94
Nowarn
pakrym Mar 7, 2019
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
7 changes: 7 additions & 0 deletions src/Logging/Logging.EventSource/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<Project>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory)..\, Directory.Build.props))\Directory.Build.props" />

<PropertyGroup>
<NoWarn>$(NoWarn);PKG0001</NoWarn>
</PropertyGroup>
</Project>
2 changes: 1 addition & 1 deletion src/Logging/Logging.EventSource/ref/Microsoft.Extensions.Logging.EventSource.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<Compile Include="Microsoft.Extensions.Logging.EventSource.netstandard2.0.cs" />
<Reference Include="Microsoft.Extensions.Logging" />
<Reference Include="Newtonsoft.Json" />
<Reference Include="System.Threading.Tasks.Extensions" />
</ItemGroup>
</Project>
20 changes: 11 additions & 9 deletions src/Logging/Logging.EventSource/src/EventSourceLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
using System.Collections.Generic;
using System.Diagnostics.Tracing;
using System.IO;
using System.Text;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading;
using Newtonsoft.Json;

namespace Microsoft.Extensions.Logging.EventSource
{
Expand Down Expand Up @@ -205,19 +207,19 @@ private ExceptionInfo GetExceptionInfo(Exception exception)

private string ToJson(IReadOnlyList<KeyValuePair<string, string>> keyValues)
{
var sw = new StringWriter();
var writer = new JsonTextWriter(sw);
writer.DateFormatString = "O"; // ISO 8601
var arrayBufferWriter = new ArrayBufferWriter<byte>();
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... using an internal type, which wasn't meant to be used here. But hey, source package :)

var writer = new Utf8JsonWriter(arrayBufferWriter);

writer.WriteStartObject();
for (int i = 0; i < keyValues.Count; i++)
foreach (var keyValue in keyValues)
{
var keyValue = keyValues[i];
writer.WritePropertyName(keyValue.Key, true);
writer.WriteValue(keyValue.Value);
writer.WriteString(keyValue.Key, keyValue.Value, true);
}
writer.WriteEndObject();
return sw.ToString();

writer.Flush(true);

return Encoding.UTF8.GetString(arrayBufferWriter.WrittenMemory.ToArray());
}
}
}
6 changes: 5 additions & 1 deletion src/Logging/Logging.EventSource/src/Microsoft.Extensions.Logging.EventSource.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@
<PackageTags>$(PackageTags);EventSource;ETW</PackageTags>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IsShipping>true</IsShipping>
<NoWarn>$(NoWarn);CS3021</NoWarn>
</PropertyGroup>

<ItemGroup>
<Compile Include="../../shared/*.cs" />

<Reference Include="Microsoft.Bcl.Json.Sources">
Copy link
Member

Choose a reason for hiding this comment

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

Does this library have to support netstandard2.0?

Copy link

Choose a reason for hiding this comment

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

Yes, it was netstandard2.0 in 2.2 and I don't think we have plans to change that. Most of Microsoft.Extensions.* is designed to remain netstandard2.0

<PrivateAssets>All</PrivateAssets>
</Reference>
<Reference Include="Microsoft.Extensions.Logging" />
<Reference Include="Newtonsoft.Json" />
<Reference Include="System.Threading.Tasks.Extensions" />
</ItemGroup>

</Project>
4 changes: 2 additions & 2 deletions src/Logging/Logging.EventSource/test/EventSourceLoggerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
using System.IO;
using System.Linq;
using Microsoft.Extensions.Logging.EventSource;
using Newtonsoft.Json;
using Xunit;
using Microsoft.Extensions.DependencyInjection;
using Newtonsoft.Json;

namespace Microsoft.Extensions.Logging.Test
{
Expand Down Expand Up @@ -692,7 +692,7 @@ private static class EventTypes

{ "E5JS", (e) => VerifySingleEvent(e, "Logger2", EventTypes.MessageJson, 5, null, LogLevel.Critical,
@"""ArgumentsJson"":{""stringParam"":""bar"",""int1Param"":""23"",""int2Param"":""45""",
@"""ExceptionJson"":{""TypeName"":""System.Exception"",""Message"":""oops"",""HResult"":""-2146233088"",""VerboseMessage"":""System.Exception: oops ---> System.Exception: inner oops") },
@"""ExceptionJson"":{""TypeName"":""System.Exception"",""Message"":""oops"",""HResult"":""-2146233088"",""VerboseMessage"":""System.Exception: oops ---\u003e System.Exception: inner oops") },
Copy link

Choose a reason for hiding this comment

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

?

I mean, it's the same, right? \u003e should be >

Copy link
Author

Choose a reason for hiding this comment

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

Yep, Utf8JsonWriter just decides to escape it for some reason. The end result is equivalent so I didn't spend too much time digging.

Copy link

Choose a reason for hiding this comment

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

Fair enough :)

Copy link

Choose a reason for hiding this comment

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

Looks like https://github.com/dotnet/corefx/blob/3eacad602f7505f9f199dcd1992429506dd98330/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs#L15-L17

        // Only allow ASCII characters between ' ' (0x20) and '~' (0x7E), inclusively,
        // but exclude characters that need to be escaped as hex: '"', '\'', '&', '+', '<', '>', '`'
        // and exclude characters that need to be escaped by adding a backslash: '\n', '\r', '\t', '\\', '/', '\b', '\f'

It's surprising to me that < needs to be escaped in JSON. @ahsonkhan, any particular reason for this? Mostly curious but it does seem like a user would be surprised to have it escaped in a string...

Copy link
Member

Choose a reason for hiding this comment

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

The JSON escaping here is following a strict mode to be SDL compliant. This is following the semantics of the default Javascript escaper which also escapes all non-ascii characters (and some ascii as well, including those that show up in html). This is primarily for defense in depth since JSON payloads could be embedded within html.

We currently do not have a way to customize the escaping behavior, but I imagine a writer option to plug in a custom escaper. Something that would work with https://github.com/dotnet/corefx/issues/34830

Some related issues:
https://github.com/dotnet/corefx/issues/34958
https://github.com/dotnet/corefx/issues/35385

cc @GrabYourPitchforks

Copy link

Choose a reason for hiding this comment

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

Sounds fine to me. I mostly just wanted to confirm that this was an intentional choice because it was a little surprising. Thanks!


{ "E5MSG", (e) => VerifySingleEvent(e, "Logger2", EventTypes.Message, 5, null, LogLevel.Critical,
@"{""Key"":""stringParam"",""Value"":""bar""}",
Expand Down
1 change: 1 addition & 0 deletions src/Logging/Logging.EventSource/test/Microsoft.Extensions.Logging.EventSource.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<ItemGroup>
<Reference Include="Microsoft.Extensions.Logging" />
<Reference Include="Microsoft.Extensions.Logging.EventSource" />
<Reference Include="Newtonsoft.Json" />
</ItemGroup>

</Project>
6 changes: 3 additions & 3 deletions src/Logging/Logging.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 15
VisualStudioVersion = 15.0.26124.0
MinimumVisualStudioVersion = 15.0.26124.0
# Visual Studio Version 16
VisualStudioVersion = 16.0.28508.60
MinimumVisualStudioVersion = 16.0.0.0
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Logging", "Logging", "{0AD43E03-5376-4F5A-87F3-552A58807916}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.Extensions.Logging.Performance", "Logging\perf\Microsoft.Extensions.Logging.Performance.csproj", "{AFD4AC92-1110-49EA-A81B-292C62A427ED}"
Expand Down