Daily Log file - Multithread with date and time

Question

I have one window service. At any given time this window service runs 5-10 threads and each thread do some logging on text file. Logging can be exception or some information related to task/job. I have to make sure all the log should be in sequence with date and time. This is so important. Log file should create every day. If date change it should create new file and add log in new file. I have create one class which does this. Actually its two classes.

I used static variables for file name, file path and others, because I don't want to read configuration or create/destroy object every time(in logging process). Do you see anything which may cause an issue, or how I can improve performance?

class LoggingWorker
{
    class LogDetail
    {
        public string Data;
        public DateTime LogDate;

        public LogDetail(string _Data)
        {
            Data = _Data;
            LogDate = DateTime.Now;
        }
    }


    #region "Private variables"
    private static Object thisLock = new Object();
    static string LogFileName = "MyJobs.";
    static bool isVerboseMode;
    static string LogFilePath;
    static Thread thLogging;
    static StreamWriter ofileWrite;
    static Queue<LogDetail> _LogsQ;
    static bool runLogging;
    #endregion

    /// <summary>
    /// 
    /// </summary>
    static LoggingWorker()
    {
        //Reading from Config file
        isVerboseMode = AppSettings.GetSettingBool("Logging.Verbose");
        LogFilePath = AppSettings.GetSetting("Logging.Path");

        _LogsQ = new Queue<LogDetail>();


        if (!Directory.Exists(LogFilePath))
        {
            Directory.CreateDirectory(LogFilePath);
        }

        CreateFile();
        runLogging = true;
        thLogging = new Thread(StartLogging);
        thLogging.Start();
    }

    ~LoggingWorker()
    {
        runLogging = false;
    }

    private static StringBuilder GetErrorTrace(ref Exception ex)
    {

        StackTrace objTrace = new StackTrace(ex, true);
        StringBuilder strError = new StringBuilder("");
        strError.Append("Source :: " + ex.Source);
        strError.Append("\r\nError :: " + DateTime.Now.ToString());
        strError.Append("\r\nError Description : " + ex.Message);
        strError.Append("\r\nInner Exception : " + ex.InnerException + "\r\n\r\n");
        StackFrame sf;
        for (int i = objTrace.FrameCount - 1; i > -1; i--)
        {
            sf = objTrace.GetFrame(i);
            if (sf.GetFileLineNumber() < 1)
                continue;

            strError.Append("Method Name : " + sf.GetMethod() + "\r\n");
            strError.Append("File Name : " + sf.GetFileName() + "\r\n");
            strError.Append("Line Number : " + sf.GetFileLineNumber() + "\r\n\r\n");

        }
        return strError;
    }

    protected static void AddInLogQ(string msg, Exception ex)
    {
        msg = msg + "\t" + GetErrorTrace(ref ex).ToString();
        AddInLogQ(msg);
    }

    protected static void AddInLogQ(Exception ex)
    {
        AddInLogQ(GetErrorTrace(ref ex).ToString());
    }

    protected static void AddInLogQ(string Data)
    {
        try
        {
            lock (thisLock)
            {
                _LogsQ.Enqueue(new LogDetail(Data));
            }
        }
        catch (Exception ex)
        {
            EventLog.WriteEntry("AddInLogQ Failed.", ex.Message + ex.StackTrace + "\nOrignal Message:" + Data, EventLogEntryType.Error);
        }
    }

    protected static void StartLogging()
    {
        LogDetail oTmp;
        string sFileName;
        while (runLogging)
        {
            if (_LogsQ.Count > 0)
            {
                lock (thisLock)
                {
                    oTmp = _LogsQ.Dequeue();
                }

                //When Date change create new file.
                sFileName = LogFilePath + "\\" + LogFileName + DateTime.Today.ToString("yyyy_MM_dd") + ".log";

                //File is not exist so create new file.
                if (!File.Exists(sFileName))
                {
                    FileStream fs = File.Create(sFileName);
                    fs.Close();

                    if (ofileWrite != null)
                    {
                        ofileWrite.Close();
                    }
                    ofileWrite = new StreamWriter(sFileName, true);
                }

                ofileWrite.WriteLine(oTmp.LogDate.ToString("hh:mm:ss:fff tt") + " \t" + oTmp.Data);
                ofileWrite.Flush();
            }
            else
            {
                Thread.Sleep(500);
            }

        }
        if (ofileWrite != null)
        {
            ofileWrite.Close();
        }
    }


    protected static void CreateFile()
    {
        //When Date change create new file.
        string sFileName = LogFilePath + "\\" + LogFileName + DateTime.Today.ToString("yyyy_MM_dd") + ".log";

        //File is not exist so create new file.
        if (!File.Exists(sFileName))
        {
            FileStream fs = File.Create(sFileName);
            fs.Close();

            if (ofileWrite != null)
            {
                ofileWrite.Close();
            }
            ofileWrite = new StreamWriter(sFileName, true);
        }
    }

    protected static void _StopLogging()
    {
        runLogging = false;
    }

}

// This is the class which I used to Add Log from different threads/Jobs.

    internal class Logging : LoggingWorker
{
    public static void StopLogging()
    {
        _StopLogging();
    }

    internal static void CreateLog(string msg, Exception ex)
    {
        AddInLogQ(msg, ex);
    }

    internal static void CreateLog(Exception ex)
    {
        AddInLogQ(ex);
    }

    internal static void CreateLog(string Data)
    {
        AddInLogQ(Data);
    }


}

Answer 1

#region "Private variables"
private static Object thisLock = new Object();
static string LogFileName = "MyJobs.";
static bool isVerboseMode;
static string LogFilePath;
static Thread thLogging;
static StreamWriter ofileWrite;
static Queue<LogDetail> _LogsQ;
static bool runLogging;
#endregion  

One of the reasons I don't like regions is because regions can be considered as an anti-pattern.

Based on the NET naming guidelines one should

• use camelCase casing to name member variables
• shouldn't use hungarian notation to name variables

but most improtant (not stated in the guidelines) one shouldn't mix styles for naming. Here you have used 4 different naming styles:

• hungarian notation
• PascalCase
• camelCase
• underscore prefixed

in addition mixing the usage of aliases with classes/objects is bad too. Nowadays most C# developers use the aliases (Object vs object).

You should always add accessibility modifiers (private,public etc) to your variables,properties, methods and classes to make the scope of them more clear.

---

static LoggingWorker()
{
    //Reading from Config file
    isVerboseMode = AppSettings.GetSettingBool("Logging.Verbose");
    LogFilePath = AppSettings.GetSetting("Logging.Path");

    _LogsQ = new Queue<LogDetail>();


    if (!Directory.Exists(LogFilePath))
    {
        Directory.CreateDirectory(LogFilePath);
    }

    CreateFile();
    runLogging = true;
    thLogging = new Thread(StartLogging);
    thLogging.Start();
}  

What happens if the returned value of "Logging.Path" config will be either null or string.empty ?

You don't need to check if the directory exists, the Directory.CreateDirectory() method will do this for you.

---

private static StringBuilder GetErrorTrace(ref Exception ex)
{

    StackTrace objTrace = new StackTrace(ex, true);
    StringBuilder strError = new StringBuilder("");
    strError.Append("Source :: " + ex.Source);
    strError.Append("\r\nError :: " + DateTime.Now.ToString());
    strError.Append("\r\nError Description : " + ex.Message);
    strError.Append("\r\nInner Exception : " + ex.InnerException + "\r\n\r\n");
    StackFrame sf;
    for (int i = objTrace.FrameCount - 1; i > -1; i--)
    {
        sf = objTrace.GetFrame(i);
        if (sf.GetFileLineNumber() < 1)
            continue;

        strError.Append("Method Name : " + sf.GetMethod() + "\r\n");
        strError.Append("File Name : " + sf.GetFileName() + "\r\n");
        strError.Append("Line Number : " + sf.GetFileLineNumber() + "\r\n\r\n");

    }
    return strError;
}  

having the ref keyword doesn't add any value here because you don't change the object.

Returning a StringBuilder instead of a string should be considered only if you use that returned StringBuilder but that doesn't seem to be the case.

The methods of the StringBuilder are implemented using a fluent interface which means the methods return the StringBuilder itself which makes it possible to use it like so strError.Append(someValue).Append(someOtherValue);.
If you don't use the overloaded constructor of the StringBuilder the StringBuilder is created with a default capacity of 16 which needs to be doubled if the amount of added chars doesn't fit.

Appending a new line should be done by using the AppendLine() method of the StringBuilder.

Using string concatenation like strError.Append("Source :: " + ex.Source); shouldn't be done either.

Not using braces {} although they are optional is bad practice and will lead to error prone code. Using or not using them is a decision you need to make for yourself but you should stick to that decision. Right now you mixed the styles.

Shortening variable names will lead to less readable code and shouldn't be done.

Applying the mentioned points will lead to

private static string GetErrorTrace(Exception ex)
{

    StackTrace stackTrace = new StackTrace(ex, true);
    StringBuilder builder = new StringBuilder(1024);
    builder.Append("Source :: ").AppendLine(ex.Source)
           .Append("nError :: ").AppendLine(DateTime.Now.ToString())
           .Append("nError Description : ").AppendLine(ex.Message)
           .Append("Inner Exception : ").AppendLine(ex.InnerException.ToString())
           .AppendLine();

    StackFrame stackFrame;
    for (int i = stackTrace.FrameCount - 1; i > -1; i--)
    {
        stackFrame = stackTrace.GetFrame(i);

        int lineNumber = stackFrame.GetFileLineNumber();
        if (lineNumber < 1)
        {
            continue;
        }

        builder.Append("Method Name : ").AppendLine(stackFrame.GetMethod().ToString())
               .Append("File Name : ").AppendLine(stackFrame.GetFileName())
               .Append("Line Number : ").AppendLine(lineNumber.ToString())
               .AppendLine();

    }
    return builder.ToString();
}  

---

protected static void StartLogging()
{
    LogDetail oTmp;
    string sFileName;
    while (runLogging)
    {
        if (_LogsQ.Count > 0)
        {
            lock (thisLock)
            {
                oTmp = _LogsQ.Dequeue();
            }

            //When Date change create new file.
            sFileName = LogFilePath + "\\" + LogFileName + DateTime.Today.ToString("yyyy_MM_dd") + ".log";

            //File is not exist so create new file.
            if (!File.Exists(sFileName))
            {
                FileStream fs = File.Create(sFileName);
                fs.Close();

                if (ofileWrite != null)
                {
                    ofileWrite.Close();
                }
                ofileWrite = new StreamWriter(sFileName, true);
            }

            ofileWrite.WriteLine(oTmp.LogDate.ToString("hh:mm:ss:fff tt") + " \t" + oTmp.Data);
            ofileWrite.Flush();
        }
        else
        {
            Thread.Sleep(500);
        }

    }
    if (ofileWrite != null)
    {
        ofileWrite.Close();
    }
}

You already have a CreateFile() method but you have duplicated the code in this method. A call to CreateFile() would be sufficient.

Naming that method StartLogging() doesn't add any value because it will be called like LoggingWorker.Startlogging() which makes the Logging part obvious. Hence just naming it Start() would be sufficient.

Changing the logfile's name based on the date is ok, but what happens if something should be logged (added to the queue) just before the new day begins and the actual checking of the date will happen at the new day ? Your message is logged into the wrong file. You shouldn't check wether the file exists or not for checking if a new day has started, thats what dates are for.

What happens if the thread is stopped but you still have some items in the queue ? They just get silently swallowed which shouldn't happen. By changing the while condition to runLogging || _LogsQ.Count > 0 will fix this. Btw I would change runLogging to running.

If we add a class member variable private static DateTime logFileDate to the class we can pass the LogDetail.LogDate to the CreateFile() method to check if the date has changed. If it doesn't change we won't need to change the filename.

If we would override the ToString() method of the LogDetail class the actual writing of the detail would become clearer.

I don't see here a reason to use a lock because this method is only called from a single thread and the Dequeue() of the _LogsQ will be accessed only inisde this method.

So let us implement this points and afterwards let us visit the CreateFile() method which should be renamed to CreateFileIfDateChanged().

protected static void StartLogging()
{
    string sFileName;
    while (runLogging || _LogsQ.Count > 0)
    {
        if (_LogsQ.Count > 0)
        {
            LogDetail logDetail = _LogsQ.Dequeue();

            CreateFileIfDateChanged(logDetail.LogDate);

            ofileWrite.WriteLine(logDetail);
            ofileWrite.Flush();
        }
        else
        {
            Thread.Sleep(500);
        }

    }
    if (ofileWrite != null)
    {
        ofileWrite.Close();
    }
} 

looks much cleaner, doesn't it ?

---

protected static void CreateFile()
{
    //When Date change create new file.
    string sFileName = LogFilePath + "\\" + LogFileName + DateTime.Today.ToString("yyyy_MM_dd") + ".log";

    //File is not exist so create new file.
    if (!File.Exists(sFileName))
    {
        FileStream fs = File.Create(sFileName);
        fs.Close();

        if (ofileWrite != null)
        {
            ofileWrite.Close();
        }
        ofileWrite = new StreamWriter(sFileName, true);
    }
}  

Using string concatenation to build a filename should't be done but instead the Path.Combine() method should be used.

If we check for the LogDetail.LogDate we won't need to create a FileStream just for the sake of having a file in the path in addition this also fixes the issue if nothing should be logged then there won't exist any unneeded zero bytes length file in the path.

Aplying this points will lead to

private static DateTime logFileDate;
protected static void CreateFileIfDateChanged(DateTime logDate)
{
    if (logFileDate.Date == logDate.Date) { return; }

    logFileDate = logDate.Date;

    string fileName = Path.Combine(LogFilePath, LogFileName + logFileDate.ToString("yyyy_MM_dd") + ".log");

    if (ofileWrite != null) { ofileWrite.Close(); }

    ofileWrite = new StreamWriter(fileName, true);

}

---

class LogDetail
{
    public string Data;
    public DateTime LogDate;

    public LogDetail(string _Data)
    {
        Data = _Data;
        LogDate = DateTime.Now;
    }
}  

This is a private class so add an access modifier to it.

A method argument shouldn't be prefixed with an underscore.

Overriding the ToString() method is needed after the change to StartLogging().

Applying this points will lead to

private class LogDetail
{
    public string Data;
    public DateTime LogDate;

    public LogDetail(string data)
    {
        Data = data;
        LogDate = DateTime.Now;
    }

    public override string ToString()
    {
        return LogDate.ToString("hh:mm:ss:fff tt") + " \t" + Data;
    }
}  

---

protected static void _StopLogging()
{
    runLogging = false;
}  

Never prefix a method with an underscore.

---

I don't really see what it buys you to inherit this object.

Personally I would do the following

• change the class accessibility form private to internal
• change all protected methods to internal
• change the name of that class from LoggingWorker to Logger
• change the overloaded AddInLogQ methods to overloaded Log() methods

If you think that this class should be used in some other projects of you, then you should change to public where I suggested internal.