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

EventSourceEventFormatting - catch FormatException #14697

Merged
7 commits merged into from
Sep 9, 2020
+30 −2
Merged

EventSourceEventFormatting - catch FormatException #14697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9443af7
EventSourceEventFormatting - catch FormatException
asrosent Aug 30, 2020
58bc086
revision 2
asrosent Aug 31, 2020
6c9abf0
revision 3
asrosent Aug 31, 2020
e894d59
revision 4
asrosent Aug 31, 2020
0b43f0f
revision 5
asrosent Sep 1, 2020
f0c520d
revision 6
asrosent Sep 6, 2020
adbfa68
Merge remote-tracking branch 'azure/master' into fixFormatException
pakrym Sep 8, 2020
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
16 changes: 15 additions & 1 deletion sdk/core/Azure.Core/src/Shared/EventSourceEventFormatting.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using Azure.Core.Diagnostics;
using System;
using System.Diagnostics.Tracing;
using System.Globalization;
using System.Linq;
Expand All @@ -18,12 +20,24 @@ public static string Format(EventWrittenEventArgs eventData)

if (eventData.Message != null)
{
return string.Format(CultureInfo.InvariantCulture, eventData.Message, payloadArray);
try
{
return string.Format(CultureInfo.InvariantCulture, eventData.Message, payloadArray);
}
catch (FormatException)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in some occasions the message inside the eventData is not formattable.
when this happend the program recieves a FormatException in this point.

solution,
catch the format exception and build a message without the formatting.

{
Copy link
Member

Choose a reason for hiding this comment

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

We should consider adding a line here to call out that the message could not be formatted. If we get to this point, I believe that it indicates a bug in our logging code and it would be helpful if those that encounter this have the awareness and information to open a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should also think if we can catch it earlier, some analyzer or test utility possibly.

}
}

var stringBuilder = new StringBuilder();
stringBuilder.Append(eventData.EventName);

if (!string.IsNullOrWhiteSpace(eventData.Message))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we appending the eventData.Message here?

Copy link
Contributor Author

@asrosent asrosent Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If theres a case when you do have a message and you got to this part it means you failed formatting and caught the exception.

In this case you still want to print the message and the payload. Just not in a formatted way.

{
stringBuilder.AppendLine();
stringBuilder.Append(nameof(eventData.Message)).Append(" = ").Append(eventData.Message);
}

for (int i = 0; i < eventData.PayloadNames.Count; i++)
{
stringBuilder.AppendLine();
Expand Down
16 changes: 15 additions & 1 deletion sdk/core/Azure.Core/tests/AzureEventSourceListenerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Diagnostics.Tracing;
using System.Linq;
using Azure.Core.Diagnostics;
using Azure.Core.TestFramework;
using NUnit.Framework;

namespace Azure.Core.Tests
Expand Down Expand Up @@ -71,6 +70,15 @@ public void FormatsAsKeyValuesIfNoMessage()
"other = 5", message);
}

[Test]
public void FormatsUnformattableMessageAsKeyValues()
{
(EventWrittenEventArgs e, string message) = ExpectSingleEvent(() => TestSource.Log.LogUnformattableMessage("a message"));
Assert.AreEqual("LogUnformattableMessage" + Environment.NewLine +
nameof(e.Message) + " = Logging {1}" + Environment.NewLine +
"payload = a message", message);
}

private static (EventWrittenEventArgs, string) ExpectSingleEvent(Action logDelegate)
{
var invocations = new List<(EventWrittenEventArgs, string)>();
Expand Down Expand Up @@ -109,6 +117,12 @@ public void LogWithByteArray(byte[] b)
{
WriteEvent(3, b);
}

[Event(4, Message = "Logging {1}", Level = EventLevel.Critical)]
public void LogUnformattableMessage(string payload)
{
WriteEvent(4, payload);
}
}
}
}