36

I have been following Logging in ASP.NET Core Which is working just fine.

I have a question about this line

_logger.LogWarning(LoggingEvents.GetItemNotFound, "GetById({ID}) NOT FOUND", id);

I am wondering why they are not using $ - string interpolation?

_logger.LogWarning(LoggingEvents.GetItemNotFound, $"GetById({ID}) NOT FOUND");

Why would the LogWarning extension have a params object[] args paramater?

Whats the point when you can just send everything in string message.

I assume there is a reason for this but i haven't been able to find an explanation anywhere. I am wondering which method i should be using in order to log properly in .net core.

Linda Lawton - DaImTo
  • 106,405
  • 32
  • 180
  • 449
  • Logging pre-dates string interpolation. – Ben Sep 06 '18 at 09:10
  • 1
    Who nose? Probably because they want to provide support for people not on C# 6? And because existing logging frameworks have that `params object[]` overload? – CodeCaster Sep 06 '18 at 09:10
  • Also, this is in line with the definitions of various console methods, such as `Console.WriteLine`. Again, those methods didn't have string interpolation back then. – Nisarg Shah Sep 06 '18 at 09:11
  • String interpolation does work, as long as there's an `ID` property or variable in context. Of course, that means that you *lose* all information about context, properties, variables etc and have only that one string that needs parsing to find out what it contains. – Panagiotis Kanavos Sep 06 '18 at 09:12
  • You asked two question (or three). 1) Why not log only strings? Because the *parameters* matter, because the string is actually just a description and the real actionable information is part of the category, event, severity, correlation parameters. Logging frameworks can use that information to select targets, *message strings* etc. Semantic logging also uses them – Panagiotis Kanavos Sep 06 '18 at 09:14
  • 2) Doesn't string interpolation work here? It does. If there's an `ID` variable ` $"GetById({ID}) NOT FOUND"` is equivalent to `String.Format("GetById({0}) NOT FOUND",ID)`. – Panagiotis Kanavos Sep 06 '18 at 09:16
  • 3) Why didn't they use the string interpolation syntax that EF Core uses? Well, have you seen how many *bugs* and unintended string injections that caused? – Panagiotis Kanavos Sep 06 '18 at 09:18
  • @PanagiotisKanavos yeah i went digging in the source code for .net core to see if i cold figure out why it had the args. I thought adding that would improve the question and prove that i have done my research. – Linda Lawton - DaImTo Sep 06 '18 at 09:18
  • @DaImTo have you tried EF Core? It uses string interpolation. Lots of SO questions that wonder why they ended up with an invalid query instead of a paremeterized query. – Panagiotis Kanavos Sep 06 '18 at 09:20
  • @DaImTo sorry, correction. We aren't discussing string interpolation to generate a single string here, that's just bad practice that loses info. It's [FormattableString](https://learn.microsoft.com/en-us/dotnet/api/system.formattablestring?view=netframework-4.7.2) we are talking about. Why didn't the logging team use that like EF Core did, to pass the message template and data as a single parameter? It's too to break and can confuse even people that *do* know about the risks – Panagiotis Kanavos Sep 06 '18 at 09:22
  • @PanagiotisKanavos Have you considered posting an answer, instead of posting 6 comments? – user247702 Sep 06 '18 at 09:47
  • @Stijn writing for the last half hour. Then I find that Serilog's author posted a great post explaining why this is a bad idea 3 years ago!!!!! Now rewriting – Panagiotis Kanavos Sep 06 '18 at 09:49
  • 1
    Here's a solution [to use string interpolation with Serilog](https://stackoverflow.com/questions/61816775/c-sharp-serilog-how-to-log-with-string-interpolation-and-keep-argument-names-in/61816776#61816776) and not to lose structured logging benefits. – Artemious May 15 '20 at 10:19

4 Answers4

32

I suspect the question can be rephrased to :

Why didn't they provide overloads that accept a FormattableString to pass message templates and parameters using string interpolation syntax, like EF Core does for parameterized queries?

I'd say they got it right. At this point in time using FormattableString offers minimal benefits but creates a lot of confusion.

I just found that Serilog's author explains why this isn't such a good idea even though a semantic logging library looks like a natural fit for this scenario

Semantic Logging

One can argue that FormattableString would be a great addition to semantic logging libraries like Serilog. In this case an interpolated string does lose important information.

The call

Log.Information("Logged in {UserId}", loggedInUserId);

Won't just log a string based on the template, it will keep the names and values of the parameters and provide them to filters and targets. Wouldn't it be great to get the same result with :

Log.Information($"Logged in {loggedInUserId}");

Serilog's author doesn't think so and explains that :

  1. A good variable name is not necessarily a good property name
  2. Holes don’t always have obvious names, eg Log.Information($"Enabling categories {new[]{1, 2, 3}}");

and concludes that

