Initial PR for structured logging support #1588

normj · 2023-09-25T01:41:23Z

Description of changes:

This PR is the initial work to add support for structured logging natively in the Amazon.Lambda.RuntimeSupport the .NET Lambda runtime client (RIC). Serilog was used as the example of the experience but is not used directly so that we would not take a hard dependency on Serilog or interfere with anybody wanting to implement their own structured logging using Serilog.

Changes made:

The ILambdaLogger has new parameterized logging methods
The ConsoleLoggerWriter was refactored to have formatting being done in separate formatting classes that are swapped out depending on the environment variable configuring formatter. The DefaultLogMessageFormatter formatter represents what we do today and JsonLogMessageFormatter is the new structured logging.

NOTE: The new parameterize logging methods on ILambadLogger are marked with the .NET attribute RequiresPreviewFeatures. This requires users that want to use these methods to have to turn on preview language features in the project file, <LangVersion>preview</LangVersion>. This is done due to the fact that Amazon.Lambda.Core with these new APIs will go out before the .NET managed runtime is deployed with the version of Amazon.Lambda.RuntimeSupport. If users call these methods before the runtime is updated the message will be logged ignoring the parameters. For users that deploy as an executable with the latest version version of Amazon.Lambda.RuntimeSupport they will be able to use these methods with no problems. Once Amazon.Lambda.RuntimeSupport has been updated in the managed runtime a new version of Amazon.Lambda.Core will be released removing the RequiresPreviewFeatures attribute.

This PR uses a lot of the code ideas from PR #1323 by @dhhoang. The major difference are

Pushing more of the work into Amazon.Lambda.RuntimeSupport instead Amazon.Lambda.Core
Add more scenarios supported by Serilog
Match work Lambda team is doing in other language runtimes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ent new JSON structured log formatter in Amazon.Lambda.RuntimeSupport

birojnayak · 2023-10-06T18:46:04Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

        {
            _writer.WriteLine(message);
+            _writer.WriteLine(exception.ToString());


null check needed ?

birojnayak · 2023-10-06T18:46:50Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

@@ -107,7 +128,7 @@ public class LogLevelLoggerWriter : IConsoleLoggerWriter
        /// Amazon.Lambda.Core can not be relied on because the Lambda Function could be using
        /// an older version of Amazon.Lambda.Core before LogLevel existed in Amazon.Lambda.Core.
        /// </summary>
-        enum LogLevel
+        public enum LogLevel


are we making it public for user to use ? if it's internal to package, can we live with internal ?

This doesn't need to be public. Users interact with the LogLevel enum from Amazon.Lambda.Core. RuntimeSupport has to have a mirror of this enum because it can't rely on having the latest copy of Amazon.Lambda.Core when running that would have the enum.

birojnayak · 2023-10-06T18:59:11Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/AbstractLogMessageFormatter.cs

+using System.Globalization;
+using System.Text;
+
+namespace Amazon.Lambda.RuntimeSupport.Helpers.Logging


Here are few pointers as I was scanning through the Microsoft implementations. I am suggesting to see if we can get some inspirations and the code I am referring is well tested and inside their SDK (less buggy, so my inspirations :)).

https://source.dot.net/#Microsoft.Extensions.Logging.Abstractions/LogValuesFormatter.cs,66cbe2d145df9bc8 => This is where given a string (in our case it's a message template), it spits out various params.

https://source.dot.net/#Microsoft.Extensions.Logging.Abstractions/FormattedLogValues.cs,b8a158d8a121f959 => it holds on to the State (in our case MessageState). The ToString method uses the formatter to generate the string

One of the Implementation I was looking for both Json and normal logging is EventSource => https://source.dot.net/#Microsoft.Extensions.Logging.EventSource/EventSourceLogger.cs,92 , which actually adds the json values for all properties as mentioned in the req/design spec.

Libraries/src/Amazon.Lambda.Core/ILambdaLogger.cs

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/AbstractLogMessageFormatter.cs

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/JsonLogMessageFormatter.cs

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/MessageProperty.cs

philasmar · 2024-01-31T14:48:58Z

Libraries/src/Amazon.Lambda.Core/ILambdaLogger.cs

+        /// <param name="message">Message to log.</param>
+        /// <param name="args">Values to be replaced in log messages that are parameterized.</param>
+        [RequiresPreviewFeatures(ParameterizedPreviewMessage)]
+        void Log(string level, string message, params object[] args) => Log(level, message);


Shouldn't args be passed down to the next function?

Yes, good catch.

philasmar · 2024-01-31T14:50:42Z

Libraries/src/Amazon.Lambda.Core/ILambdaLogger.cs

+        /// <param name="message">Message to log.</param>
+        /// <param name="args">Values to be replaced in log messages that are parameterized.</param>
+        [RequiresPreviewFeatures("Parameterized logging is in preview till new version of .NET Lambda runtime client is deployed to the .NET Lambda managed runtime.")]
+        void Log(string level, Exception exception, string message, params object[] args)


Same comment here. Not sure why args is not being used.

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

philasmar · 2024-01-31T14:59:15Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

+        /// <param name="level"></param>
+        /// <param name="exception"></param>
+        /// <param name="message"></param>
+        /// <param name="args"></param>


Could you add more docs for the params similar to ILambdaLogger?

philasmar · 2024-01-31T15:12:07Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/AbstractLogMessageFormatter.cs

+        /// <param name="messageTemplate"></param>
+        /// <returns></returns>


nit: add docs for these tags

philasmar · 2024-01-31T15:14:39Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/AbstractLogMessageFormatter.cs

+        /// <param name="state"></param>
+        /// <returns></returns>


