SLVS-1723 Introduce log contexts #5913

georgii-borovinskikh-sonarsource · 2024-12-19T13:16:30Z

src/Integration.Vsix/CFamily/VcxProject/FileConfigProvider.cs

gabriela-trutan-sonarsource

I am not really happy with the design decisions as we are creating a lot of instances of the logger and the context, which will increase the memory usage and the performance overhead especially because logging is a very used operation.

I would rather make the LoggerBase and LoggerContextManager Mef exportable components with the creation policy non-shared.
Then the contextManager of a logger can be configured to hold a specific context or to enhance existing contexts.

src/Roslyn.Suppressions/Roslyn.Suppressions/Logger.cs

src/Integration.UnitTests/Helpers/SonarLintOutputTests.cs

gabriela-trutan-sonarsource · 2024-12-27T08:02:18Z

src/Core/Logging/LoggerFactory.cs

+[Export(typeof(ILoggerFactory))]
+[PartCreationPolicy(CreationPolicy.NonShared)]
+[method: ImportingConstructor]
+public class LoggerFactory(ILoggerContextManager loggerContextManager) : ILoggerFactory


The LoggerFactory is a level an abstraction that does not actually bring any value. It's just used in a test class and in the Container.cs.
Please remove it completely and all test classes related to it

LoggerContextManager implementation is not accessible to Container.cs (as it is an internal class). I think this is a better solution than making the class public. Also it means that users of LoggerBase don't need to mef-export the manager and it also allows to use Substitute (by returning ILogger)

src/Core.UnitTests/Logging/LoggerContextManagerTests.cs

gabriela-trutan-sonarsource · 2024-12-27T08:49:24Z

src/Core/Logging/LoggerBase.cs

+    }
+
+    private static StringBuilder AppendProperty(StringBuilder builder, string property) =>
+        builder.Append('[').Append(property).Append(']').Append(' ');


I think this is a lot more readable

Suggested change

builder.Append('[').Append(property).Append(']').Append(' ');

builder.AppendFormat("[{0}] ", property);

gabriela-trutan-sonarsource · 2024-12-27T08:49:46Z

src/Core/Logging/LoggerBase.cs

+        builder.Append('[').Append(property).Append(']').Append(' ');
+
+    private static StringBuilder AppendPropertyFormat(StringBuilder builder, string property, params object[] args) =>
+        builder.Append('[').AppendFormat(property, args).Append(']').Append(' ');


A lot more readable

Suggested change

builder.Append('[').AppendFormat(property, args).Append(']').Append(' ');

builder.AppendFormat("[{0}] ", new StringBuilder().AppendFormat(property, args));

Technically this is faster and has less allocations

Chars about 2x faster than double formatting and uses less memory. I don't think it's that unreadable.

gabriela-trutan-sonarsource · 2024-12-27T08:52:45Z

src/Core/Logging/LoggerBase.cs

+    private static StringBuilder AppendProperty(StringBuilder builder, string property) =>
+        builder.Append('[').Append(property).Append(']').Append(' ');
+
+    private static StringBuilder AppendPropertyFormat(StringBuilder builder, string property, params object[] args) =>


Please make the return type for both AppendProperty void. Or create extension methods for this class

What's the motivation for that?

The return type of AppendPropertyFormat is not used. Additionally, it feels awkward to receive a stringBuilder instance then return it unchanged.

Created extension methods

gabriela-trutan-sonarsource · 2024-12-27T09:39:53Z

src/Core/Logging/LoggerBase.cs

