Skip to content

US354240: Provide CAF Logging support for Serilog#20

Open
xbreizh wants to merge 76 commits intomasterfrom
US354240
Open

US354240: Provide CAF Logging support for Serilog#20
xbreizh wants to merge 76 commits intomasterfrom
US354240

Conversation

@xbreizh
Copy link
Copy Markdown

@xbreizh xbreizh commented Feb 9, 2022

@xbreizh xbreizh self-assigned this Feb 9, 2022
@buildmachine-sou-jenkins2
Copy link
Copy Markdown
Contributor

@dermot-hardy dermot-hardy changed the title US354240: Provide CAF Logging support for .NET 6 US354240: Provide CAF Logging support for Serilog Feb 17, 2022
@xbreizh xbreizh requested review from frankmccarry and removed request for frankmccarry May 9, 2022 12:39
@xbreizh
Copy link
Copy Markdown
Author

xbreizh commented May 10, 2022

@frankmccarry I added the file configuration and it performs as expected. The only downside is that we have to pass the filepath to the configuration as it's a requirement from WriteTo.File . I tested with a default name, but then the template doesn't get applied onto the file mentioned in .ReadFrom.Configuration(configuration).

Here is a sample of call:

var config_path = "C:\\Users\\XLamourec\\IdeaProjects\\caf-logging\\ConsoleApp1\\appsettings.json";
var configuration = new ConfigurationBuilder()
           .AddJsonFile(config_path)
           .Build();

 var levelSwitch = new LoggingLevelSwitch();
 levelSwitch.MinimumLevel = LogEventLevel.Verbose;
 Log.Logger = CafLoggingLoggerConfiguration
     .GetLoggerConfig(levelSwitch, "testlog.txt")
     .ReadFrom.Configuration(configuration)
     .CreateLogger();

@xbreizh xbreizh requested review from frankmccarry and removed request for frankmccarry May 10, 2022 14:09
Copy link
Copy Markdown

@frankmccarry frankmccarry left a comment

Choose a reason for hiding this comment

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

I think the changes meet the remit for the ticket.

@dermot-hardy dermot-hardy self-requested a review May 10, 2022 15:09
{
if (char.IsControl(c))
{
return true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So we're saying that if the message being logged contains a control character that it is safe to log!!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Reversed the logic

// http://www.csc.villanova.edu/~tway/resources/ascii-table.html
foreach (char c in message)
{
if (char.IsControl(c))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This check doesn't relate to the comment above. Are we checking if the characters are ASCII, or are we checking if they are control characters?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I updated to match what we do here Note that we consider non-latin characters as unsafe.


## Examples

`[2022-05-10 13:29:45.444Z #217.001 WARN john-tenant Cidi] c.m.d.f.f.f.FieldFullTextFixer: "Greek test: κόσμε"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does the strings being logged have quotes around them? And also why are there extra quotes in the strings in the message field?

I.e. I tried this: Log.Error(ex, "Caught an exception");

I expected: {"message":"Caught an exception","exception":"..."}
But I got: {"message":"\"Caught an exception\"","exception":"..."}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed


## Pattern

`[{@t:yyyy-MM-dd HH:mm:ss.fffZ} {Tid(ProcessId,ThreadId)} {Log(@l):5} {Sanitize(tenantId, 12, 12)} {Sanitize(correlationId, 4, 4)}] {Sanitize(logger, 30, 30)}: {MaybeJsonMsgAndEx(@m,@x)}\n`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • When I tested this I did not see the time coming out in UTC.
  • Tenant Id and Correlation Id are not defaulting to a dash if they are not present

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Both should be fixed now. I am getting: [2022-05-13 15:10:53.160Z #272.001 ERROR - -] c.m.d.f.f.f.FieldFullTextFixer: {"message":"{FirstName"}

# CAF Logging Serilog
This project configures Serilog to meet the CAF Logging Standard.

The log level is controllable via value that can be provided on Logger initialization. The default used is `INFO`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we control the log level by setting the CAF_LOG_LEVEL environment variable the same way we do with the Logback and the Log4j 2 versions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

@xbreizh xbreizh requested a review from dermot-hardy May 13, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants