2
\$\begingroup\$

I have an asp.net web api. I would like to log errors into a file. So I decided to use NLog. I already have UnhandledExceptionHandler so I added NLog as follows. I removed all the try/catch in order to log from a single point, just inside the handler.

Would you please have a look at my code? Is it good enough?

using System;
namespace Game.Handlers
{
    public class UnhandledExceptionLogger : ExceptionLogger
    {
        private static Logger logger = LogManager.GetCurrentClassLogger();

        public override void Log(ExceptionLoggerContext context)
        {
            var ex = context.Exception;

            string strLogText = FormatException(ex);

            var requestedURi = (string)context.Request.RequestUri.AbsoluteUri;
            var requestMethod = context.Request.Method.ToString();
            var timeUtc = DateTime.Now;

            SqlErrorLogging sqlErrorLogging = new SqlErrorLogging();
            ApiError apiError = new ApiError()
            {
                Message = strLogText,
                RequestUri = requestedURi,
                RequestMethod = requestMethod,
                TimeUtc = DateTime.Now
            };
            //sqlErrorLogging.InsertErrorLog(apiError);
            //NLOG
            logger.Error(strLogText + Environment.NewLine + DateTime.Now);
        }


        private string FormatException(Exception ex, int depth = 0)
        {
            var indent = "";
            if (depth > 0)
                indent.PadRight(depth * 4);

            var sb = new StringBuilder();
            IndentLine($"Source --- {ex.Source}");
            IndentLine($"StackTrace --- {ex.StackTrace}");
            IndentLine($"TargetSite --- {ex.TargetSite}");

            if (ex.InnerException != null)
            {
                IndentLine("--- Inner Exception ---");
                sb.Append(FormatException(ex.InnerException, depth + 1));
                IndentLine("--- End Inner Exception ---");
            }
            if (ex.Message != null)
            {
                IndentLine($"Message --- {ex.Message}");
            }
            return sb.ToString();

            void IndentLine(string s)
            {
                sb.Append(indent);
                sb.AppendLine(s);
            }
        }

    }
}

Here is the sample NLog config:

<nlog xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
    <targets>
      <target name="logfile" xsi:type="File" fileName="${basedir}/MyLogs/${date:format=yyyy-MM-dd}-api.log" />
      <target name="eventlog" xsi:type="EventLog" layout="${message}" log="Application" source=" GamePIN Api Services" />
      <target name="database" type="Database" connectionString="Data Source=(localdb)\MSSQLLocalDB;Database=Game; Initial Catalog=GameAPI;MultipleActiveResultSets=True;">
        <commandText> insert into ExceptionLog ([TimeStamp],[Level],Logger, [Message], UserId, Exception, StackTrace) values (@TimeStamp, @Level, @Logger, @Message, case when len(@UserID) = 0 then null else @UserId end, @Exception, @StackTrace); </commandText>
        <parameter name="@TimeStamp" layout="${date}" />
        <parameter name="@Level" layout="${level}" />
        <parameter name="@Logger" layout="${logger}" />
        <parameter name="@Message" layout="${message}" />
        <parameter name="@UserId" layout="${mdc:user_id}" />
        <parameter name="@Exception" layout="${exception}" />
        <parameter name="@StackTrace" layout="${stacktrace}" />
        <dbProvider>System.Data.SqlClient</dbProvider>
      </target>
    </targets>
    <rules>
      <!-- I am adding my 3 logging rules here -->
      <logger name="*" minlevel="Error" writeTo="database" />
      <logger name="*" minlevel="Error" writeTo="logfile" />
      <logger name="*" minlevel="Error" writeTo="eventlog" />
    </rules>
  </nlog>
\$\endgroup\$
1
\$\begingroup\$

The variable ex is not used anywhere else, so it can be in-lined.

string strLogText = FormatException(context.Exception);

The multiple calls to DateTime.Now will give different time stamps.

Hold on to the timestamp in one variable early in the function and reuse that.

It was already assigned here

 var timeUtc = DateTime.Now;

so use that

//...

ApiError apiError = new ApiError()
{
    Message = strLogText,
    RequestUri = requestedURi,
    RequestMethod = requestMethod,
    TimeUtc = timeUtc
};

//...

Given the variable name, the assumption is that the timestamp was meant to be UTC, which would mean that it should be using DateTime.UtcNow

The SQL error logging (though commented out) can be encapsulated into its own function/concern

private void LogSQL(string message, HttpRequestMessage request, DateTime timestamp) {
    var requestedURi = (string)request.RequestUri.AbsoluteUri;
    var requestMethod = request.Method.ToString();
    SqlErrorLogging sqlErrorLogging = new SqlErrorLogging();
    ApiError apiError = new ApiError() {
        Message = message,
        RequestUri = requestedURi,
        RequestMethod = requestMethod,
        TimeUtc = timestamp
    };
    //sqlErrorLogging.InsertErrorLog(apiError);
}

This can be abstracted out into its own service/concern if so desired

The above and a few minor changes results in a refactoring of

public class UnhandledExceptionLogger : ExceptionLogger {
    private static readonly Logger logger = LogManager.GetCurrentClassLogger();

    public override void Log(ExceptionLoggerContext context) {
        DateTime timestamp = DateTime.UtcNow;
        string strLogText = FormatException(context.Exception);            
        //SQL
        SqlLog(strLogText, context.Request, timestamp);
        //NLOG
        NLog(logger, strLogText, timestamp);
    }

    private void NLog(Logger logger, string message, DateTime timestamp) {
        var sb = new StringBuilder();
        sb.AppendLine(message);
        sb.AppendLine(timestamp);
        logger.Error(sb.ToString());
    }

    private void SqlLog(string message, HttpRequestMessage request, DateTime timestamp) {
        var requestedURi = (string)request.RequestUri.AbsoluteUri;
        var requestMethod = request.Method.ToString();
        SqlErrorLogging sqlErrorLogging = new SqlErrorLogging();
        ApiError apiError = new ApiError() {
            Message = message,
            RequestUri = requestedURi,
            RequestMethod = requestMethod,
            TimeUtc = timestamp
        };
        //sqlErrorLogging.InsertErrorLog(apiError);
    }

    private string FormatException(Exception ex, int depth = 0) {
        //... omitted for brevity
    }
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you @Nkosi, I wonder if managing errors from a single point like UnhandledExcepitonHandler is a good idea? As a result I removed all try/catch block in the code. \$\endgroup\$ – Fake Aug 18 '19 at 7:48
  • \$\begingroup\$ I suggest reading up on cross cutting concerns. \$\endgroup\$ – Nkosi Aug 18 '19 at 8:11
  • \$\begingroup\$ Should I change timestamp to this sb.AppendLine(timestamp.ToLongDateString()); \$\endgroup\$ – Fake Aug 18 '19 at 8:40
  • \$\begingroup\$ I think all the exceptions are not handled in UnhandledExceptionLogger. Would it be correct if I add logging also inside of GlobalExceptionHandler? What is your opinion? \$\endgroup\$ – Fake Aug 19 '19 at 7:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.