+    ILoggerWriter writer,
+    ILoggerSettingsProvider settingsProvider) : ILogger
+{
+    public ILogger ForContext(params string[] context) =>


Creating a new instance for each context feels like a very bad design decision as this will increase the memory usage and the performance overhead.

This is not a correct conclusion. The instances are lightweight (partially thanks to immutable lists and lazy string property initialization, but in general they don't hold a lot of data apart from a few object references and the aforementioned property list and formatted string). This will not increase the overhead in any meaningful way.

On top of that, most of these instances will be created in user-class constructors and therefore will be long-living.

It is a correct conclusion and it will bring more pressure to the garbage collector, which will impact the performance.

On top of that, most of these instances will be created in user-class constructors and therefore will be long-living.

This is correct for the LoggerBase instances. That's why I see no reason why it shouldn't be a Mef component with a non-shared creation policy (which will basically lead to the same thing but without the need of factories).
This is not true, however for the LoggerContextManager and I think we can find better design than just creating instances for each context.

Again, that's not true. It will, of course impact the performance, as pretty much every single added allocation or instruction does, but the question is will it impact it in any meaningful way? The answer is no. It has less of an impact on memory than the existing string allocations inside the logger.

Uncustomized logger context manager has references to 2 empty immutable lists and 2 non-initialized lazies...

Added log methods overloads for single-time-use contexts

gabriela-trutan-sonarsource · 2024-12-27T09:40:46Z

src/Core/Logging/LoggerContextManager.cs

+    public string FormatedContext => formatedContext.Value;
+    public string FormatedVerboseContext => formatedVerboseContext.Value;
+
+    public ILoggerContextManager CreateAugmentedContext(IEnumerable<string> additionalContexts) => new LoggerContextManager(contexts.AddRange(additionalContexts), verboseContexts);


Creating a new instance for each context feels like a very bad design decision as this will increase the memory usage and the performance overhead.

gabriela-trutan-sonarsource

I still have some suggestions, but the implementation seems better now

gabriela-trutan-sonarsource · 2025-01-07T15:29:42Z

src/Core/Logging/StringBuilderLoggingExtensions.cs

-        }
+internal static class StringBuilderLoggingExtensions
+{
+    public static StringBuilder AppendProperty(this StringBuilder builder, string property) =>


I would rename it

Suggested change

public static StringBuilder AppendProperty(this StringBuilder builder, string property) =>

public static StringBuilder AppendContextProperty(this StringBuilder builder, string property) =>

gabriela-trutan-sonarsource · 2025-01-07T15:30:08Z

src/Core/Logging/StringBuilderLoggingExtensions.cs

-            WriteLine(messageFormat, args);
-        }
-    }
+    public static StringBuilder AppendPropertyFormat(this StringBuilder builder, string property, params object[] args) =>


I would rename it

Suggested change

public static StringBuilder AppendPropertyFormat(this StringBuilder builder, string property, params object[] args) =>

public static StringBuilder AppendContextPropertyFormat(this StringBuilder builder, string property, params object[] args) =>

Log properties also include thread id, debug, possibly other things. Context is a word I used to specifically describe the log properties that are set for a certain logical scope

src/Core/Logging/StringBuilderLoggingExtensions.cs

src/Core.UnitTests/Logging/LoggerContextManagerTests.cs

src/Core.UnitTests/Logging/LoggerBaseTests.cs

gabriela-trutan-sonarsource · 2025-01-07T16:25:33Z

src/Core/Logging/ILogger.cs

+    /// <summary>
+    /// Logs a message and appends a new line.
+    /// </summary>
+    void WriteLine(string message);


Let's get rid of this overload to have less clutter in the interface

technically this overload is faster than the format overload, because string/sb format checks for {0} in the string even if the parameters list is empty

gabriela-trutan-sonarsource · 2025-01-07T16:25:44Z

src/Core/Logging/LoggerBase.cs

+            writer,
+            settingsProvider);
+
+    public void WriteLine(string message) =>


We could actually get rid of this overload.

gabriela-trutan-sonarsource · 2025-01-07T16:43:38Z

src/Core/Logging/ILogger.cs

+    void LogVerbose(string messageFormat, params object[] args);
+    void LogVerbose(MessageLevelContext context, string messageFormat, params object[] args);
+
+    ILogger ForContext(params string[] context);


Consider renaming these two methods (lines 38 and 40) with something like GetNewWithContext, CreateNewWithContext.

I think it's enough that it has a return type of ILogger, this implies that it's a new instance

… responsibility from LoggerBase to LoggerContextManager, added StringBuilderLoggingExtensionsTests

