Custom error page with information tokens implementation 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 =&gt; 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.
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)
        =&gt; 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;
}