-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: Add bring your own log formatter to logger #375
feat: Add bring your own log formatter to logger #375
Conversation
libraries/src/AWS.Lambda.Powertools.Logging/Internal/PowertoolsLogger.cs
Show resolved
Hide resolved
libraries/src/AWS.Lambda.Powertools.Logging/Internal/PowertoolsLogger.cs
Outdated
Show resolved
Hide resolved
try | ||
{ | ||
var logObject = logFormatter.FormatLogEntry(logEntry); | ||
if (logObject is not null) |
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.
In what scenarios can logObject be null?
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.
if custom formatter implemented by user for any reason returns null as output.
{ | ||
foreach (var (key, value) in CurrentScope.ExtraKeys) | ||
{ | ||
if (!string.IsNullOrWhiteSpace(key)) |
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.
don't think is possible at runtime to have an empty or null key for dictionary, this could be simplified with logEntry.ExtraKeys = new Dictionary<string, string>(extraKeys);
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.
It cannot, I can remove the null check but is's an append to the existing dictionary, so cannot be replaced.
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 have replaced it with a new throw new LogFormatException (custom exception)
case LoggingConstants.KeyCorrelationId: | ||
logEntry.CorrelationId = value as string; | ||
break; | ||
default: |
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.
In here we are changing the premise that what is logged should be exactly what is returned from the formatter.
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.
We don't change the object returning from formatter and we log it as is. This is not for the object returned from the formatter, it's for creating an LogEntry to pass it to the formatter.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
GTM. Great work Amir. I think there is room for improving the code in the future, specifically in PowertoolsLogger line 300 to line 330, those loops and checks could be improved.
Issue number: #366
Summary
This PR allows users to being their own log formatter
Changes
No changes to exiting functionalities only adding a new feature.
User experience
CustomLogFormatter implementation
Example CloudWatch Logs excerpt
Checklist
Please leave checklist items unchecked if they do not apply to your change.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.