RehanSaeed / Serilog.Exceptions

Log exception details and custom properties that are not output in Exception.ToString().
MIT License
514 stars 52 forks source link

Add destructurer for SocketException #249

Closed TysonMN closed 4 years ago

TysonMN commented 4 years ago

I suggest adding a destructor for SocketException. I volunteer to do this work, but I also want to do a bit more than the naive implementation.

SocketException has a few properties related to the error (namely ErrorCode, NativeErrorCode, and SocketErrorCode). However, they are all a bit cryptic. The first two are of type int and the last one is of type SocketError, which is an enum. Of course it also has the property HelpLink inherited from Exception. If there was ever a time to set that value, it would be now, but I have only seen it set to null. I think a great help page is the documentation for SocketError, which has a short phrase explaining each case of the enum.

Alongside the other properties of SocketException, I would prefer to also add that URL for the key SocketErrorCodeHelpLink.

How does this sound?

RehanSaeed commented 4 years ago

Enriching the exceptions is an interesting idea. I can see only two issues:

  1. Increasing log size is something that worries people. It costs money to store more JSON in ElasticSearch for example.
  2. Microsoft always seems to break their links after a few years.

If you only want to add SocketErrorCodeHelpLink, that doesn't sound too bad but wouldn't it just always have the same link?

TysonMN commented 4 years ago

Increasing log size is something that worries people. It costs money to store more JSON in ElasticSearch for example.

If an application is throwing (and logging) SocketExceptions so frequently that an extra property and URL is using too much space (whatever the sink), then the application has more important problems to worry about.

Application-level enrichers are common place (like the machine name enricher) and will consume much more space than what I am proposing. Each sink already has to confront this duplication and decide what to do about it. To save on space, a table with the unique values could be created. Then each log event wild contain references to entries in this table. It is a classic time-space tradeoff. I don't think Serilog.Exceptions should limit itself because some sinks optimize for time over space.

However, for those overly concerned about storage space, they can filter out all Serilog.Exceptions with selective enrichment or (I think) just this addition property we are discussing via the property filtering feature of Serilog.Exceptions.

When there is an exception, the default behavior

  1. could be primarily provide lots of useful information and secondarily consider space usage or
  2. could be primarily optimize for little space usage and secondarily squeeze in some useful information.

Like Nicholas says in his selective enrichment post:

Enrichers need to be selected carefully, but more often than not, ease of diagnostics wins over collection costs, and events still end up with a lot of additional properties.

TysonMN commented 4 years ago

Microsoft always seems to break their links after a few years.

I agree about the fragility of the URL.

In my local implement of this, I didn't log the URL. Instead, I copied the sentence of documentation for each enum case into a dictionary, used the enum value to obtain the sentence of documentation, and enriched that information instead of a URL. This is more useful because it doesn't require an internet connection and doesn't require the loading and searching of a webpage.

How about we do the same here?

RehanSaeed commented 4 years ago

That sounds good to me.