nit: add docs for the empty tags in this class

philasmar · 2024-01-31T15:17:21Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/MessageProperty.cs

+        private static readonly char[] PARAM_FORMAT_DELIMITERS = { ':' };
+
+        /// <summary>
+        /// Parse the string string representation of the message property without the brackets 


string is duplicated

philasmar · 2024-01-31T15:17:53Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/MessageProperty.cs

+        /// </summary>
+        public string MessageToken { get; private set; }
+
+        public enum Directive { Default, JsonSerialization };


add docs for this property

philasmar · 2024-04-04T13:20:16Z

Libraries/src/Amazon.Lambda.Core/ILambdaLogger.cs

+            "project file to \"true\"";
+
+        /// <summary>
+        /// Log message catagorized by the given log level


nit: typo in "catagorized"

philasmar · 2024-04-04T13:20:23Z

Libraries/src/Amazon.Lambda.Core/ILambdaLogger.cs

+        void Log(string level, string message, params object[] args) => Log(level, message, args);
+
+        /// <summary>
+        /// Log message catagorized by the given log level


nit: typo in "catagorized"

philasmar · 2024-04-04T13:20:38Z

Libraries/src/Amazon.Lambda.Core/ILambdaLogger.cs

+        }
+
+        /// <summary>
+        /// Log message catagorized by the given log level


nit: typo in "catagorized"

philasmar · 2024-04-04T13:20:45Z

Libraries/src/Amazon.Lambda.Core/ILambdaLogger.cs

+        void Log(LogLevel level, string message, params object[] args) => Log(level.ToString(), message, args);
+
+        /// <summary>
+        /// Log message catagorized by the given log level


nit: typo in "catagorized"

philasmar · 2024-04-04T13:25:59Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

@@ -89,9 +104,18 @@ public void FormattedWriteLine(string message)
            _writer.WriteLine(message);
        }

-        public void FormattedWriteLine(string level, string message)
+        public void FormattedWriteLine(string level, string message, params object[] args)


nit: add docs to public methods

philasmar · 2024-04-04T13:26:13Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/ConsoleLoggerWriter.cs

+            _wrappedStdOutWriter.FormattedWriteLine(level, (Exception)null, message, args);
+        }
+
+        public void FormattedWriteLine(string level, Exception exception, string message, params object[] args)


nit: add docs to public methods

philasmar · 2024-04-04T13:32:43Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/AbstractLogMessageFormatter.cs

+                            }
+                            else
+                            {
+                                argument = messageArguments[propertyIndex];


Could you please add a null check for messageArguments before indexing into it? I got a warning for it in Rider.

I added a short circuit at the top that exits out if messageArguments is null or empty.

philasmar · 2024-04-04T13:34:42Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/JsonLogMessageFormatter.cs

+        private static readonly UTF8Encoding UTF8NoBomNoThrow = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: false);
+
+        // Options used when serializing any message property values as a JSON to be added to the structured log message.
+        private JsonSerializerOptions _jsonSerialiazationOptions;


typo in _jsonSerialiazationOptions. should be _jsonSerializationOptions

philasmar · 2024-04-04T13:36:37Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/JsonLogMessageFormatter.cs

+                    writer.WriteStartObject();
+                    foreach (DictionaryEntry entry in ((IDictionary)messageArgument))
+                    {
+                        writer.WritePropertyName(entry.Key.ToString());


entry.Key.ToString() throws a possible null warning. Could you change it to entry.Key.ToString() ?? string.Empty or something similar?

Done but if key is null by this point something has really gone wrong.

philasmar · 2024-04-04T13:38:02Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Helpers/Logging/MessageProperty.cs

+
+            try
+            {
+                var formatedValue = string.Format("{0:" + formatArgument + "}", value);


typo in formatedValue

ashovlin

Good assuming you address @philasmar 's comments, but don't need to see again.

Update Amazon.Lambda.Core to have new parameterize logging and implem…

21d32b5

…ent new JSON structured log formatter in Amazon.Lambda.RuntimeSupport

normj mentioned this pull request Sep 25, 2023

Add parameterized logging support to ILambdaLogger #1323

Closed

normj requested review from ashovlin, 96malhar and philasmar September 25, 2023 17:40

birojnayak reviewed Oct 6, 2023

View reviewed changes

ashovlin reviewed Jan 29, 2024

View reviewed changes

philasmar reviewed Jan 31, 2024

View reviewed changes

This was referenced Mar 29, 2024

Lambda multiline logging messages - different behaviour between Zip & Image (docker) package type #1662

Closed

LAMBDA_NET_SERIALIZER_DEBUG=true splits payloads over multiple lines #1705

Closed

Address PR comments.

924b1cd

normj force-pushed the normj/initial-structured-logging branch from 56ba04d to 924b1cd Compare April 2, 2024 06:24

philasmar reviewed Apr 4, 2024

View reviewed changes

ashovlin approved these changes Apr 4, 2024

View reviewed changes

Address PR comments

cb5a9df

philasmar approved these changes Apr 4, 2024

View reviewed changes

normj merged commit 920f42f into feature/structured-logging Apr 4, 2024
2 of 3 checks passed

normj deleted the normj/initial-structured-logging branch April 4, 2024 22:45

		/// <param name="messageTemplate"></param>
		/// <returns></returns>

Initial PR for structured logging support #1588

Initial PR for structured logging support #1588

Conversation

normj commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birojnayak Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birojnayak Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashovlin left a comment

Choose a reason for hiding this comment

birojnayak Oct 6, 2023 •

edited

Loading

birojnayak Oct 6, 2023 •

edited

Loading