String interpolation is a great feature, one I’ve looked forward to in C# for a long time. The idea of providing direct support for Serilog is a very interesting one and worth exploring, but I’m increasingly convinced it’s unnecessary.

Interpolation is nice when it keeps code DRY, cutting out the redundant clutter of {0} and {1} and preventing parameter mismatches.

In the case of Serilog, I think it’s incorrect to consider the property names like {UserId} as redundant; in a well-implemented logging strategy they’re an incredibly important part of the picture that deserve their own consideration. You wouldn’t let variable names determine the table and column names in a relational database – it’s exactly the same trade-off being considered here.

Original explanation

That's one of the most controversial features of EF Core actually, and can easily result in the very SQL injection and conversion problems one wants to avoid by using parameters.

This call :

string city = "London";
var londonCustomers = context.Customers
    .FromSql($"SELECT * FROM Customers WHERE City = {city}");

calls FromSql(FormattableString) and will create a parameterized query :

SELECT * FROM Customers WHERE City = @p0

And pass London as a parameter value.

On the other hand this :

string city = "London";
var query=$"SELECT * FROM Customers WHERE City = {city}";
var londonCustomers = context.Customers.FromSql(query);

calls FromSql(string) and will generate :

SELECT * FROM Customers WHERE City = London

Which is invalid. It's far too common to fall in this trap even when you do know about the risk.

It doesn't help at all when you already have predefined queries or messages. This usage is far more common in logging, where you (should) use specific message templates defined in well known locations, instead of sprinkling similar looking strings in every log location.

One could argue that this addition made EF Core 2.0 somewhat safer because people had already started using string interpolation in EF Core 1.0, resulting in invalid queries. Adding the FormattableString overload the EF Core team was able to mitigate that scenario while making it easier to accidentally cause a different problem.

At this point in time the logging designers decided to avoid this confusion. Logging a raw string doesn't have such catastrophic consequences.

Community
  • 1
  • 1
