Localize log messages in controllers

[hishamco]

Two weeks ago we merged #954 which localize the log messages in a centeral place, but few days ago @sbwalker remind me the localization should support external modules as first class citizen, then I realize what I did in that PR need to be fixed in a right way. With this PR module creator are able to create thier RESX files that contains log messages as well

[hishamco]

@sbwalker just one question about structured logs, if we take the following as example

_logger.Log(LogLevel.Information, this, LogFunction.Delete, _localizer["Job Deleted {JobId}"], id);

I think we could remove the log param and localize it like

_logger.Log(LogLevel.Information, this, LogFunction.Delete, _localizer["Job Deleted {0}", id]);

which is I think is right, if nothing break here, I should replace all the log params except the one which hold a complex object, Am I right?

[PavelVsl]

I have question about log messages localization. To which language will be log messages translated? Admin? Current user?
Default site culture?
I have feeling that it is current user culture. In multilanguage site result will be multilanguage log ?? every line in different language? Maybe I'm wrong, but poor admin if he must read messages.

[hishamco]

The log messages by default in English, but whenever you added a supported culture with a proper RESX the logs will start to be localised

I have feeling that it is current user culture

Yes

In multilanguage site result will be multilanguage log ??

Yes, this is how localization works

[sbwalker]

@hishamco in regards to your question about log messages:

_logger.Log(LogLevel.Information, this, LogFunction.Delete, _localizer["Job Deleted {JobId}"], id);

Oqtane uses the same approach as Serilog where the properties in the log message are actually valuable content as they provide context for the property values. You can see this in the following screenshot from the Event Log:

So we definitely should not change the property names to numeric tokens such as {0} as we will lose all of this context

[sbwalker]

@chlupac thank you for raising this item. Because the content of the vent log contains many internal system details, the audience for the Event Log is restricted to Host users. So unless Host users can read the content of the Event Log entries, they are completely useless. For this reason, event log entries must not be stored in the language of the current user - they should be stored in the default language for the installation. @hishamco I assume this makes sense to you?

[hishamco]

@sbwalker what if the RESXs for the installation culture are missing?

[sbwalker]

Then it would fallback to the hard coded log text - which is English.

[hishamco]

The main issue here is how to restrict the localizer to the default culture, coz localizer will look for the proper RESX and use it

IMHO leaving this as how ASP.NET Core handle localization and write some docs as reminder to create localization resources for the logs in case to be localized

[PavelVsl]

I think than log should be stored in invariant language (English) and localized using template and data in moment of view to observer language. Translation of log event in moment of creation to current user language is wrong because log entry is stored for future ADMIN use.

IMHO:

• Store log messages in English (template +data)
• In log viewer translate message template for current user, inject data and display localized message

[sbwalker]

I agree with @chlupac The focus should not be on how .NET Core handles localization but who the audience is for the specific content and how it is expected to be used within the application. In the case of logs it is only Host users that will have access and it is important that the content is consistently stored in a single language. I should have recognized this sooner but what we are discussing here is content localization - not static localization. Logs are content that is produced by the system. Content localization is not in scope for our current efforts. We should just ignore Localization when it comes to logging and retain the existing functionality where log messages are stored in English. I apologize that this means rolling back a variety of changes that were already committed for logging - but we need to have the correct solution for our users. Note that providing dynamic translation of content like @chlupac is suggesting is a good idea but is a completely different feature that would be part of content localization.

[hishamco]

Oqtane uses the same approach as Serilog where the properties in the log message are actually valuable content as they provide context for the property values. You can see this in the following screenshot from the Event Log:

Ya, this is similar to Serilog, but if you look closely to the screenshot above the properties could be important if we have a complex object, but in case the log associate with one property I don't think there's a difference if we pass a property or inject its value directly into the log message

Rearding the logs I don't see any issue with localize the messages, we already something similar in Orchard Core, perhaps here the Host have access to these logs, nothing but if someone use other than the database logger these logs can be visible, so me as developer or IT guy should see them in my prefered culture, Am I right? But if you see that we need a time for a proper solution I can revert the changes in this PR, then we can say the localization is almost done if it's not completely done ;)

Thanks

[sbwalker]

If you look at the screen shot, PageId is a good example. It is not a complex object - it is just an integer. However the property name of “PageId” is parsed from the log template message itself. If the log template message contained {0} as you suggested then the property would say 0:3 instead of PageId:3 - making the logging a lot less meaningful and useful.

Orchard Core is a completely different framework with different goals and may have a different audience for its logs - which is why localization may make sense in its case but not for Oqtane. DNN’s architecture is similar to Oqtane... and DNN does not localize it’s log messages for the exact reasons which chlupac and I outlined above. I think the best approach at this point is to not localize log messages.

[hishamco]

If the log template message contained {0} as you suggested then the property would say 0:3 instead of PageId:3 - making the logging a lot less meaningful and useful.

The log message will not contains any placeholder because it will be replaced by the localizer, anyhow let me revert my changes and not localize the logs for now