Currently I'm writing something like a messaging or logging framework (I'm not really sure how you name something like that). There are different servers which have to be notified if something happens.
The servers mainly store the data e. g. for statistics. One server is an ELK stack which receives the data via a REST api.
But every server expects a slightly different model, so the events have to be transformed in different ways, e. g. by adding an application key or formatting the timestamp.
A windows desktop application, which runs on multiple devies, creates the events, which have to be transmitted to the servers.
The following code is part of the client (windows desktop application).
I have a facade which accepts a event and passes it to all targets.
public class EventController : IEventSender
{
public EventController(params IEventTarget[] targets)
{
Targets = targets;
}
public IEventTarget[] Targets { get; }
public void SendEvent(IEvent e)
{
foreach (var target in Targets)
{
target.SendEvent(e);
}
}
}
Each target has to handle the event on its own. But the events have to be send to the servers in the right order (for that I created a queue with System.Collections.Concurrent.BlockingCollection and System.Collections.Concurrent.ConcurrentQueue). For the beginning (and later for debugging and testing purposes) I created a target which writes the events to a file. Currently 3 different targets (+ FileTarget) are intended. Each targe should implement its own queue and background task (I want to prevent duplicated code by inheritance).
public class FileTarget : IEventTarget
{
private static readonly ILogger Log = LoggerFactory.GetLogger(typeof(FileTarget)); // logger interface, not of importance
private static readonly object FileLock = new object(); // lock to protect multiple file access
private readonly BlockingCollection<IEvent> _queue =
new BlockingCollection<IEvent>(new ConcurrentQueue<IEvent>()); // queue of elements to proceed
private bool _backgroundActionShouldRun = true; // token to exit background action
public FileTarget(string fileName)
{
FileName = fileName;
StartBackgroundAction();
}
public string FileName { get; }
public void SendEvent(IEvent e)
{
_queue.Add(e);
}
~FileTarget()
{
_backgroundActionShouldRun = false;
}
private void StartBackgroundAction()
{
Task.Run((Action) BackgroundAction);
}
private void BackgroundAction()
{
// im not sure if this task will be completed if the instance get destroyed
while (_backgroundActionShouldRun) {
try
{
var e = _queue.Take();
var transformedEvent = TransformEvent(e);
WriteEventToFile(transformedEvent);
}
catch (Exception ex)
{
Log.Error("Unable to take an object from queue.", ex);
}
}
}
private TransformedEvent TransformEvent(IEvent e)
{
// transform the event and return it
}
private void WriteEventToFile(TransformedEvent e)
{
try
{
// im not sure if this is thread save, if it is i can remove the lock
lock (FileLock)
{
// System.IO.File.AppendAllLines requires a collection, I'm not happy with this
string[] contents = {e.ToString()};
System.IO.File.AppendAllLines(FileName, contents );
}
}
catch (Exception ex)
{
Log.Error($"Failed to write message to file {FileName}.", ex);
}
}
}
I have multiple questions on how to improve that code
- Is there a better way to queue the events?
- Is this thread safe (as long as no other process/thread tries to access the file)?
- I'm afraid that in some cases (or maybe in most?) the background task will never be completed.
- Should I move the queue and the background process to the facade?
- I have oriented myself by some logging frameworks. Should I take advantage of one like NLog with a custom target?
- Do you have some other suggestions to improve the code?
