Refactor LogEventQueue when removing support for .NET Framework 4.5

[erikmf12]

I am wondering what the design choices are for the LogEventQueue. There are better options than rolling your own these days, mainly the Channel<T> class with ChannelReader<T> and ChannelWriter<T>. This might be overkill for this implementation, but any time I see code with a lock in it I try to think of alternative solutions.

[github-actions]

Hi there and welcome to this repository!

A maintainer will be with you shortly, but first and foremost I would like to thank you for taking the time to report this issue. Quality is of the highest priority for us, and we would never release anything with known defects. We aim to do our best but unfortunately you are here because you encountered something we didn't expect. Lets see if we can figure out what went wrong and provide a remedy for it.

[FantasticFiasco]

Hi @erikmf12!

Are channels also supported on .NET Framework 4.5? Because I'm hesitant to use different implementations on different .NET versions. That wouldn't improve the testing story.

[erikmf12]

True, didn't think about that. I think .net framework version 4.6 is required with a nuget package.

[FantasticFiasco]

I'm thinking about renaming this issue to Refactor LogEventQueue when removing support for .NET Framework 4.5. Whould that be ok for you?

[erikmf12]

That sounds good.

[kuskmen]

Hi @FantasticFiasco, I might be interested in doing this however, after short observation of the code I noticed that even if we move to channels it will still need synchronization for queueLimitBytes which is unfortunate and it will kinda obsolete the channels in the first place.

So my question is "maybe" we can compromise the configuration not being an exact number (meaning race conditions are allowed) in favor of performance and lock-free construct or we can re-define limitation to be based on logEvents rather than bytes and then bounded channels fit nicely.

Also, channels are typically used in asynchronous code, but I notice that ILogEventSink interface's method Emit is simply void. Its not the end of the world I believe to use synchronous methods of channel but I am just thinking if there is an option to use async flow?

Thanks in advance about your input!

[FantasticFiasco]

That the queue can be configured with a byte size limit is actually really important, because log servers sometimes are limited with a fixed max payload size, and any requests with a size that exceeds this limit are rejected.

A rejected payload should be considered more detrimental than a potential performance gain on the producing part. If we decide to refactor this code, we have to do it with the requirements that the promises of the current implementation still stands, that the code will become more clean and thus easier to maintain, and that we benchmark the solution to prove that the change is equal or an improvement to the current implementation (I can do the benchmarks).

Thought?

[kuskmen]

Wait, so I am missing something probably. Isn't there another configuration called logEventLimitBytes that will answer for the log servers payload size? I couldn't understand if you are using requests/log events here interchangeably because if not, I think that the request payload can be configured by batchSizeLimitBytes, no?

Nevertheless, I also agree that current api and configuration should be kept as they are, but then what do you think of my compromise not synchronizing this queueLimitBytes and expose it to "some" race condition because I believe it will not be critical if we yield favorable results during the benchmarks?

[FantasticFiasco]

Maybe I'm missing something, let me see if I can correctly analyze the problem during this week.