Conversation
Add an EventSource listener to listen to the RabbitMQ event source which logs info, warn, and error messages. Forward these messages to an ILogger. Fix #564
src/Components/Aspire.RabbitMQ.Client/RabbitMQEventSourceLogForwarder.cs
Show resolved
Hide resolved
src/Components/Aspire.RabbitMQ.Client/RabbitMQEventSourceLogForwarder.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Aspire.RabbitMQ.Client; | ||
|
|
||
| internal sealed class RabbitMQEventSourceLogForwarder : IDisposable |
There was a problem hiding this comment.
This commit message made my day 🤣
| /// </summary> | ||
| private sealed class RabbitMQEventSourceListener : EventListener | ||
| { | ||
| private readonly List<EventSource> _eventSources = new List<EventSource>(); |
There was a problem hiding this comment.
I don't think thread safety is a concern here since this List is only touched in between the base ctor running and this class's ctor running. Between that time, I don't think another thread can touch this object.
Note that this implementation was copied from the Azure SDK, and it hasn't changed in years. So I semi-trust it.
There was a problem hiding this comment.
I'm not sure there's a bug here with _eventSources, but I'm also not sure what this accomplishes either. If I recall correctly, you're right that OnEventSourceCreated will be called from the base constructor, and so you do have to handle the case where the EventListener might not be fully initialized when events start to show up in OnEventWritten. That said, you already handle this on line 225 where you ensure that _log is non-null before calling Invoke. I think you can get rid of all of this _eventSources machinery.
I realize I'm late to the party here, but would be worth removing for simplicity, or if it doesn't work without this, identifying an issue that needs to be documented/fixed.
| if (_log == null) | ||
| { | ||
| _eventSources.Add(eventSource); | ||
| } |
There was a problem hiding this comment.
Why store all event sources?
There was a problem hiding this comment.
Note that this implementation was copied from the Azure SDK.
Only the event sources that are added before the ctor fully finishes (i.e. _log == null) get added to the list. I think this is to handle the case where the base EventListener starts calling virtual methods on this object before the ctor has fully completed. This ensures those event sources are listened to.
| configurationOptionsSection.Bind(factory); | ||
|
|
||
| // the connection string from settings should win over the one from the ConnectionFactory section | ||
| var connectionString = settings.ConnectionString; |
There was a problem hiding this comment.
no, this was just me being nit-picky. This variable isn't used until here, so I moved it since it seemed out of place.
* Implement RabbitMQ logging Add an EventSource listener to listen to the RabbitMQ event source which logs info, warn, and error messages. Forward these messages to an ILogger. Fix #564 * Stop coding like a dinosaur.
danmoseley
added
the
area-integrations
5 participants
Add an EventSource listener to listen to the RabbitMQ event source which logs info, warn, and error messages. Forward these messages to an ILogger.
Fix #564