Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSHARP-5432: Feature/logging source generated - Logging design change proposal #1568

Closed
wants to merge 4 commits into from

Conversation

dtila
Copy link

@dtila dtila commented Dec 6, 2024

While working for a client, I have discovered a bug in the MongoDB driver related to handing default nullable values.
Since now I am contract free I have decided to dedicate some hours to fix it.
However I struggled a bit because getting the code as it is causes compilation errors on my machine.

The build errors were cause by the GetParms since compiler can not check the amount of parameters that are passed to LogDebug.
Since I had found nothing in the CONTRIBUTING readme, I propose a fix that removes code.

Motivation
The following PR is not fixing the initial bug, but is addressing some issues regarding build.
The PR purpose is to make the codebase more easy to work with and encourage other people to do PRs

The fix
The proposal is to use Source Generators to generate code for logging. For more information about this check out the official documentation https://learn.microsoft.com/en-us/dotnet/core/extensions/logger-message-generator
The benefit is that allows the compiler having more information about what's in the code - requiring less human atention and less error prone on potential new features

Scope of the fix
The modifications are just on the the lines that are causing build errors, but a unified approach should be considered on all Logging as final version.

Breaking changes
Current proposal it make MongoDB driver build successfully but causes test to fail. The reason is because of the current mechanism TemplateProvider that is implemented with arrays and delegates.

Example:

 public void LogAndPublish<TEvent, TArg>(TEvent @event, TArg arg) where TEvent : struct, IEvent
 {
     var eventTemplateProvider = StructuredLogTemplateProviders.GetTemplateProvider(@event.Type);

     if (_logger?.IsEnabled(eventTemplateProvider.LogLevel) == true)
     {
         var @params = eventTemplateProvider.GetParams(@event, _eventLogFormattingOptions, arg);
         var template = eventTemplateProvider.GetTemplate(@event);

         Log(eventTemplateProvider.LogLevel, template, exception: null, @params);
     }

     _eventPublisher?.Publish(@event);
 }

The tests are currently failing because the logging function's signature no longer requires TemplateProvider parameters and it fails the casting of the delegate.

If we proceed with these changes, the related code will need to be modified or entirely removed. In fact, the use of the TemplateProvider class is now obsolete. My proposal is to eliminate the runtime array registration and replace it with a compile-time function. I estimate that the code in MongoDB.Driver.Core.Logging could be consolidated into 2-3 classes.

I have not yet made this modification as it exceeds the effort initially estimated for addressing the original bug. However, I am open to continuing if needed.

Out of scope
Microsoft.Extensions.Logging.Abstractions version has been changed as well to acomodate new version. Version 6 supports net72,net6.0 here is a test project: https://github.com/dtila/youtube/tree/main/logging-multitargets

However it causes conflicts in projects targeting .NET 8 (they have to build on 6 or have target specifics) - (AstrolabeWorkloadExecutor, Benchmarks, etc)

Conclusions
While this PR is proof of concept I've opened to start initiating a discussion about the proposed changes and identify any potential risks, and to make sure the PR is aligning with the vision of the project. From my perspective, the risks are low since this area of the codebase is well-tested, and the compiler will gain better awareness of the changes.

I see this as a step toward making the codebase lighter and more efficient. While it might not be a high priority in the short term, it aligns with the long-term vision of improving C# compiler speed and enhancing compiler-aware insights.

I am open to implement the feature completely, but I want to ensure everyone is aligned

BTW can you add some information about how is the setup is on a dev machine to work with the code as it is?

@dtila dtila requested a review from a team as a code owner December 6, 2024 14:14
@dtila dtila requested review from JamesKovacs and removed request for a team December 6, 2024 14:14
@dtila dtila changed the title Feature/logging source generated CSHARP-5432: Feature/logging source generated - Logging design change proposal Dec 9, 2024
@BorisDog BorisDog requested review from BorisDog and removed request for JamesKovacs December 11, 2024 21:18
@BorisDog BorisDog removed their request for review December 20, 2024 23:49
@BorisDog
Copy link
Contributor

Thank you for your suggestion to use the high performance logging.
We'll be evaluating this idea and it's protentional issues (outlined in CSHARP-5432).
Please follow CSHARP-5432 for further updates.

Closing this PR as we won't be accepting it for now.

@BorisDog BorisDog closed this Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants