1
\$\begingroup\$

I have created custom error pages (for 500 and 404 http codes) for asp.net web site. After an error, application should create new entry in database in Logs table with information about error. Each entry has unique hash string (it's not primary key) that is returned to end user on error page. With this hash string user can contact with support for advice.

protected void Application_Error(object sender, EventArgs e)
            {
                var exception = Server.GetLastError();

                if (exception is Httpexceptionception httpexceptionception)
                {
                    if (httpexceptionception.GetHttpCode() == 404)
                    {
                        HttpContexceptiont.Current.ClearError();
                        Response.Redirect($"~/NotFound.aspx", false);
                        return;
                    }
                }

                DatabaseLog log = new DatabaseLog()
                {
                    Timestamp = DateTime.UtcNow,
                    Stacktrace = exception.StackTrace
                };

                long id = log.InsertLog();

                string hash;

                using (var md5 = MD5.Create())
                {
                    hash = String.Concat(md5.ComputeHash(BitConverter
                      .GetBytes(id))
                      .Select(x => x.ToString("x2")))
                      .Remove(0, 26);
                }

                log.InterfaceId = hash;
                log.UpdateLog(id);

                HttpContexceptiont.Current.ClearError();
                Response.Redirect($"~/Error.aspx?id={hash}", false);
            }

Following code is responsible for creating new entry in database, id of inserted entry is returned, finally random hash token based on inserted id is generated for user (and inserted entry is updated with this token). I use md5 because I want unique random sequence of characters, but I only want 6 char length token and I don't want use GUID. I think that 6 char tokn length is sufficient. I don't want to return primary key of database entry to error page. Generated hash is returned in query string and this hash string is display to end user.

        public class DatabaseLogger
    {
        public string InterfaceId { get; set; }
        public DateTime Timestamp { get; set; }
        public string Stacktrace { get; set; }

        string _conString = /* ... con string ...*/

        public long InsertLog()
        {
            using (SqlConnection con = new SqlConnection(_conString))
            {
                con.Open();
                string sql = $@"insert into Logs (TimeStamp, Stacktrace) output INSERTED.Id 
                                values (@timeStamp, @stackTrace)";
                using (SqlCommand cmd = new SqlCommand(sql, con))
                {
                    cmd.Parameters.AddWithValue("@timeStamp", this.Timestamp);
                    cmd.Parameters.AddWithValue("@stackTrace", this.Stacktrace);
                    return (long)cmd.ExecuteScalar();
                }

            }
        }

        public void UpdateLog(long id)
        {
            using (SqlConnection con = new SqlConnection(_conString))
            {
                con.Open();
                string sql = $"update Logs set InterfaceId = @interfaceId where Id=@id";
                using (SqlCommand cmd = new SqlCommand(sql, con))
                {
                    cmd.Parameters.AddWithValue("@id", id);
                    cmd.Parameters.AddWithValue("@interfaceId", this.InterfaceId);
                    cmd.ExecuteNonQuery();
                }
            }
        }
    }

And table has columns:

  • Id - long, primary key
  • InterfaceId - string
  • Timestamp - DateTime
  • Stacktrace - string

How can I improve it? Thanks.

\$\endgroup\$
2
\$\begingroup\$

First of all I'd consider if hashing is required. Think that:

  • It's longer to type and impossible to remember.
  • If user is calling technical support by phone then he will have hard time to spell it out (especially if not native speaker).
  • It has not any benefit over a simpler short number.
  • You're computing a MD5 hash (128 bits, 32 bytes) then converting to hex string (64 characters) then removing the first 26. Are you sure you do not introduce collisions in your hash?!

You already have an ID then you might want to simply return that ID (eventually you may add a check-digit to prevent simple mistakes). This will also simplify your code because you do not need an UPDATE to store the calculated hash (check digit may simply be recomputed). Hashing here is (IMO) just overthinking (if you want to obfuscate the ID simply return its hexadecimal value).


You're using long (mapped to bigint). Do you plan to have more than 2^32 errors in your log? 16 Gb just to store the IDs...it's unlikely, don't consider just the number itself but disk space used for each row which has also a very long string).

You're logging stack trace but you're ignoring exception message and exception types which are probably more important than the stack.

You're logging the exception as-is but think about MethodInvocationException, AggregateException, TypeLoadException (just to mention few) where content of InnerException (or InnerExceptions) is much more important than its wrapper. You have a nice Exception.GetBaseException() to get the root cause of the current exception.

I don't know how you throw your own exceptions but often I use Exception.Data to provide more context and useful details. It may be not your case but don't forget about it.


You hard-coded 404 HTTP status code, why don't you use HttpStatusCode.NotFound?


DatabaseLogger.InsertLog() (fix your calling code because it's DatabaseLog) is a missed opportunity of generalization. I'd not go with complex architectures, factories and abstractions (but a simple IoC container might do well here), the fact that storage is in a database is a detail.

Also class should be sealed (currently you do not have extension points and it's not supposed to be a base class):

sealed Logger {
    public int Write(Exception exception) {
        var timestamp = DateTime.UtcNow;
        var content = TranslateExceptionToText(exception);

        // Code to write to DB storage...
    }

    private static string TranslateExceptionToText(Exception exception)
        => exception.GetBaseException().ToString();
}

Notes:

  • TranslateExceptionToText() is pretty naive, better logic should be used.
  • I moved responsibility to extract useful information from your error handler to the logger class but you may consider to introduce a 3rd class for this when logic is more complex.
  • Move SQL code to private const string fields, it's easier to find and edit keeping your code cleaner.
  • Do not use AddWithValue(). It has some, not obvious, drawbacks. It's not always a bad choice but I think it's easier to stop using it than determining each time if its usage is good or not.
  • Logging may fail and you dot have any error handling! What's your recovery policy in this case?
  • Strings for SQL code are not (do not use) interpolated strings. Drop $.
  • Strings for SQL code are not (do not use) verbatim strings. Drop @.

Application_Error might be simplified:

protected void Application_Error(object sender, EventArgs e)
{
    var exception = Server.GetLastException();
    HttpContextException.Current.ClearError();

    if (IsPageNotFoundException(exception))
        RedirectToNotFound();
    else
        RedirectToError(Logger.Default.Write(exception));
}

Implementation of IsPageNotFoundException(), RedirectToNotFound() and RedirectToError() should be clear but let's write IsPageNotFoundException():

private static bool IsPageNotFoundException(Exception exception)
{
    if (exception is HttpException httpException)
        return httpException.GetHttpCode() == (int)HttpStatusCode.NotFound;

    return false;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'm curious about this statement Do not use AddWithValue(). Do you have link to a discussion why this is considered a bad practice? \$\endgroup\$ – t3chb0t May 26 '17 at 13:18
  • 2
    \$\begingroup\$ @t3chb0t yes, let me find it! ;) blogs.msmvps.com/jcoehoorn/blog/2014/05/12/…, for example (but I remember there are other better ones). It's not always bad if you know what you're doing but I find, personally, much harder to think about it every time I have to use it than simply avoid its use. \$\endgroup\$ – Adriano Repetti May 26 '17 at 13:41
  • \$\begingroup\$ This seems to be really tricky sometimes. I think one should be aware of the implications the usage of AddWithValue might have so that if any bugs arise you know where they come from and of course it needs to be tested. On the other side we are lazy and pick the simplest solution ;-] \$\endgroup\$ – t3chb0t May 26 '17 at 15:11
  • \$\begingroup\$ @AdrianoRepetti Thanks for a review. I wonder why I must change my class access level from public to private sealed? After this, this is not accessible from Global.asax file. Or do you mean create DatabaseLogger class and nested class Logger inside? \$\endgroup\$ – mkul May 26 '17 at 15:27
  • 1
    \$\begingroup\$ @mkul No, when writing that sentence I forgot you're using it from an ASPX page. Just make it sealed (you can't inherit a new class from it because, so far, it has no extension points). That's why reviewed code...has to be re-reviewed! :) \$\endgroup\$ – Adriano Repetti May 26 '17 at 15:41

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.