Skip to content

#74 Console.WriteLine has been replaced with EventSource #79

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

Merged
merged 3 commits into from
Feb 24, 2022

Conversation

0UserName
Copy link
Contributor

@0UserName 0UserName commented Feb 23, 2022

EventSource-based logger that implements the following interface:

    internal interface ILogEventSource
    {
        /// <summary>
        /// Writes an informational log message.
        /// </summary>
        /// 
        /// <param name="message">
        /// </param>
        ILogEventSource LogInformation(string message);

        /// <summary>
        /// Writes a warning log message.
        /// </summary>
        /// 
        /// <param name="message">
        /// </param>
        ILogEventSource LogWarning(string message);

        /// <summary>
        /// Writes an error log message.
        /// </summary>
        /// 
        /// <param name="message">
        /// </param>
        ILogEventSource LogError(string message);

        /// <summary>
        /// Writes an error log message.
        /// </summary>
        /// 
        /// <param name="message">
        /// </param>
        /// 
        /// <param name="exception">
        /// The exception to log.
        /// </param>
        ILogEventSource LogError(string message, Exception exception);
    }

Also added several tests verifying the basic aspects: event level match, payload name match, payload match (occurrence).

Fixes #74

@lukebakken
Copy link
Contributor

This is great! Thank you. I'll update the ApiApproval test.

Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukebakken
Copy link
Contributor

lukebakken commented Feb 23, 2022

Would it be possible to make this implementation the same or very similar to here:

https://github.com/rabbitmq/rabbitmq-dotnet-client/tree/main/projects/RabbitMQ.Client/client/logging

If your implementation in this pull request (#79) has advantages, please let me know what they are. The reason I ask is that I was planning to use the rabbitmq/rabbitmq-dotnet-client implementation when addressing issue #74.

@codecov-commenter
Copy link

Codecov Report

Merging #79 (322e2aa) into main (325550c) will increase coverage by 0.21%.
The diff coverage is 93.13%.

❗ Current head 322e2aa differs from pull request most recent head 4d2a236. Consider uploading reports for the commit 4d2a236 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   90.66%   90.88%   +0.21%     
==========================================
  Files          66       69       +3     
  Lines        4436     4532      +96     
  Branches      261      264       +3     
==========================================
+ Hits         4022     4119      +97     
+ Misses        357      353       -4     
- Partials       57       60       +3     
Impacted Files Coverage Δ
RabbitMQ.Stream.Client/Consumer.cs 93.02% <0.00%> (+1.06%) ⬆️
RabbitMQ.Stream.Client/Message.cs 89.33% <0.00%> (ø)
RabbitMQ.Stream.Client/Producer.cs 78.75% <0.00%> (+0.48%) ⬆️
RabbitMQ.Stream.Client/LogEventListener.cs 78.57% <78.57%> (ø)
RabbitMQ.Stream.Client/LogEventSource.cs 100.00% <100.00%> (ø)
Tests/EventSourceTests.cs 100.00% <100.00%> (ø)
RabbitMQ.Stream.Client/Subscribe.cs 63.73% <0.00%> (-1.10%) ⬇️
RabbitMQ.Stream.Client/MetaData.cs 90.65% <0.00%> (-0.94%) ⬇️
RabbitMQ.Stream.Client/Client.cs 90.54% <0.00%> (+1.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 325550c...4d2a236. Read the comment docs.

@0UserName
Copy link
Contributor Author

Here's one instance of Console that needs to be updated:

Fixed. My bad.

Would it be possible to make this implementation the same or very similar to here:

I thought the implementations were similar :)

  • Initialized properties Message and Keywords in Event attribute:

[Event(1, Message = "INFO", Keywords = Keywords.Log, Level = EventLevel.Informational)]

[Event(2, Message = "WARN", Keywords = Keywords.Log, Level = EventLevel.Warning)]

[Event(3, Message = "ERROR", Keywords = Keywords.Log, Level = EventLevel.Error)]

  • Added new methods with a similar signature from the implementation mentioned above::

public ILogEventSource LogInformation(string message, params object[] args)

public ILogEventSource LogWarning(string message, params object[] args)

public ILogEventSource LogError(string message, Exception exception, params object[] args)

  • Line break manipulation removed from LogEventSource.

  • There are no .NET452 preprocessor directives. Should it be added or can it be ignored?

  • There is no overload for RabbitMqExceptionDetail because there is no exception implementation. Instead, a signature with an argument of type Exception is used. Calling ToString on an exception will return the full set (message from inner exceptions, etc) of information.

  • There is no ESLog wrapper class. Instead, the ILogEventSource interface is used, which seems to me more logical if the original purpose of the wrapper was to hide details (properties, methods) from the EventSource.

@0UserName 0UserName requested a review from lukebakken February 24, 2022 10:25
@lukebakken
Copy link
Contributor

@0UserName thanks for the explanation. It is very valuable to know for a review. This looks great.

@lukebakken lukebakken merged commit a702c19 into rabbitmq:main Feb 24, 2022
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.

Use EventSource for logging
3 participants