SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
755 stars 223 forks source link

New Rule Idea: Do not convert collections to strings when using Microsoft.Extensions.Logging #9292

Open bdovaz opened 1 month ago

bdovaz commented 1 month ago

To keep the logs structured and a collection to be a collection (and not a collection converted to string) it is important not to do:

logger.LogInformation("My data: {data}", string.Join(", ", list));

And instead do:

logger.LogInformation("My data: {data}", list);

Here you can see that internally it already does this for us:

https://github.com/dotnet/runtime/blob/087e15321bb712ef6fe8b0ba6f8bd12facf92629/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs#L275

And in turn it respects the parameter without converting it to string only to represent it in the log message.

I have no experience in analyzers so I do not know to what extent it can be detected.:

Example 1:

logger.LogInformation("My data: {data}", string.Join(", ", list));

Example 2:

string data = string.Join(", ", list);

logger.LogInformation("My data: {data}", data);
sebastien-marichal commented 1 month ago

Hello @bdovaz,

I like the idea; I see its value in your first example, but less so in the second one. My understanding of the implementation is that the collection will be joined using commas anyway.

I don't have a lot of experience with structured logging, so I might miss some key points. Detecting the issue in your second example may complicate the rule implementation, and, from my point of view now, it is not worth it.

Could you elaborate if you think it is important that we should also detect the second scenario? Is there any scenario where we would want to format the collection another way than using commas?

Thank you,

bdovaz commented 1 month ago

I like the idea; I see its value in your first example, but less so in the second one. My understanding of the implementation is that the collection will be joined using commas anyway.

Yes, but the difference is that if you do a string.Join on your own the collection would go as a string field, but if you don't do it and delegate to the linked code from Microsoft library, those fields that you send are not lost for later processing in, for example, OpenSearch.

I don't have a lot of experience with structured logging, so I might miss some key points.

Context: https://messagetemplates.org/

Could you elaborate if you think it is important that we should also detect the second scenario? Is there any scenario where we would want to format the collection another way than using commas?

The second example was simply a possible case, the priority is the first one because if you see the code that I have put from Microsoft, it is clear that it does it for us and if we do it the real problem is that the fact of having those elements of the list separately at the time of processing the data is lost.

sebastien-marichal commented 1 month ago

Thank you, @bdovaz; I think there is value in implementing this rule.

We will need to have a specification sprint to evaluate its impact so we can prioritize it.

bdovaz commented 1 month ago

Ok, thanks!