Suchiman / SerilogAnalyzer

Roslyn-based analysis for code using the Serilog logging library. Checks for common mistakes and usage problems.
Apache License 2.0
309 stars 29 forks source link

New rule proposal: Log event messages should be fragments, not sentences #51

Open prlcutting opened 4 years ago

prlcutting commented 4 years ago

I've seen it written that log messages should be fragments, and not sentences, and therefore we should avoid including a dot/period at the end of a log message, e.g.:

var firstName = "John";

Log.Information("First name is {FirstName}.", firstName);  // Bad - trailing period
Log.Information("First name is {FirstName}", firstName);  // Good - no trailing period

How about a rule to catch this, and a quick-fix to remove the trailing period?

I'm basing this on Ben Foster's excellent Serilog Best Practices blog post (see Sentences vs. Fragments section). I'm sure I've also read similar from the Serilog man himself (@nblumhardt), but despite seaching, couldn't find a reference to cite.

nblumhardt commented 4 years ago

:+1: this would be nice

prlcutting commented 4 years ago

@Suchiman - would you be open to including this new rule proposal in your SerilogAnalyzer project? If so, would you be interested in a PR for it?

Suchiman commented 4 years ago

@prlcutting Yes, i'll gladly take a PR!

Balfa commented 2 years ago

I too have seen Ben Foster's blog post (and several other mentions of this best practice that all point to his blog post as the source of truth), but can anyone (@nblumhardt ?) explain in a little more detail a practical example of why this is a bad thing? Am I supposed to also not start log messages with a capital letter?

nblumhardt commented 2 years ago

@Balfa 👋 Having a consistent convention makes it more pleasant to read logs created by multiple developers; the details of the convention itself are mostly just opinion. I'm pretty sure I picked this up from older codebases and others I worked with.

When log messages get short and pithy I think the periods are just distracting:

Starting up.
Unhandled exception.

vs.

Starting up
Unhandled exception

YMMV :-)

Balfa commented 2 years ago

Thanks, that makes sense 🙂 It still seems a bit weird how we use capital letters at the start of log fragments, but not full stops at the end.

rcdailey commented 2 years ago

I struggle with this rule a lot. It doesn't make sense to me. Maybe that's because of my use case.

My application is a .NET console app. I have two log sinks: File and console (stdout):

Especially for the console output, I need my logs to be human vs machine readable. I don't really benefit from the structural aspects of the logs that Serilog provides. Here's an example of a piece of code that checks for some invalid configuration. For each piece of invalid configuration, I log some diagnostic information. At the end, I provide a helpful message to the user that guides them on how to address the issues. This is a combination of two sentences.

if (_guideProcessor.DuplicateScores.Any())
{
    foreach (var (profileName, duplicates) in _guideProcessor.DuplicateScores)
    foreach (var (trashId, dupeScores) in duplicates)
    {
        _log.Warning(
            "Custom format with trash ID {TrashId} is duplicated {Count} times in quality profile " +
            "{ProfileName} with the following scores: {Scores}",
            trashId, dupeScores.Count, profileName, dupeScores);
    }

    _log.Warning(
        "When the same CF is specified multiple times with different scores in the same quality profile, " +
        "only the score from the first occurrence is used. To resolve the duplication warnings above, " +
        "remove the duplicate trash IDs from your YAML config.");

    _console.Output.WriteLine("");
}

I obviously get an analysis warning on the second Warning() log in my example above. But I don't see what I'm doing wrong here. Am I violating best practices? Are logs not meant to be written this way in Serilog? I'm not sure what to think and the analyzer just confuses the situation even more.

wolf8196 commented 1 year ago

I wouldn't pretend to know all the history of Serilog, but in regards to logging in general - the rule doesn't make sense to me. What do you mean messages are fragments, not sentences? IMO they absolutely are sentences for other humans to read. You start with a capital letter, follow basic grammar & punctuation so that other people are not put off by what you wrote.

The only argument for following that rule is if it's very widespread in general & followed not only by those who use Serilog. Then it would be the issue of consistency. But I highly doubt that it is so widespread (but again, not an expert, maybe it is that widespread).