42

I understand the concept of this warning (similar to this question), but what is wrong with this code?

    private async Task LogWarningAsync(short? userCodeId, string message, params object[] args)
    {
        _logger.LogWarning(message, args);

        // Do something on the database...
    }

The warning:

CA2254

The logging message template should not vary between calls to 'LoggerExtensions.LogWarning(ILogger, string?, params object?[])'

Peet Brits
  • 2,911
  • 1
  • 31
  • 47
  • I'm guessing simply because of the fact that `message` is a variable. Why are you calling a method which runs `_logger.LogWarning`? Why can't you use `_logger.LogWarning` directly with a compile time log template? Also, your method `LogWarningAsync` _also_ makes database calls? – gunr2171 Feb 02 '22 at 12:53
  • @gunr2171 perhaps `LogWarningAsync()` is badly named, but the situation is that in this class some sensitive log messages have to go to both the log file and a system log database table (a rare situation). At the time of writing, before this warning, the two were combined for the obvious reasons of reducing code duplication and not wanting to accidentally forget about the log. – Peet Brits Feb 02 '22 at 13:22
  • Suppose I split this into two calls: `_logger.LogWarning('My sample message'); await LogWarningAsync('My sample message');` every time this method gets called, then I will have to duplicate the input message and arguments, which I'm not too excited to do. – Peet Brits Feb 02 '22 at 13:28
  • _logger.LogWarning("{message} arguments: {args}", message, args); i think it will work – lda573 Sep 06 '22 at 13:36

5 Answers5

25

Here is a discussion of other people experiencing similar issues regarding CA2254. Hopefull this will get addressed in future versions.

For the time being, my best solution is to ignore the warning.

    private async Task LogWarningToDatabaseAsync(short? userCodeId, string message, params object[] args)
    {
#pragma warning disable CA2254 // Template should be a static expression
        _logger.LogWarning(message, args);
#pragma warning restore CA2254 // Template should be a static expression

        // Do something on the database...
    }

The alternative is not very exciting.

    private async Task LogWarningToDatabaseAsync(short? userCodeId, string message, params object[] args)
    {
        // Move to parent.
        //_logger.LogWarning(message, args);

        // Do something on the database...
    }

    private async Task SampleAsync(short? userCodeId, string aaa, string bbb)
    {
        // I'm not happy about repeating the input message every time this gets called.
        _logger.LogWarning("My sample message with data {aaa} and more data {bbb}", aaa, bbb);
        await LogWarningToDatabaseAsync(userCodeId, "My sample message with data {aaa} and more data {bbb}", aaa, bbb);
    }
Peet Brits
  • 2,911
  • 1
  • 31
  • 47
  • 1
    The string interpolation causes the warning. So, passing variables like string.Format works as shown by @peet-brits in the method Sample task works and removes the warning. – Rahul Mahadik May 09 '22 at 06:19
  • 2
    You should not disable this warning, since it's a valid warning due to string interpolation which cannot be grouped if the log is not in the same template. – Verbe Nov 07 '22 at 10:12
  • @Verbe - like I said, "For the time being". The linked issue on github is still open. This answer does not suggest disabling the warning, only ignoring this individual exceptional case. – Peet Brits Nov 07 '22 at 14:26
  • @Verbe What do you mean by "cannot be grouped"? – Josh Sutterfield Jul 14 '23 at 23:32
23

Do this:

_logger.LogWarning("{Message}", message);

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2254

Nakres
  • 1,212
  • 11
  • 27
18

This is actually a relevant warning. When producing logs within your application, you should not include variables to construct your log message. For example, "User 128973 logged in" is not a good log because you will not be able to group all of these "logged in" logs together to produce stats. Instead, you should put the userid in a separate object in your log object (additional data)

Yoann Diguet
  • 249
  • 1
  • 5
  • The situation I'm dealing with is when the variable is "User {UserId} logged in", which is not the same issue. – Peet Brits Mar 18 '22 at 06:23
  • well, that's what Event Ids are for. Who cares what the log message is if you give it an ID? You can just group by ID. – Andy Dec 09 '22 at 16:20
  • 3
    Let _me_ be the judge of what I can, will or need to group when _I_ analyse my logs. This warning just pushes me towards using another logging solution. – Jonas Äppelgran Jun 12 '23 at 08:44
7

You really shouldn't suppress this analysis rule here, it's correct in your case to flag. Your use case scenario of wanting to log both to a DB table and to a log file should not be resolved by wrapping the regular logging system. It should be resolved by adding a logging output to your logger which logs to the table in question. Yes, it can be filtering and not log everything. Look up how to best do it with your logging library of choice.

Philip Borgström
  • 146
  • 1
  • 8
  • I hear you, but I'm not logging to the DB as a rule. This is a special case with special functionality. Another place where this warnig will occur is when the message is defined in some or other resource file. This should also be considered correct usage. – Peet Brits May 01 '22 at 00:15
  • Ideally you would still define that logging rule in your logging library. Alternatively you don't take a string but an enum and typed object to get it properly structured. When the message is defined in a resource or somewhere else that's loaded dynamically one really should use https://learn.microsoft.com/en-us/aspnet/core/fundamentals/logging/loggermessage – Philip Borgström Jun 16 '22 at 07:14
1

Add this to your project file in an item group.

<NoWarn>$(NoWarn),CA2254</NoWarn>
Luke Puplett
  • 42,091
  • 47
  • 181
  • 266