Panagiotis Kanavos
  • 120,703
  • 13
  • 188
  • 236
  • We use Nlog but i wouldn't be surprised if this doesn't hold for that as well. – Linda Lawton - DaImTo Sep 06 '18 at 10:06
  • 2
    @DalmTo the Serilog article points to another problem. [FormattableString doesn't capture the property names](https://github.com/dotnet/roslyn/issues/142) . You can only get arguments [by position](https://learn.microsoft.com/en-us/dotnet/api/system.formattablestring.getarguments?view=netframework-4.7.2#System_FormattableString_GetArguments). This means that adding `FormattableString` to the generic Logging abstraction would cause problems when used with any library that cares about properties – Panagiotis Kanavos Sep 06 '18 at 10:10
  • SQL Injection in a log file? It has only to log text. Show me the logging framework which cannot handle a formatted text insert into a database automatically. The only answer to this question is performance. – FrankM Sep 06 '18 at 10:30
  • @FrankM you misunderstood what I wrote then. Serilog's author explains why it's *not* about performance. It's about losing valuable info: the properties and their values. Logging doesn't just log strings, it uses properties and their values for filtering, selecting targets, correlations. – Panagiotis Kanavos Sep 10 '18 at 08:44
  • 2
    @FrankM SQL Injection is an *example* from another service that *does* use interlopation and ended up causing trouble: EF Core. – Panagiotis Kanavos Sep 10 '18 at 08:45
  • 1
    @PanagiotisKanavos Serilogs author did not write anything about performance - pro or con. The other part I do now understand, the names of the parameters are replacing the string.Format's {0} {1}... for semantic evaluation of the given values. Thanks for explaining. – FrankM Sep 10 '18 at 15:41
  • @FrankM Tangential, but security concerns don't just stop with SQL injection. Being able to inject unescaped user-provided characters into a log is also a [security vulnerability](https://www.owasp.org/index.php/Log_Injection). – Seafish Jan 02 '20 at 21:13
  • @Seafish Link is broken. I'm dying to learn why on earth that would be the case. – John Apr 16 '20 at 19:43
  • @John Here's an [updated link to the OWASP page](https://owasp.org/www-community/attacks/Log_Injection). The Google-able term is "log forging" or "log injection" if you'd like to learn more from different sources or if the link breaks again. – Seafish Apr 16 '20 at 23:27
  • 1
    Here's a solution to use string interpolation with Serilog and not to lose structured logging benefits. https://stackoverflow.com/questions/61816775/c-sharp-serilog-how-to-log-with-string-interpolation-and-keep-argument-names-in/61816776#61816776 – Artemious May 15 '20 at 10:21
  • 1
    I've just published a library (with extensions to Serilog, NLog, and Microsoft ILogger) which allows writing log messages using interpolated strings and yet defining the property names through anonymous objects. Would love to hear some feedback: https://github.com/Drizin/InterpolatedLogging – drizin Apr 11 '21 at 00:30
  • Entity Framework Disagrees with the statement "You wouldn’t let variable names determine the table and column names in a relational database – it’s exactly the same trade-off being considered here." – Hunter Web Apps Oct 03 '22 at 13:57
  • @HunterWebApps no it doesn't. EF can't change how databases work. If you think so I can guess you're using `Set<>()` assuming it doesn't do what a `DbSet<>` property already does - configure and cache the metadata of a specific entity. In fact, before EF Core 6 you'd be paying the configuration penalty on every single call. You may even be using the "generic repository" antipattern. A DbContext is a multi-entity domain-level repository and Unit-of-Work, not a database connection or model or database driver. – Panagiotis Kanavos Oct 03 '22 at 14:01
  • @HunterWebApps you *can't* have a SQL query where table and column names are parameters. Tables and columns are like types in a strongly typed language. The database engine compiles the SQL query into an execution plan using the actual table and column types, indexes *and* data statistics. That execution plan can use parameters for *values*, not tables and columns. EF doesn't change that, it generates SQL queries with concrete table and column names based on the already configured mappings. – Panagiotis Kanavos Oct 03 '22 at 14:09
19

I know this is 4 years old but I don't feel any of the answers provided are really correct.

The reason is structured logging, if you use string interpolation you are just logging a single string, with structured logging you log the variables seperatly so you can access them more easily.

Imagine you have a website and you want to alert or provide a report on how long a page takes to load. You log it like this logger.LogDebug($"{PageName} took {ms}ms to load."); All your log can contain is "Index took 53ms to load."

If you log it like this instead logger.LogDebug("{PageName} took {ms}ms to laod.", PageName, sw.ElapsedMilliseconds); Then depending on your logging provider you can access all the properties seperatly and group by the PageName easily and filter all load times over 50ms. Without having to revert to Regular Expressions. E.g. SELECT PageName, Count(*) FROM Logs WHERE LogText = '{PageName} took {ms}ms to laod.' and ms > 50 GROUP BY PageName

Matt
  • 1,436
  • 12
  • 24
  • 2
    Great example, makes you think about how logs are queries in third party services like New Relic, CloudWatch etc. – adR Jun 15 '22 at 00:46
15

At least two reasons.

First, logging pre-dates string interpolation, and Microsoft have not yet invented a time machine. String interpolation was only introduced in C# 6 in July 2015, but the logging methods follow the same pattern used in Microsoft.Build.Utilities since dotnet framework 2.0.

Second, Performance. If string interpolation is used and a string is passed as a parameter, then the interpolation is done before the call to Log. However not every call to Log results in something being logged - it depends on the configuration.

If you log something at DEBUG level and your current configuration is for INFORMATION level, the it is a waste of time to do string interpolation, you can just say "Thanks, but no thanks", and return immediately after doing nothing with the arguments.

Expanding on Second, Performance Internally, the most logger look basically like this:

void LogDebug(string Message, params object[] args){
    if(this.DebugEnabled){
        Log.Write(string.Format(Message,args)); 
    }
}

// 1 with parameters
LogDebug("GetById({ID}) NOT FOUND", id);
// 2 interpolated
LogDebug($"GetById({id}) NOT FOUND");

So if Debug is not enabled, one less interpolation operation is done.

Ben
  • 34,935
  • 6
  • 74
  • 113
  • 1
    A far more important reason is that it's a bad idea. EF Core uses string interpolation for parameterized queries. The result: a lot of unintended SQL injection problems. Imagine extracting the the interpolation to a separate line. Instead of a parameterized query you get an injected string – Panagiotis Kanavos Sep 06 '18 at 09:18
  • 6
    The interpolated string isn't constructed from `FormattableString` until the conversion to `string` happens, if ever. So there's no performance penalty. Also, the first point explains why they didn't provide the feature initially. It doesn't explain why they *still* don't provide an overload taking a `FormattableString`. – John Apr 16 '20 at 19:38
  • That's correct. I've developed this library which converts `FormattableString` into message-template (it allows us to give meaninful-names to the interpolated variables), and according to my benchmarking tests the cost of creating FormattableString is irrelevant, and the cost of converting a FormattableString into a string is almost the same of building the strings in other ways. To sum, FormattableString is not as ugly as some people think. Take a look at benchmarks here: https://github.com/Drizin/InterpolatedLogging – drizin Apr 16 '21 at 01:47
0

The reason in my opinion is that .NET is using multiple levels of logging.

Trace = 0, Debug = 1, Information = 2, Warning = 3, Error = 4, Critical = 5 and None = 6.

There are differences between below codes although they look similar.

Structural logging

Logger.LogInformation("Hello {Name}", myName);

and

String Interpolation

Logger.LogInformation($"Hello {myName}");

String interpolation under the hood is identical to the use of string.Format(). Which means string.Format will always be executed disregarding the log level and pass the formatted string to the logger.

On the other hand, when structural logging is used, the string will only be formatted as per the log level is met.

alex
  • 614
  • 1
  • 6
  • 15