sonarqubecloud · 2025-01-08T12:14:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[SLVS-1723](https://sonarsource.atlassian.net/browse/SLVS-1723) [SLVS-1723]: https://sonarsource.atlassian.net/browse/SLVS-1723?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

Base automatically changed from feature/hardening-q4-6 to master December 20, 2024 10:16

georgii-borovinskikh-sonarsource changed the base branch from master to feature/hardening-q4 December 23, 2024 09:07

georgii-borovinskikh-sonarsource changed the base branch from feature/hardening-q4 to gb/collections-immutable-reference December 23, 2024 09:16

georgii-borovinskikh-sonarsource force-pushed the gb/log-context branch from 771a9bc to c098c1b Compare December 23, 2024 09:17

georgii-borovinskikh-sonarsource commented Dec 23, 2024

View reviewed changes

src/Integration.Vsix/CFamily/VcxProject/FileConfigProvider.cs Outdated Show resolved Hide resolved

georgii-borovinskikh-sonarsource force-pushed the gb/log-context branch from c098c1b to 6fdc8e7 Compare December 23, 2024 09:23

georgii-borovinskikh-sonarsource requested a review from gabriela-trutan-sonarsource December 23, 2024 13:49

georgii-borovinskikh-sonarsource marked this pull request as ready for review December 23, 2024 13:49

Base automatically changed from gb/collections-immutable-reference to feature/hardening-q4 December 24, 2024 09:44

georgii-borovinskikh-sonarsource force-pushed the gb/log-context branch from ba2069e to d01d7dd Compare December 24, 2024 09:45

gabriela-trutan-sonarsource requested changes Dec 27, 2024

View reviewed changes

georgii-borovinskikh-sonarsource deleted the branch feature/hardening-q4 December 27, 2024 14:13

georgii-borovinskikh-sonarsource closed this Dec 27, 2024

georgii-borovinskikh-sonarsource reopened this Dec 27, 2024

georgii-borovinskikh-sonarsource added 12 commits January 6, 2025 09:36

SLVS-1723 Introduce log contexts

100b767

Add verbose contexts + refactoring

b7fbb2a

refactor loggers

cae09f3

more fixes and refactoring

0f0e2fa

add some

ae21a2b

add license headers

b090724

Add LogContextManagerTests

4f3592a

rearrange code in TestLogger

af52314

renames

a02aa00

Add LoggerFactoryTests

d5f91d9

Add LoggerBaseTests

b364094

change format

dc2b4c0

georgii-borovinskikh-sonarsource force-pushed the gb/log-context branch from d01d7dd to dc2b4c0 Compare January 6, 2025 08:38

georgii-borovinskikh-sonarsource added 3 commits January 6, 2025 13:28

Add message-level logging context

62dc2fa

cleanup

b541115

cleanup

55280f0

Add missing tests

067f7a6

georgii-borovinskikh-sonarsource requested a review from gabriela-trutan-sonarsource January 6, 2025 13:52

gabriela-trutan-sonarsource approved these changes Jan 7, 2025

View reviewed changes

georgii-borovinskikh-sonarsource added 2 commits January 8, 2025 11:31

Fix CR issues

52316f7

Fixed a bug with null message level contexts, moved the null checking…

051625e

… responsibility from LoggerBase to LoggerContextManager, added StringBuilderLoggingExtensionsTests

georgii-borovinskikh-sonarsource merged commit 9051497 into feature/hardening-q4 Jan 8, 2025
4 checks passed

georgii-borovinskikh-sonarsource deleted the gb/log-context branch January 8, 2025 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SLVS-1723 Introduce log contexts #5913

SLVS-1723 Introduce log contexts #5913

georgii-borovinskikh-sonarsource commented Dec 19, 2024 •

edited by jira bot

Loading

gabriela-trutan-sonarsource left a comment

gabriela-trutan-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Jan 8, 2025

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Jan 6, 2025

georgii-borovinskikh-sonarsource Jan 6, 2025

gabriela-trutan-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Jan 6, 2025

gabriela-trutan-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024 •

edited

Loading

georgii-borovinskikh-sonarsource Dec 27, 2024 •

edited

Loading

georgii-borovinskikh-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Jan 6, 2025

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource left a comment

gabriela-trutan-sonarsource Jan 7, 2025

gabriela-trutan-sonarsource Jan 7, 2025

georgii-borovinskikh-sonarsource Jan 8, 2025

gabriela-trutan-sonarsource Jan 7, 2025

georgii-borovinskikh-sonarsource Jan 8, 2025

gabriela-trutan-sonarsource Jan 7, 2025

gabriela-trutan-sonarsource Jan 7, 2025

georgii-borovinskikh-sonarsource Jan 8, 2025 •

edited

Loading

sonarqubecloud bot commented Jan 8, 2025

	builder.Append('[').Append(property).Append(']').Append(' ');
	builder.AppendFormat("[{0}] ", property);

	builder.Append('[').AppendFormat(property, args).Append(']').Append(' ');
	builder.AppendFormat("[{0}] ", new StringBuilder().AppendFormat(property, args));

	public static StringBuilder AppendProperty(this StringBuilder builder, string property) =>
	public static StringBuilder AppendContextProperty(this StringBuilder builder, string property) =>

SLVS-1723 Introduce log contexts #5913

SLVS-1723 Introduce log contexts #5913

Conversation

georgii-borovinskikh-sonarsource commented Dec 19, 2024 • edited by jira bot Loading

gabriela-trutan-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabriela-trutan-sonarsource Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

georgii-borovinskikh-sonarsource Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabriela-trutan-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgii-borovinskikh-sonarsource Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 8, 2025

Quality Gate passed

georgii-borovinskikh-sonarsource commented Dec 19, 2024 •

edited by jira bot

Loading

gabriela-trutan-sonarsource Dec 27, 2024 •

edited

Loading

georgii-borovinskikh-sonarsource Dec 27, 2024 •

edited

Loading

georgii-borovinskikh-sonarsource Jan 8, 2025 •

edited

Loading