When to log errors in ASPX when using database connections?

Question

When to log errors in ASPX when using database connections? In the following code, the authors of Pro ASP.NET 3.5 in C# 2008 (Third ed), suggest logging an SQL error in the catch block. My questions are:

Is this common practice?

Is this the best practice?

In short, would it be better to close the current connection and handle the error completely separately? What are the advantages and disadvantages for either approach?

Edited to add: To clarify the question is not IF one should log (that question is already discussed elsewhere), this question is more specific. The question is where in the code to log. Should it be done in the catch (while the connection is still open) or should the current connection be close and logging completely separate? Not the section to add logging as part of the catch (even if it calls an outside method, that connection remains open) until it returns and reaches the 'finally'.

 public int InsertEmployee(EmployeeDetails emp)
 {
  SqlConnection con = new SqlConnection(connectionString);
  SqlCommand cmd = new SqlCommand("InsertEmployee", con);
  cmd.CommandType = CommandType.StoredProcedure;

  cmd.Parameters.Add(new SqlParameter("@FirstName", SqlDbType.NVarChar, 10));
  cmd.Parameters["@FirstName"].Value = emp.FirstName;
  cmd.Parameters.Add(new SqlParameter("@LastName", SqlDbType.NVarChar, 20));
  cmd.Parameters["@LastName"].Value = emp.LastName;
  cmd.Parameters.Add(new SqlParameter("@TitleOfCourtesy", SqlDbType.NVarChar, 25));
  cmd.Parameters["@TitleOfCourtesy"].Value = emp.TitleOfCourtesy;
  cmd.Parameters.Add(new SqlParameter("@EmployeeID", SqlDbType.Int, 4));
  cmd.Parameters["@EmployeeID"].Direction = ParameterDirection.Output;

  try 
  {
   con.Open();
   cmd.ExecuteNonQuery();
   return (int)cmd.Parameters["@EmployeeID"].Value;
  }
  catch (SqlException err) 
  {
   // Replace the error with something less specific.
   // You could also log the error now.
   throw new ApplicationException("Data error.");
  }
  finally 
  {
   con.Close();   
  }
 }

Answer 1

I typically take a "catch-all" approach and log ANY exceptions that are truly exceptional, and swallow the rest. :)

EDIT:

I guess I need to make myself clearer... The rule of thumb for exceptions is to log any and all exceptions, except in those situations where it makes sense to catch the exception.

Example:

In .NET 1.1, the only data type that had TryParse() was Double. Thus, for the other data types (such as int) it makes sense to do this:

try{
    int.Parse(x)
} catch (FormatException){
    //take care of invalid input
}

I would suggest NLog. Fantastic logging library.

Answer 2

IMHO that's bad advice.

Supposing you're using raw ADO.NET, you should wrap your IDisposable objects (like IDbConnection) in a using block.

If an unexpected exception (i.e. you can't do anything) happens don't catch it and let some error handler like ELMAH handle logging it.

Answer 3

Short answer: it depends...

Long answer:

Logging only really matters if someone checks the logs. If this is a personal site, then you may not care much. However if this is a mission critical app, then you should log as many errors as possible. The error logs not only provide information to help debug by showing you were in code the error occurred, but the logs can also show you if the error occurs during a specific time of day (maybe conflicting with another process). However, you need to keep in mind the extra overhead involved in logging small errors. Sometimes, depending on how minor, it might be OK to just show the error to the user and let them decide if it is a big enough deal to contact you or not.

I personally have un-handled exceptions emailed to me. Very easy to do with ASP.NET apps using the Page_Error event handler.

protected void Page_Error(object sender, EventArgs e)
{
    //Handle Error Here
}

Timeout exceptions and other exceptions that I catch I usually log to a database somewhere and keep forever. I usually log mundane errors and success audits to a text file somewhere which I keep for a month or so.

Hope this helps.

Update: I think I understand your question better now. To log in the catch while connection is open or log after the connection is closed? You will probably have better performance if you log after you close the connection. However, I think simple logic and readability is worth the trade-off of logging in your catch and close the connection in the finally.

Answer 4

To achieve separation of concerns, you need to pull the error handling out of that method. There are 3 ways to do that:

1) (least elegant) Use two methods:

public void InsertEmployee(EmployeeDetails emp)
{
    ...
}

public void TryInsertEmployee(EmployeeDetails emp)
{
    try
    {
     InsertEmployee(emp);
    }
    catch (Exception e)
    {
     logger.Error(e);
     throw new ApplicationException("Data error.");
    }
}

2) Use the decorator pattern.

public class ErrorHandlingEmployeeGateway
{
    ...
}

3) Use AOP

[LogException]
public void InsertEmployee(EmployeeDetails emp)
{
    ...
}