SLVS-1723 Introduce log contexts #5913

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

georgii-borovinskikh-sonarsource · 2024-12-23T09:18:31Z

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

@@ -119,6 +119,10 @@ public void LogVerbose(string message, params object[] args)
            {
                // no-op
            }
+
+            public ILogger ForContext(params string[] context) => this;


This class is removed in the CFamily branch

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

sonarqubecloud · 2024-12-24T09:54:55Z

Quality Gate passed

Issues
0 New issues
1 Accepted issue

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

See analysis details on SonarQube Cloud

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.

gabriela-trutan-sonarsource · 2024-12-27T07:43:38Z

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

-        }
-    }
+[ExcludeFromCodeCoverage]
+internal class SystemDebugLoggerWriter : ILoggerWriter


The class name does not match the file name anymore.

gabriela-trutan-sonarsource · 2024-12-27T07:56:43Z

src/Integration.UnitTests/Helpers/SonarLintOutputTests.cs

-using SonarLint.VisualStudio.Core;
-using SonarLint.VisualStudio.TestInfrastructure;
+using SonarLint.VisualStudio.Core.Logging;
+using SonarLint.VisualStudio.Integration.Helpers;

 namespace SonarLint.VisualStudio.Integration.UnitTests.Helpers
 {
    [TestClass]
    public class SonarLintOutputTests


The class name is outdated: it no longer matches the class under test

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

gabriela-trutan-sonarsource · 2024-12-27T08:04:55Z

src/Core.UnitTests/Logging/LoggerContextManagerTests.cs

+        MefTestHelpers.CheckTypeCanBeImported<LoggerContextManager, ILoggerContextManager>();
+
+    [TestMethod]
+    public void MefCtor_CheckIsSingleton() =>


Test name is wrong

gabriela-trutan-sonarsource · 2024-12-27T08:05:25Z

src/Core.UnitTests/Logging/LoggerContextManagerTests.cs

+        MefTestHelpers.CheckIsNonSharedMefComponent<LoggerContextManager>();
+
+    [TestMethod]
+    public void EmptyContext()


Suggested change

public void EmptyContext()

public void Ctor_EmptyContext()

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));

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.

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...

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.

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

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

SLVS-1725 Update Microsoft.VSSDK.BuildTools nuget package (#5916)

16a9f7e

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 added 12 commits December 24, 2024 10:45

SLVS-1723 Introduce log contexts

02f1b9f

Add verbose contexts + refactoring

e80ab46

refactor loggers

8fd79d4

more fixes and refactoring

f17af35

add some

9ddb8ef

add license headers

ac2a4ea

Add LogContextManagerTests

b4a3407

rearrange code in TestLogger

e84dc94

renames

b9a2df9

Add LoggerFactoryTests

135ea4a

Add LoggerBaseTests

48aeac6

change format

d01d7dd

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

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

georgii-borovinskikh-sonarsource Dec 23, 2024

sonarqubecloud bot commented Dec 24, 2024

gabriela-trutan-sonarsource left a comment

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

georgii-borovinskikh-sonarsource Dec 27, 2024

gabriela-trutan-sonarsource Dec 27, 2024

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

gabriela-trutan-sonarsource Dec 27, 2024

	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));

SLVS-1723 Introduce log contexts #5913

Are you sure you want to change the base?

SLVS-1723 Introduce log contexts #5913

Conversation

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

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 24, 2024

Quality Gate passed

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

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